Skip to content

Issue #121 (ginkelsoft-development/buildora): onderzoeken of FileField een verplichte extensie/MIME-whitelist en deny-list voor executables mist, en een reproductie/testplan opleveren. - #183

Open
zyra-ginkelsoft[bot] wants to merge 1 commit into
mainfrom
zyra/issue-121-ginkelsoft-development-buildor-50144776
Open

zyra-ginkelsoft[bot] wants to merge 1 commit into
mainfrom
zyra/issue-121-ginkelsoft-development-buildor-50144776

Conversation

@zyra-ginkelsoft

Copy link
Copy Markdown
Contributor

Nu ga ik de FileField class en gerelateerde bestanden bekijken.Bevestigd: accept() is puur client-side (Alpine.js check + HTML accept-attribuut). Laten we het volledige blade-component bekijken.We hebben geen root-pakket voor gd beschikbaar. We gebruiken --ignore-platform-req=ext-gd bij composer install, zoals bij eerdere issues waarschijnlijk ook gedaan is..zyra/proef.sh bestaat al in deze werkplaats. Laten we het bekijken en checken of het werkt.Dat bestaat al en lijkt goed. Laten we composer install draaien om te testen.Composer install gelukt. Nu bekijk ik de bestaande testsuite structuur om te weten hoe ik reproductietests moet opzetten.Goed, defineFields() is de enige abstracte methode. Nu bekijk ik resolveFields en getModelInstance en FieldValidator::validate.Route bestaat waarschijnlijk, en wordt bij het laden van de service provider geregistreerd. Direct de controller aanroepen zou moeten werken zolang route('buildora.index', ...) bestaat. Laten we dat kort verifiëren en dan de fixtures/tests schrijven.Goed, die route bestaat. Nu maak ik de fixtures en de testbestanden.Nu het feature-testbestand.Nu draai ik deze tests om te zien of ze slagen (het huidige gedrag aantonen).Model moet HasBuildora trait gebruiken. Laten we dat bekijken.Ik moet Ginkelsoft\Buildora\Traits\HasBuildora toevoegen aan de fixture-model.Alle vier reproductietests slagen (bewijzen het huidige onveilige gedrag). Laten we de PHPUnit deprecations checken (waarschijnlijk /** @test */ annotatie-stijl, zoals ook in bestaande tests).Bevestigd: dit is bestaand gedrag in de repo (de @test-annotatie deprecation), niet iets dat ik veroorzaak. Nu voeg ik nog een pure unit-test toe voor FileField zelf om de kernoorzaak (geen automatische validatieregels) te documenteren op veldniveau, en draai daarna de volledige testsuite + phpstan + lint.phpstan/lint zijn alleen op src/, dus onze tests/fixtures beïnvloeden die niet. Nu voeg ik de aanvullende unit-test toe voor FileField.Nu draai ik de volledige testsuite, phpstan en lint.Bevestigd: geen enkel bestaand (tracked) bestand is gewijzigd — alleen nieuwe testbestanden toegevoegd. De 4 falende tests (CurrencyField/FieldContract) zijn pre-existing reproductietests van andere issues (#142/#143), niet door mij veroorzaakt. Nu draai ik lint en phpstan op src/.Nu controleer ik of .zyra/proef.sh werkt (tot en met migraties, zonder de server oneindig te laten draaien).Migraties werken. Laten we nog kort testen dat de server opstart..zyra/proef.sh werkt correct. Nu ruim ik de testartefacten van deze handmatige proefrun op (sqlite db-bestand) zodat die niet per ongeluk gecommit worden.Geen zwerfbestanden. Nu commit ik het werk.## Samenvatting

Onderzoek FileField — verplichte extensie/MIME-whitelist en deny-list voor executables ontbreken (issue #121)

Bevinding (Hoog — CWE-434, Unrestricted Upload of File with Dangerous Type / OWASP A04:2021 Insecure Design)

FileField (src/Fields/Types/FileField.php) biedt drie configuratiemethodes die een ontwikkelaar laten geloven dat er beperkingen gelden:

  • ->accept('image/*')
  • ->maxSize(200)
  • ->imageDimensions(800, 600)

Deze worden echter nergens omgezet naar server-side validatieregels. Ze vullen alleen:

  • de helptekst (getHelpText()),
  • het HTML accept-attribuut en een client-side Alpine.js-check in resources/views/components/input/file.blade.php.

FileField::getValidationRules() (via HasValidation-trait) retourneert standaard een lege array — tenzij de resource-auteur zelf expliciet ->validation(['veld' => 'mimes:...|max:...']) toevoegt. BuildoraController::store()/update() roept $request->validate($validationRules) aan en gebruikt daarna direct $uploadedFile->store(...) zonder enige extra typecontrole.

Exploitatie: een aanvaller met toegang tot het formulier (of die de request direct via curl/Postman stuurt, dus de client-side JS/HTML-accept omzeilt) kan bij elke FileField — ook eentje met ->accept('image/*') — gewoon een .php/.phtml/.phar-bestand of een bestand van willekeurige grootte uploaden en laten opslaan. Of dit tot RCE leidt hangt af van de disk-configuratie (bv. publieke disk die door de webserver als PHP wordt uitgevoerd), maar de package biedt zelf geen enkele drempel.

Wat al wél gemitigeerd is (niet door Buildora, maar door Laravel): UploadedFile::store() gebruikt standaard een willekeurige hash-bestandsnaam i.p.v. de door de client opgegeven naam, dus padtraversal via de bestandsnaam (../../etc/passwd) leidt niet tot het escapen van de upload-map. Dit is framework-gedrag, geen bewuste beveiliging van Buildora — een toekomstige "originele bestandsnaam behouden"-feature kan dit stilzwijgend doorbreken.

Fix-voorstel voor de vervolgtaak

  1. FileField::allowedMimeTypes(array $mimes) toevoegen die verplicht is of een veilige default heeft.
  2. Altijd-actieve deny-list voor executable/gevaarlijke types (.php, .phtml, .php3-8, .phar, .cgi, .pl, .exe, .sh, .htaccess, …), ongeacht wat accept() toestaat.
  3. getValidationRules() van FileField laten samenstellen op basis van accept()/maxSize()/imageDimensions() (auto-genereren van mimes:/mimetypes:/max:/dimensions:), zodat de UI-configuratie ook echt wordt afgedwongen.
  4. Server-side contentdetectie via finfo/Symfony MimeTypes i.p.v. vertrouwen op client-Content-Type, met een expliciete check tegen de deny-list.
  5. UUID/random-naamgeving bij opslag bevestigen/expliciet maken (nu impliciet via Laravel store()), zodat dit niet per ongeluk verdwijnt bij toekomstige wijzigingen.

Getest

  • Nieuwe unit-tests (tests/Unit/Fields/FileFieldTest.php): bevestigen dat accept()/maxSize()/imageDimensions() geen enkele validatieregel opleveren.
  • Nieuwe feature-tests (tests/Feature/FileUploadSecurityTest.php) via BuildoraController::store() met een fixture-resource/model (tests/Fixtures/): tonen aan dat een .php/.phtml/.phar-upload en een te groot bestand gewoon worden opgeslagen ondanks ->accept('image/*')/->maxSize(200); en dat padtraversal via bestandsnaam niet werkt (huidig, framework-gedreven, mitigatie).
  • Volledige testsuite gedraaid: 4 nieuwe + 4 nieuwe = alle nieuwe tests slagen. De suite bevat 4 al bestaande, pre-existing falende tests (issues Bug: Field::make() gebruikt new self() — HasOneField/ViewField breken #142/Bug: CurrencyField crasht op string-input (number_format type error) #143, CurrencyField/FieldContract) — niet door mij veroorzaakt, geen bestaand bestand door mij gewijzigd (git diff --stat op tracked files is leeg).
  • composer lint en composer analyse (phpstan) op src/ draaien met vooraf bestaande bevindingen, ongewijzigd t.o.v. main — er is geen productiecode aangeraakt.
  • .zyra/proef.sh bestond al en werkt: composer install (met --ignore-platform-req=ext-gd fallback voor de ontbrekende gd-extensie hier), testbench sqlite-db + migraties, en testbench serve op 0.0.0.0:$PORT getest en bevestigd werkend.

Open staat

  • De daadwerkelijke fix (whitelist/deny-list/finfo-detectie/UUID-naming) is een aparte taak (Noor); deze tests zijn bewust rood-voor-de-fix, groen-nu (ze leggen het huidige onveilige gedrag vast en kunnen als regressietoets dienen zodra de fix er is — de asserts moeten dan worden omgedraaid).
  • Ik heb geen toegang gebruikt om daadwerkelijk op GitHub issue Security: FileField extensie/MIME whitelist verplicht stellen #121 te reageren; bovenstaande tekst is bedoeld als concept-reactie/PR-beschrijving.
  • Niet onderzocht: InlineRelationController slaat geen bestanden op (geen FileField-verwerking daar), dus buiten scope; wel het overwegen waard bij de fix of daar in de toekomst dezelfde upload-flow terechtkomt.

…ld mist een verplichte extensie/MIME-whitelist en deny-list voor executables.

FileField::accept()/maxSize()/imageDimensions() zijn puur decoratief (help-tekst
+ HTML accept-attribuut + client-side Alpine.js-check) en worden nooit omgezet
naar server-side validatieregels. BuildoraController::store()/update() valideert
uploads alleen als een resource-auteur zelf expliciet ->validation(['veld' =>
'mimes:...']) toevoegt; standaard is er geen enkele beperking op bestandstype
of -grootte.

Nieuwe tests leggen dit huidige gedrag vast:
- FileFieldTest: accept()/maxSize()/imageDimensions() leveren geen
  validatieregels op.
- FileUploadSecurityTest: de controller slaat een .php/.phtml/.phar-bestand
  gewoon op ondanks ->accept('image/*'), en een bestand van 5MB ondanks
  ->maxSize(200). Ook vastgelegd: padtraversal via de bestandsnaam werkt niet,
  omdat Laravel's UploadedFile::store() standaard een willekeurige hash-naam
  gebruikt (mitigatie van het framework, niet van Buildora).

Geen productiecode gewijzigd; alleen onderzoek + reproductietests voor de
vervolgtaak die de fix bouwt.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants