SQL-injectie bij numerieke ID's
Als ik via GET nummers meegeef en ik doe natuurlijk ook controle of het wel nummers zijn, moet ik dan nog werken met mysqli_real_escape_string?
is (int)$_GET['id'] voldoende of niet
Eveneens gelinkt aan deze vraag; de 2° parameter is een string van 1 karakter(slechts 7 mogelijkheden) opgevangen door een switch en omgezet naar de juiste databasestring van ook 1 karakter. Zelfde vraag als hierboven.
Dank bij voorbaat.
Jan
Ward:
Titel aangepast.
Gewijzigd op 24/08/2018 07:44:22 door Ward van der Put
https://stackoverflow.com/questions/5052932/how-to-get-int-instead-string-from-form
Allereerst moet je je realiseren dat het hier om twee verschillende dingen gaat:
* het veilig opnemen van USER DATA in een query
Je wilt dat je queries veilig zijn.
* validatie van de USER DATA
Je wilt dat wat je in je database stopt ook voldoet aan bepaalde regels.
Deze twee dingen dienen verschillende doelen en zul je dus ook in afzondering moeten behandelen.
Quote:
is (int)$_GET['id'] voldoende of niet
Dit is niet echt rechtstreeks te beantwoorden. Ja, het is voldoende in die zin dat een typecast ervoor zorgt dat een hele hoop rotzooi (en mogelijk alle?) uit $_GET['id'] wordt verwijderd. Nee, omdat er mogelijk nog steeds de grootst mogelijke onzin in staat, zo zal, als $_GET['id'] "aap" bevat, deze omgezet worden naar de integer 0.
Een typecast, of alles wat een poging doet om de invoer te repareren -en daarmee ook om te zetten- is wat mij betreft ongeschikt omdat dit mogelijk recht probeert te buigen wat krom is. Het is zaak dat je een validatie-stap hebt voordat je data wegschrijft. Voldoet de invoer niet, dan is deze gewoon fout en geef je deze terug aan de gebruiker om deze te repareren.
Het gebruik van een whitelist is aan te bevelen als de invoer slechts één of enkele waarden zou mogen hebben. Dit is dan onderdeel van je validatie-routine waarbij je kijkt of de waarde voorkomt in een lijst van toegestane waarden.
Voor de goede orde, als $_GET['id'] bijvoorbeeld refereert aan het id van een record in de database, zou op het bestaan van het record gecontroleerd moeten worden. En het inserten van nieuwe data gekoppeld aan dit record (of het updaten van de data van dit record zelf) zou in een transactie moeten zitten waarin het record waaraan gerefereerd wordt gelocked zou moeten worden. Zodat deze hele transactie één ondeelbaar geheel vormt.
Het belangrijkste bij het veilig maken van je queries lijkt mij dat je altijd consequent dezelfde methode toepast. Als je bepaalde data wel escaped met real_escape_string() (in combinatie met quotes, het een is niet veilig zonder het ander!) en andere niet, dan werkt dat verwarrend. Immers, wat betekent het dat een bepaald stuk data niet ge-escaped is? Betekent dit dat dit niet nodig was, of dat het toch vergeten was? Dit werkt gewoon verwarrend. Escape gewoon altijd en overal. Ook al is het niet direct nodig. Dit zorgt voor een consistente omgang met externe (user) data, waarbij je niet bij elk item een afweging hoeft te maken of het wel of niet nodig is om te escapen.
Maar validatie en het veilig maken van queries zijn dus twee compleet verschillende dingen...
Gewijzigd op 23/08/2018 19:12:23 door Thomas van den Heuvel
id: start met is_numeric dan (int) en dan bestaat het id wel
waarde indien niet in lijst ==> verworpen
Ik zal dus ook nog escapen.
Overigens een tipje voor dit soort checks: je kunt ook een array binnen krijgen: index.php?id[]=5. In de $_GET heb je nu een array met een entry met waarde 5. Omdat checks hier vaak niet op rekenen kun je soms toch net even verder komen dan bedoelt (of in ieder geval de boel goed in de soep laten lopen).
Als dit onderdeel uitmaakt van een administratief systeem zou ik gewoon maatregelen treffen zodat de data in dit systeem kloppend is en kloppend blijft.
Vervolgens is het ook zaak om te controleren of dit een bestaand record (en/of een record waar iemand toegang toe heeft!) betreft. Dit regel je in een transactie met FOR UPDATE.
Waarom niet ctype_digit?
Jan R op 23/08/2018 17:02:37:
Als ik via GET nummers meegeef en ik doe natuurlijk ook controle of het wel nummers zijn, moet ik dan nog werken met mysqli_real_escape_string?
Nee, na een controle op nummers is escapen helemaal nergens voor nodig …
Jan R op 23/08/2018 17:02:37:
is (int)$_GET['id'] voldoende of niet
… en ja, is een type cast inderdaad voldoende.
Gewijzigd op 24/08/2018 07:47:48 door Ward van der Put
Behalve dan dat ook niet-geldige invoer wordt omgezet naar een getal, zoals Thomas in z'n eerste post uitlegt. Je doet in dit geval dus niet een controle of het wel of niet een getal is, maar je zet alles om naar een getal.
Quote:
Als ik via GET nummers meegeef en ik doe natuurlijk ook controle of het wel nummers zijn […]
Kortom, als je al een controle uitvoert op de geldigheid van nummers die je nota bene zelf genereert, dan cast (int) eventueel ongeldige invoer naar niets meer dan een 0 die bij een AUTO_INCREMENT ook nog ongeldig is.
Quote:
Nee, na een controle op nummers is escapen helemaal nergens voor nodig …
Maar zoals ik hier boven in mijn relaas aangeef spelen er ook andere overwegingen - je wilt een consistente werkwijze waarbij je alle user data op eenzelfde wijze behandelt zodat je je niet bij elk DATA-deel in je SQL wilt afvragen "is dit nu expres niet ge-escaped omdat dit niet nodig is of ben ik dit hier toch vergeten?". Dit werkt gewoon niet prettig en nog belangrijker, dit is erg foutgevoelig. Dus, pas overal dezelfde werkwijze toe, of gebruik prepared statements zoals @Rob aangeeft, die dwingt dit, mits correct toegepast, voor je af.
Quote:
Waarom niet ctype_digit?
Accepteert meer invoer dan waarin je geïnteresseerd bent. Als je een auto-increment id verwacht, dan zou ik een controle uitvoeren die daarop is toegespitst en enkel een (syntactisch) geldig auto-increment id accepteert. Dit is dan je syntactische check, uiteraard moet je dan ook nog een semantische check uitvoeren waarin je controleert of het id ook daadwerkelijk refereert aan een bestaand record.
@Ward heeft in die zin gelijk dat het geen "risico's" oplevert ten aanzien van de veiligheid van de query, en ook levert zo'n typecast een syntactisch correcte query op, maar tegelijkertijd houdt zo'n typecast mogelijk ook in dat je potentieel verkeerde invoer aanpast/"repareert" (wat mij onwenselijk lijkt) en de query waarin het cijfer 0 dan wordt opgenomen zal ook nooit het gewenste resultaat opleveren. De vraag is dan of als de query geen resultaat oplevert of dan de verdere verwerking van data wordt stopgezet/teruggedraaid zodat er geen rotzooi in de database komt.
Het lijkt mij gewoon makkelijker om alles aan de poort te controleren en als dit in eerste instantie al niet (exact) voldoet dat je dan alle vervolgacties (zoals queries die dan toch geen zinnig resultaat meer opleveren) gewoon afblaast.
Gewijzigd op 24/08/2018 14:54:23 door Thomas van den Heuvel
Ik snap wat je bedoelt hoor Ward, maar het is geen controle in die zin dat je controleert of de input klopt. Dat is wat ik bedoelde te zeggen.
Stel je hebt vierkantjes nodig. Wat je doet bij een controle is kijken of alles wat binnenkomt een vierkant is. Zo niet, dan staak je de huidige actie. Bij typecasting zeg je, maakt niet uit wat er allemaal binnenkomt (rondjes, driehoeken enz.) ik maak overal een vierkantje van en ga vrolijk verder. Dat kan, maar het is iets anders dan een controle.
Ik beantwoorde daarmee de concrete vraag wat directer (ga je met die stappen SQL-injectie tegen), maar we zijn het verder vooral erg eens. ;-)
Zelf gebruik ik voor dit soort ID's overigens vaak data objects juist omdat je daarmee méér kunt formaliseren dan alleen een "dit is een integer". Bij een AUTO_INCREMENT mag zo'n data object bijvoorbeeld niet kleiner dan 1 zijn, iets wat bij een gewone integer wel mag. Voldoet input niet aan de norm, dan rolt er al bij het genereren van het data object een exception uit.
Haha, dan is het goed :-)
Quote:
Als je niet 100% zeker weet dat die controle helemaal waterdicht is, kun je ná die controle nog een typecast naar een integer toepassen.
Of je zorgt ervoor dat de controle de lading wel dekt of dat het opnemen van DATA in SQL altijd veilig is (of nog beter, beide)? :/
Ja, je gaat met een typecast wellicht SQL-injectie tegen, maar dit is zoiets als je tuin in de fik steken met kerosine, omdat dit een insektenplaag kan voorkomen. Het neemt dan weliswaar een ongewenst effect weg, maar introduceert weer andere ongewenste dingen.
Dan moet je je afvragen hoe je je andere DATA-delen beschermt in je query, gebruik je hiervoor andere methoden/truuks? Zou het niet veel makkelijk zijn om hier een (één) lijn in aan te brengen? Zodat je - als je over een tijd je code nog eens overleest of raadpleegt, je niet elk element moet inspecteren om te zien wat hier aan de hand is? Al die elementen zullen dan stuk voor stuk op een juiste (specifieke) aanpak geïnterpreteerd moeten worden. Elke keer opnieuw. Idealiter wil je een makkelijke manier die veilig is, en dat is echt niet moeilijk om voor elkaar te krijgen.
En nogmaals, het controleren/repareren van invoer en het op een veilige manier uitvoeren van queries zijn twee verschillende dingen. Behandel deze dan ook in afzondering (separation of concerns).
Ah well, ik val in herhaling.