Repository navigation
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
Open
zyra-ginkelsoft[bot] wants to merge 1 commit into
zyra-ginkelsoft[bot] wants to merge 1 commit into
Conversation
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nu ga ik de FileField class en gerelateerde bestanden bekijken.Bevestigd:
accept()is puur client-side (Alpine.js check + HTMLaccept-attribuut). Laten we het volledige blade-component bekijken.We hebben geen root-pakket voor gd beschikbaar. We gebruiken--ignore-platform-req=ext-gdbij composer install, zoals bij eerdere issues waarschijnlijk ook gedaan is..zyra/proef.shbestaat 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 ikresolveFieldsengetModelInstanceenFieldValidator::validate.Route bestaat waarschijnlijk, en wordt bij het laden van de service provider geregistreerd. Direct de controller aanroepen zou moeten werken zolangroute('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 moetHasBuildoratrait gebruiken. Laten we dat bekijken.Ik moetGinkelsoft\Buildora\Traits\HasBuildoratoevoegen 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 voorFileFieldzelf om de kernoorzaak (geen automatische validatieregels) te documenteren op veldniveau, en draai daarna de volledige testsuite + phpstan + lint.phpstan/lint zijn alleen opsrc/, 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 opsrc/.Nu controleer ik of.zyra/proef.shwerkt (tot en met migraties, zonder de server oneindig te laten draaien).Migraties werken. Laten we nog kort testen dat de server opstart..zyra/proef.shwerkt 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.## SamenvattingOnderzoek 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:
getHelpText()),accept-attribuut en een client-side Alpine.js-check inresources/views/components/input/file.blade.php.FileField::getValidationRules()(viaHasValidation-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
FileField::allowedMimeTypes(array $mimes)toevoegen die verplicht is of een veilige default heeft..php,.phtml,.php3-8,.phar,.cgi,.pl,.exe,.sh,.htaccess, …), ongeacht wataccept()toestaat.getValidationRules()vanFileFieldlaten samenstellen op basis vanaccept()/maxSize()/imageDimensions()(auto-genereren vanmimes:/mimetypes:/max:/dimensions:), zodat de UI-configuratie ook echt wordt afgedwongen.finfo/SymfonyMimeTypesi.p.v. vertrouwen op client-Content-Type, met een expliciete check tegen de deny-list.store()), zodat dit niet per ongeluk verdwijnt bij toekomstige wijzigingen.Getest
tests/Unit/Fields/FileFieldTest.php): bevestigen dataccept()/maxSize()/imageDimensions()geen enkele validatieregel opleveren.tests/Feature/FileUploadSecurityTest.php) viaBuildoraController::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).git diff --statop tracked files is leeg).composer lintencomposer analyse(phpstan) opsrc/draaien met vooraf bestaande bevindingen, ongewijzigd t.o.v. main — er is geen productiecode aangeraakt..zyra/proef.shbestond al en werkt: composer install (met--ignore-platform-req=ext-gdfallback voor de ontbrekende gd-extensie hier), testbench sqlite-db + migraties, entestbench serveop0.0.0.0:$PORTgetest en bevestigd werkend.Open staat
InlineRelationControllerslaat geen bestanden op (geenFileField-verwerking daar), dus buiten scope; wel het overwegen waard bij de fix of daar in de toekomst dezelfde upload-flow terechtkomt.