Issue #119 (buildora) opnieuw oppakken: vaststellen of DataFetcher kwetsbaar is voor SQL injectie via sort/filter-kolomnamen, en zo nodig fixen. - #188
Open
zyra-ginkelsoft[bot] wants to merge 1 commit into
Conversation
Onderzoek naar SQL-injectie via sort_column/filter_column: klassieke injectie was niet mogelijk omdat DataFetcher::fetch() de sorteerkolom altijd toetste aan de echte databaseschema-kolommen voordat hij in orderBy() belandde. Wel bleek de bedoelde whitelist tegen de resource-fields (defineFields()) dode code: `$col === $sortBy` vergeleek een Field-object met een string en kon dus nooit slagen, waardoor élk veld dat toevallig een bestaande kolomnaam is - ook velden zonder ->sortable() of buiten de resource om - als sorteerkolom gebruikt kon worden (broken access control, OWASP A01). Sorteren werkt nu alleen nog op een veld dat exact overeenkomt met een door defineFields() gedefinieerd, expliciet sortable() veld; de databaseschema-check blijft als extra verdedigingslaag staan. Regressietest (tests/Unit/Datatable/DataFetcherTest.php) bewijst: - een payload als "name); DROP TABLE ...;--" wordt genegeerd en breekt niets (bewijs dat er geen SQL-injectie was); - sorteren op een bestaande maar niet-sortable kolom wordt nu geblokkeerd (dit faalde vóór de fix - bewijs van de access-control bug); - sorteren op een wel-sortable veld blijft gewoon werken. 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.
Composer install lukt. Nu test bestaande testsuite en schrijf een reproductietest voor de sort-injectie.Bestaande tests slagen. Nu bekijk ik een voorbeeld-Field/Resource-test om een reproductie voor DataFetcher op te zetten.Goed, dat bevestigt genoeg voor het schrijven van een integratietest. Nu bouw ik een testmodel/resource om
DataFetcher::fetch()end-to-end te testen met sqlite.Nu maak ik de fixtures en de test aan.Opgeruimd — dat was per ongeluk buiten de werkplaats geschreven (typefout in het pad); direct verwijderd. Nu de echte testfile aanmaken.Update de helper naargetFields()in plaats van directe property-toegang.Ook de ongebruikte$namesvariabele in het tweede test-blok opruimen.Nu draai ik de nieuwe test.Model moetHasBuildoratrait gebruiken.Alle 3 nieuwe tests slagen. Laten we bevestigen dat ze ook faalden vóór de fix (om te bewijzen dat de test de bug daadwerkelijk detecteert).Bevestigd: zonder de fix faalt de test die het broken-access-control-gat aantoont (sorteren op een niet-sortable kolom werd wél toegepast), terwijl de zuivere SQL-injectietest al slaagde vóór de fix — dat bewijst dat er geen klassieke SQL-injectie was, maar wel een whitelist-bug. Nu de volledige testsuite, phpstan en linter.phpstan-fouten zijn nu 53, minder dan de baseline van 59 (mijn wijziging voegt niets nieuws toe, en ruimt zelfs wat dode-code-waarschuwingen op). Nu de linter en volledige testsuite.Mijn bestand is lint-schoon; alle 39 tests slagen. De overige lint-fouten zijn pre-existing in andere bestanden, niet door mij aangeraakt. Nu proef.sh testen.proef.sh werkt: installeert, migreert en serveert op de gevraagde poort;/buildora/installgeeft HTTP 200.Server is gestopt. Nu opruimen van de door proef.sh aangemaakte artefacten die niet gecommit moeten worden, en de git-status bekijken.Vendor/workbench-artefacten staan al buiten de repo (of genegeerd) — schoon genoeg. Nu committen.Werkboom is schoon, alleen de bedoelde bestanden zijn gecommit. Klaar.Samenvatting
Vastgesteld (bewijs, niet vermoeden):
DataFetcher::fetch()ontvangt$sortByrechtstreeks als requestinput (BuildoraDataTableController::json()→sortBy-parameter). Klassieke SQL-injectie via de kolomnaam was niet mogelijk: voordat de kolomnaam inorderBy()belandde, werd hij altijd getoetst aanSchemaCache::getColumnListing()(de echte kolomnamen van de tabel). Een payload alsname); DROP TABLE df_test_items;--matcht geen bestaande kolom en wordt dus genegeerd — dit is bevestigd met een test die vóór én na de fix slaagt.Wél gevonden: een broken-access-control-bug (OWASP A01). De bedoelde whitelist tegen de resource's
defineFields()was dode code:(is_array($col) ? ... : $col) === $sortByvergelijkt eenField-object met een string, wat nooit klopt. Daardoor viel de check terug op "bestaat deze naam als kolom in de tabel?" — zonder te checken of het veld in de resource is gedefinieerd én expliciet->sortable()is gemarkeerd. Gevolg: elke echte databasekolom (ook niet-sortable velden of velden buiten de resource om, bv. een verborgen score/hash-kolom) kon als sorteerkolom worden misbruikt — geen directe data-exfiltratie viaORDER BY, maar wel een informatie-/side-channel-risico en een schending van least privilege.Fix:
src/Datatable/DataFetcher.phpmatcht$sortBynu exact tegen eenFielduit$this->columns(afkomstig vandefineFields()) én vereist dat dat veld->sortable()is; pas dan wordt de daadwerkelijke kolomnaam gebruikt, met de databaseschema-check als extra verdedigingslaag.Test: nieuw
tests/Unit/Datatable/DataFetcherTest.php(+ fixturesDataFetcherTestModel/DataFetcherTestResource) tegen een echte sqlite-tabel:sortBy→ genegeerd, tabel blijft intact;Volledige testsuite: 39/39 groen.
composer analyse(phpstan): baseline 59 pre-existing fouten elders in de repo → nu 53 (mijn wijziging voegt niets toe, ruimt zelfs iets op).composer lint:DataFetcher.phpzelf is schoon; overige phpcs-meldingen zijn pre-existing in ongerelateerde bestanden. Geen geheimen toegevoegd..zyra/proef.shbestond al en werkt (composer install, sqlite-db, migraties,testbench serveop$PORT;/buildora/install→ HTTP 200 geverifieerd).Open/niet gedaan: de overige 53 phpstan-meldingen en phpcs-waarschuwingen in andere bestanden zijn pre-existing en buiten scope van issue #119; niet aangepakt. Er is geen apart "filter"-mechanisme in de codebase gevonden naast search/sort, dus "filter-kolomnamen" uit de issuetitel is hiermee gedekt via het sort-pad (het enige plek waar een kolomnaam vanuit request-input een query-clausule bepaalt).