Is dit kwetsbaar voor SQL injection?

[Verwijderd]

Naar aanleiding van het frontpage artikel over de blunder van Kluwer Software, een vraagje (ik had hem bij dat artikel ook al gepost maar wellicht is het hier beter op z'n plaats).

Iemand stelde dat je geen SQL queries binnen code zou moeten construeren, of uberhaupt geen SQL queries in je code zou moeten hebben staan.

Nu vraag ik me af, wat is het probleem met dit:

code:

$name = mysql_real_escape_string($_POST['naam']);
$salt = 'qG(KXen*637Of2b#vR-p9';
$pasw = hash('sha512',$_POST['wachtwoord'].$salt);
$query = "SELECT * FROM gebruikers WHERE name='$name' AND pasw='$pasw' LIMIT 1";
mysql_query($query);

(even kort door de bocht, ik weet dat mysql_real_escape_string niet in alle gevallen zaligmakend is, hoewel hier volgens mij wel)

[NMe]

Verwijderd schreef op vrijdag 06 januari 2012 @ 14:36:
Iemand stelde dat je geen SQL queries binnen code zou moeten construeren, of uberhaupt geen SQL queries in je code zou moeten hebben staan.

Onzin, je queries moeten toch ergens staan. Het is de manier waarop je je queries bouwt die bepaalt of het al dan niet veilig is.

Zowel $name als $pasw kunnen geen quotes bevatten die niet escaped zijn en kunnen dus je string niet afbreken. Dit is dus gewoon veilig voor SQL-injectie.

[SaphuA]

.

[ Voor 98% gewijzigd door SaphuA op 31-01-2022 15:41 ]

[RobIII]

Dit specifieke geval is niet^* kwetsbaar voor SQL injection: De 2 parameters die je dynamisch in je query mikkert worden ofwel escaped ($name) ofwel zullen nooit andere tekens bevatten dan 0-9/A-F.

Ikzelf prefereer wel parameterized queries boven 't zelf aan elkaar plakken van strings, maar als je consequent bent/blijft en daarbij ook alert en secuur dan is jouw methode prima te doen.

* Even er van uit gaand dat mysql_real_escape_string z'n werk goed doet uiteraard

[ Voor 44% gewijzigd door RobIII op 06-01-2012 14:43 ]

[NMe]

SaphuA schreef op vrijdag 06 januari 2012 @ 14:39:
Je geeft het antwoord eigenlijk zelf al

http://stackoverflow.com/...t-for-cleaning-user-input

Leuk hoe ze daar weer prepared statements opgooien als be all and end all tegen SQL-injectie. Dat is inderdaad een bijproduct van prepared statements, maar uiteindelijk is het niet het doel ervan.

[Verwijderd]

OK duidelijk.

Ik zou in de praktijk ook nog wel een max lengte op die $POST variabelen zetten, want ik weet bijvoorbeeld niet wat er gebeurt als iemand een naam van 3 miljard karakters zou submitten

Maar wat de SQL injectie betreft ben ik gerustgesteld, bedankt.

[NMe]

Verwijderd schreef op vrijdag 06 januari 2012 @ 14:50:
OK duidelijk.

Ik zou in de praktijk ook nog wel een max lengte op die $POST variabelen zetten, want ik weet bijvoorbeeld niet wat er gebeurt als iemand een naam van 3 miljard karakters zou submitten

Niets, totdat hij die 3GB aan data naar je site heeft verstuurd. Sowieso staat er op de meeste webservers een setting voor de maximale grootte van een POST-request.

Een maxlength beveiligt je daar trouwens ook niet tegen, HTML heb ik zo aangepast in Firebug of desnoods maak ik zelf even een formuliertje dat naar jouw site postbackt.

[cariolive23]

NMe schreef op vrijdag 06 januari 2012 @ 14:44:
[...]

Leuk hoe ze daar weer prepared statements opgooien als be all and end all tegen SQL-injectie. Dat is inderdaad een bijproduct van prepared statements, maar uiteindelijk is het niet het doel ervan.

Niet het doel, wel het resultaat. Het is lekker eenvoudig en werkt uitstekend, daar is toch niks mis mee?

[Verwijderd]

NMe schreef op vrijdag 06 januari 2012 @ 14:59:
Niets, totdat hij die 3GB aan data naar je site heeft verstuurd. Sowieso staat er op de meeste webservers een setting voor de maximale grootte van een POST-request.

Een maxlength beveiligt je daar trouwens ook niet tegen, HTML heb ik zo aangepast in Firebug of desnoods maak ik zelf even een formuliertje dat naar jouw site postbackt.

Ja, ik bedoelde server side

Dus bijv. zoiets:

code:

$name = mysql_real_escape_string(substr($_POST['naam'],0,1000));

Inderdaad zal het in de praktijk met 3GB daarvoor al mis gaan, maar stel dat er een lengte is die iemand nog wel daadwerkelijk weet te POSTen, maar waarvan de SQL parser op z'n bek gaat. Bijvoorbeeld. Ik kan die mogelijkheid niet uitsluiten in ieder geval.

[ Voor 36% gewijzigd door Verwijderd op 06-01-2012 15:20 ]

[T-MOB]

Haha, de reactie van de webmaster van Kluwer Software is echt tenenkrommend: "We hadden de maximale wachtwoordlengte aangepast naar twaalf tekens om dit soort aanvallen te voorkomen". Ongelofelijk dat je met zo weinig elementaire kennis over dit soort zaken bij zo'n groot bedrijf kunt komen te werken. Het verklaart wel waarom SQL-injectie nog tot problemen leidt, terwijl het zo simpel te voorkomen is.

Zelf controleer ik eigenlijk ook altijd in de software of de teruggegeven informatie uit de database klopt. In geval van het voorbeeld van TS zou ik me in de query beperken tot "SELECT * FROM gebruikers WHERE name='$name'" en dan in de software de loginstatus zetten aan de hand van een check op het wachtwoord. Ik vind dat beter leesbaar en het is gelijk wat defensiever.

Verwijderd schreef op vrijdag 06 januari 2012 @ 15:20:
Inderdaad zal het in de praktijk met 3GB daarvoor al mis gaan, maar stel dat er een lengte is die iemand nog wel daadwerkelijk weet te POSTen, maar waarvan de SQL parser op z'n bek gaat. Bijvoorbeeld. Ik kan die mogelijkheid niet uitsluiten in ieder geval.

MySQL heeft daar een max query length voor. Als je daar overheen gaat krijg je gewoon een database error, de parser gaat er in elk geval niet van op zijn bek.

[ Voor 25% gewijzigd door T-MOB op 06-01-2012 15:48 ]

[H!GHGuY]

Je script mag dan vandaag wel veilig zijn mbt SQL injectie, zoals hij er nu staat is het wachten op je collega (of jezelf) die later iets aanpast en alsnog de boel om zeep helpt.

Ofwel zet je er een comment bij die aangeeft waarom dit veilig is, ofwel fix je alsnog je script om prepared statements te gebruiken (zoals je natuurlijk in alle andere scripts doet ). Deze laatste oplossing heeft trouwens als extra voordeel dat je scripts er overal hetzelfde uitzien en dus sowieso onderhoudbaarder zijn.

[Ram0n]

NMe schreef op vrijdag 06 januari 2012 @ 14:44:
[...]

Leuk hoe ze daar weer prepared statements opgooien als be all and end all tegen SQL-injectie. Dat is inderdaad een bijproduct van prepared statements, maar uiteindelijk is het niet het doel ervan.

Het is bovendien geen heilige graal, aangezien niet alle SQL statements prepared opgebouwd kunnen worden. Jezelf aanleren hoe je veilig moet escapen is dus nog altijd nuttig.

[Verwijderd]

H!GHGuY schreef op vrijdag 06 januari 2012 @ 18:01:
Je script mag dan vandaag wel veilig zijn mbt SQL injectie, zoals hij er nu staat is het wachten op je collega (of jezelf) die later iets aanpast en alsnog de boel om zeep helpt.

Yep, daar sloeg de "even kort door de bocht" op Dit is een geïsoleerd voorbeeld maar in het echt is het meer gestandaardiseerd.

Idem voor dat hashen van een password, want natuurlijk niet random hardcoded tussendoor staat maar in een aparte functie (zodat bijvoorbeeld ook de salt string die ermee gemoeid is maar op één plek voorkomt).

[Ventieldopje]

Wat betreft die maximale $_POST lengte etc. Als je een goeie webhost hebt draait die php suhosin, een extensie om php "hardened" oftewel veiliger te maken, deze beperkt onder andere de maximale lengte / diepte van $_POST requests

Je code lijkt me veilig van SQL injectie zoals ook al eerder aangegeven, echter is een andere salt voor elk wachtwoord nog veiliger, je zou dus het random salt (niet gebaseerd op wat dan ook voor user informatie) bij in de user record in de database op kunnen slaan samen met een salt hard-coded in je code

[ Voor 3% gewijzigd door Ventieldopje op 07-01-2012 03:05 ]

[.oisyn]

NMe schreef op vrijdag 06 januari 2012 @ 14:39:
[...]

Onzin, je queries moeten toch ergens staan. Het is de manier waarop je je queries bouwt die bepaalt of het al dan niet veilig is.

Klopt, je query moet ergens staan. Maar het stukje waar je query staat hoeft nog niet zelf verantwoordelijk te zijn van het opbouwen ervan dmv string concatenation. Dat kun je beter uitbesteden om fouten te voorkomen.

[ Voor 96% gewijzigd door .oisyn op 07-01-2012 03:25 ]

[Ventieldopje]

Met name voor grote queries (denk veel velden en/of joins) wordt dat een zooitje en zorgt geregelt voor fouten die vrij lastig te vinden zijn

[Verwijderd]

Ventieldopje schreef op zaterdag 07 januari 2012 @ 03:02:
Wat betreft die maximale $_POST lengte etc. Als je een goeie webhost hebt draait die php suhosin, een extensie om php "hardened" oftewel veiliger te maken, deze beperkt onder andere de maximale lengte / diepte van $_POST requests

Ah, OK. Nooit van gehoord Ik heb ook niet altijd controle over de servers waar mijn code wordt gedraaid. Maar evengoed wel nuttig om te weten om zelf op te letten bij het kiezen van servers / providers.

Je code lijkt me veilig van SQL injectie zoals ook al eerder aangegeven, echter is een andere salt voor elk wachtwoord nog veiliger, je zou dus het random salt (niet gebaseerd op wat dan ook voor user informatie) bij in de user record in de database op kunnen slaan samen met een salt hard-coded in je code

Yep, nou in het echt prop ik behalve het password en een constante hardcoded salt (van random karakters) ook nog de UserID bij die hash in.