De PHPhulp kroeg
testen sql-injection
Wat ik ook probeer, uit mijn tabel tbWeeg krijg ik binnen het eerste record (weegId = 1) het veld weegkg niet gewijzigd met ' update tblWeeg set weegkg = 44 where weegId = 1; //--
Niet in een textfield en niet binnen een url. Overigens nu heeft weegkg de waarde 14.
Na submitten van het textfield wordt de waarde uit dit textfield gestopt in een variabele dmv
Code (php)
1
<?php $spgewicht = $_POST['txtspnkg']; ?>
de query ziet er vervolgens zo uit :
Code (php)
1
2
3
4
2
3
4
<?php
$query_schaap_bijwerken= "UPDATE tblschapen SET speendm ='$spdatum', speenkg = '$spgewicht'
WHERE schaapId = $schaapId " or die (mysql_error());
mysql_query($query_schaap_bijwerken); ?>
$query_schaap_bijwerken= "UPDATE tblschapen SET speendm ='$spdatum', speenkg = '$spgewicht'
WHERE schaapId = $schaapId " or die (mysql_error());
mysql_query($query_schaap_bijwerken); ?>
Als ik $query_schaap_bijwerken in het script aanpas wordt het update-statement wel uitgevoerd na submitten. Ook als ik het update statement in de variabele $spgewicht stop.
Als ik het update statement in het textveld plak zie ik niets gebeuren. Wat ik met puntkomma en enkele en/of dubbele quotes ook probeer.
Is dit een teken dat mijn code al is beveiligd of kan iemand mij vertellen wat ik niet goed doe ?
Gewijzigd op 06/02/2015 21:18:45 door Bas van de Ven
Klopt het dat er ergens in jouw code iets staat van
Code (php)
1
<?php $schaapId = $_POST['schaapId']; ?>
?
Dan stel ik voor om de <input name="schaapId"> te voor zien van de value
0 or 1=1
(ja, ook als die input hidden is, kan dat eenvoudig, maar misschien moet je hem voor de test even niet-hidden maken.)
Toevoeging op 06/02/2015 21:22:25:
los van het bewust proberen om de query verkeerd uit te laten voeren:
aangezien er sprake is van een gewicht, zou er ook sprake kunnen zijn van een lengte.
50cm is dan niet raar, maar iemand kan ook in inch of foot willen werken, en dan 3' invullen....
Wat ik als sql-injection wilde bereiken was een willekeurig update statement onderbreken (zie $query_schaap_bijwerken) door een ander update statement uit te laten voeren op een heel andere tabel. Nl. update tblWeeg set weegkg = 44 where weegId = 1; //--
Met jou voorstel heb ik ook een sql-injection bereikt. 1 veld in de hele tabel tblSchapen is nu voorzien van één en dezelfde waarde. Nu dan herstellen en moeilijker zien te maken d.m.v. een beveiliging.
Bedankt voor de hulp.
Code (php)
1
2
3
4
5
6
7
8
9
2
3
4
5
6
7
8
9
<?php
$query_schaap_bijwerken= "
UPDATE tblschapen
SET speendm ='". mysql_real_escape_string($spdatum) ."',
speenkg = '". mysql_real_escape_string($spgewicht) ."'
WHERE schaapId = '". mysql_real_escape_string($schaapId) ."'
" or die (mysql_error());
mysql_query($query_schaap_bijwerken);
?>
$query_schaap_bijwerken= "
UPDATE tblschapen
SET speendm ='". mysql_real_escape_string($spdatum) ."',
speenkg = '". mysql_real_escape_string($spgewicht) ."'
WHERE schaapId = '". mysql_real_escape_string($schaapId) ."'
" or die (mysql_error());
mysql_query($query_schaap_bijwerken);
?>
of met prepared statements.
_real_escape_string() functies zijn geen toverformule die alles ineens goed en veilig maakt.
Quotes rond numerieke velden zetten (in combinatie met _real_escape_string()) is imo de verkeerde manier om met numerieke velden om te gaan. Het "werkt" wel, in die zin dat je query wellicht syntactisch correct is en "veilig" is, maar als je voor een id-veld "aap" invult doet je query niets, en dat schiet het doel toch een beetje voorbij.
Als je wilt/verwacht dat een veld een numerieke waarde heeft controleer hier dan op, en als deze controle niets oplevert voer de query dan in zijn geheel niet uit maar klaag dat je niet-numerieke invoer niet het juiste formaat had. Vervolgens is het wellicht een beetje loos maar imo wel goed voor de bewustwording in die zin dat je met DATA te maken hebt, en niet met SQL, om deze waarde te escapen met _real_escape_string() (maar zoals gezegd, doet dat niets). Je hoeft er dan ook niet over na te denken of iets wel of niet geescaped hoeft te worden.
Escape alle DATA-delen gewoon altijd met _real_escape_string(), en filter je invoer als je numerieke waarden verwacht... Numerieke velden hebben dan geen quotes nodig.
Wanneer je wel quotes gebruikt levert dit soms ook onvoorspelbare resultaten op. Als je namelijk vergelijkingen doet tussen numerieke waarden, en je zet hier quotes omheen, dan geldt de alfabetische sortering, en niet de numerieke sortering:
Code (php)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
2
3
4
5
6
7
8
9
10
11
12
13
14
15
mysql> SELECT 213 > 35;
+----------+
| 213 > 35 |
+----------+
| 1 |
+----------+
1 row in set (0.00 sec)
mysql> SELECT '213' > '35';
+--------------+
| '213' > '35' |
+--------------+
| 0 |
+--------------+
1 row in set (0.00 sec)
+----------+
| 213 > 35 |
+----------+
| 1 |
+----------+
1 row in set (0.00 sec)
mysql> SELECT '213' > '35';
+--------------+
| '213' > '35' |
+--------------+
| 0 |
+--------------+
1 row in set (0.00 sec)
Als je deze vuistregels hanteert, dan zijn je queries veilig. En dit komt eigenlijk weer op de universele regel filter input, escape output neer.
En for the love of all that is good and holy, gebruik mysqli of PDO.
Tevens: voor de correcte werking van _real_escape_string() functies is het van cruciaal belang dat je (op de goede manier) de juiste character encoding selecteert. Dit doe je met een _set_charset() functie, bij voorkeur direct na het maken van een connectie met je database.
Daarnaast is het met normale "query" functies in MySQL niet mogelijk om meerdere queries tegelijkertijd uit te voeren met één functie-aanroep. Bij mijn weten levert dat te allen tijde een query fout op, maar het lijkt mij verstandig om dit ook even te testen.
Gewijzigd op 07/02/2015 00:26:20 door Thomas van den Heuvel
Een integer behandelen als een string is niet alleen onlogisch, maar ook nadelig voor de performance:
WHERE schaapId = '" . mysql_real_escape_string($schaapId) . "'"
De typeconversie van integers naar strings zit in de weg, vooral als je daarbij een JOIN krijgt op twee verschillende datatypen. Je kunt hier veel beter een cast naar een integer gebruiken:
WHERE schaapId = ' . (int)$schaapId
test.php?id=12
$_GET['id'] is ook een string... Alle data in $_GET en $_POST zijn strings.
Een typecast is ongewenst, omdat het een variabele probeert om te zetten naar een vorm die deze variabele mogelijk helemaal niet heeft.
test.php?id=aap
Jouw typecast zet $_GET['id'] om naar het cijfer 0.
In mijn aanpak moet je waarden waarbij je verwacht dat deze numeriek zijn controleren of deze ook daadwerkelijk numeriek zijn. Nog voordat je je query uberhaupt uitvoert. Een query die normaal resultaten geeft maar nooit iets oplevert is altijd een verspilling.
Een numerieke waarde hoeft niet per se van een numeriek type (bijvoorbeeld integer) te zijn. Daarnaast is deze eis nogal nutteloos als je deze pur sang integer vervolgens concateneert in een string... Zolang het format van de numerieke string maar klopt. En je hier vantevoren op controleert (input filtering).
Maar goed, dit werkt voor mij, is ondubbelzinnig, heeft heldere en eenvoudige regels, levert veilige queries op en respecteert in zekere zin de types van tabelkolommen (er worden immers geen quotes om getallen gezet). Het maakt mij niet zoveel uit wat je doet, zolang je maar begrijpt waarom.
Voor zover ik weet maken de types van variabelen pas echt uit als je gebruik maakt van prepared statements. Anders wordt je hele SQL-statement sowieso als string naar je database gestuurd.
Als je PHP gebruikt om SQL te formuleren, denk ik dat je het over types in SQL en MySQL moet hebben.
Je kunt in het Nederlands wel uitleggen hoe je iets in het Engels schrijft, maar niet zonder uit te gaan van de regels voor spelling en grammatica van het Engels natuurlijk.
Maar los daarvan: een JOIN op een miljoen schapen die links een nummer (integer) hebben en rechts een naam (string), is gewoon geen goed plan.
Natuurlijk moet je de regels van het spel (van zowel PHP als MySQL) in beschouwing nemen, dat is wat ik doe in mijn opzet, waarin ik probeer uit te leggen wat ik doe (en ook vooral waarom) om queries veilig te maken.
En als je aan het joinen bent op een tekstuele kolom kun je wel stellen dat er iets mis is met je database-ontwerp, maar het gaat hier over de beveiliging van queries, niet over de database-opzet.
Mijn voorbeeld met SELECT-statements (als je het daar over had) was bedoeld als illustratie om te laten zien wat er kan gebeuren als je overal maar klakkeloos quotes omzet als een soort van toverformule om dingen veilig te maken. Je query is dan mogelijk veilig(er) maar het kan ook ongewenste gevolgen hebben omdat de informatie op een andere manier geïnterpreteerd wordt dan de bedoeling was... Voor de duidelijkheid: ik ben er juist voor om quotes uit te bannen op plaatsen waar deze echt niet thuishoren.
Het gebruik van _real_escape_string() functies op die plekken is enkel bedoeld als een soort van bewustwording (en markering) van DATA-delen in je query, om het onderscheid tussen SQL en DATA nogmaals te onderstrepen en de denkstap die je anders elke keer moet maken (moet ik dit nu wel of niet escapen?) te elimineren.
De topicstarter vraagt naar SQL-injectie. SQL-injectie is mogelijk met een string, maar vrijwel onmogelijk met een integer.
Je hebt voor SQL-injectie immers een SQL-expressie nodig. Met enkel een integer kom je er niet.
Het advies om een integer te behandelen als een string is in meerdere opzichten geen goed advies. Als je integers nodig hebt maar ruimere strings accepteert, ondermijnt dat de beveiliging. En, zoals gezegd, lever je dan ook nog performance in.
Verder ben ik het wel met je eens natuurlijk: je moet verder kijken dan naar alleen de SQL-expressie met wat escapes.
Quote:
Met enkel een integer kom je er niet.
I see. Een string die een numeriek patroon heeft behandelen / gebruiken als getal is "not done", maar een niet-numerieke string typecasten naar een integer is a-ok. Okee dan :).
Quote:
Het advies om een integer te behandelen als een string is in meerdere opzichten geen goed advies.
Waar doe ik dit dan? Alles wat uit $_GET komt (of een ander superglobal array, for that matter) is al van het type string, zij het met een mogelijk numerieke waarde, waar je op moet controleren.
Quote:
Als je integers nodig hebt maar ruimere strings accepteert, ondermijnt dat de beveiliging.
Waar doe ik dit dan? Ik had al aangegeven dat er een controle *moet* plaatsvinden op de variabele als je een numerieke waarde verwacht. Deze controle doe je bijvoorbeeld met een regexp of filter_var(). Ik accepteer *precies* wat is toegestaan, en anders niets. Daarbij controleer ik het patroon, en niet zozeer het type. PHP is sowieso loosely typed dus ik snap niet dat er zo'n grote nadruk op het type moet worden gelegd als je een legitieme controle op het format doet.
Alleen als je PDO gebruikt maakt het type van je variabele echt iets uit.
Quote:
En, zoals gezegd, lever je dan ook nog performance in.
Het uitvoeren van een regexp / filter_var controle (input filtering) is nog altijd effficiënter dan het voor niets uitvoeren van een query die weliswaar syntactisch correct is, maar nooit een resultaat oplevert.
Thomas van den Heuvel op 07/02/2015 13:29:35:
$_GET['id'] is ook een string... Alle data in $_GET en $_POST zijn strings.
Ja, nou en?
Is dat een voldongen feit?
Of ben je programmeur?
Quote:
Is dat een voldongen feit?
Of ben je programmeur?
Of ben je programmeur?
Allebei. Het een sluit het ander niet uit.
Een typecast is gewoon de verkeerde oplossing voor het probleem, omdat deze mogelijk niet-toegestane invoer (?id=aap) omvormt tot iets wat wel toegestaan is ((int)$_GET['id'] == 0) maar wat vervolgens niets bruikbaars oplevert.
Het is gewoon een luie oplossing voor mensen die hun invoer niet willen controleren.
Het ging me ook om dat einde van de rit: er moet uiteindelijk een integer in die query, dus de cast (int) ligt daar meer voor de hand dan een functie voor een string-escape die een string oplevert.
Dat je daaraan voorafgaand méér controles uitvoert, spreekt voor zich. Upperbound en lowerbound om een bufferoverflow te voorkomen bijvoorbeeld.
Alleen een string escapen geeft schijnveiligheid. Zo heel oneens zijn we het helemaal niet, integendeel ;-)
Quote:
Het ging me ook om dat einde van de rit: er moet uiteindelijk een integer in die query, dus de cast (int) ligt daar meer voor de hand dan een functie voor een string-escape die een string oplevert.
Ik snap ook wel dat dat niets doet, maar dat heeft een hele andere reden: dat doe ik om die invoer als DATA-deel (externe input) te markeren in je SQL-statement. Dit om het onderscheid DATA-SQL gewoon glashelder te houden en niet zozeer om veiligheidsredenen, de invoer zou met controles al veilig moeten zijn in dat geval.
Ook al is dat in dat specifieke geval dus niet nodig (je hebt je invoer al gecontroleerd of desnoods een typecast gedaan :)), je hoeft dan in ieder geval de overweging niet eens meer te maken of dat onderdeel wel of niet ge-escaped moet worden, je behandelt *alle* DATA-delen gewoon op één uniforme manier. Hiermee elimineer je een denkstap en kun je het ook nooit fout doen (een keer niet doen terwijl het toch verstandiger bleek te zijn om wel te escapen :)).
Ik heb er nog even over na zitten denken: SQL injectie is volgens mij vaak juist mogelijk omdat invoer onvolledig wordt gecontroleerd (gebrek aan input filtering). En andere security issues in PHP-applicaties vinden haar oorsprong mogelijk in het onvolledig of niet ontdoen van de speciale betekenis die bepaalde data in een bepaalde context kan hebben (gebrek aan output escaping).
Hypothetische stelling: als je je invoer goed en volledig zou filteren, zouden functies als _real_escape_string() niet eens nodig zijn :). (Discuss.... :S)
De oplossing met gebruikmaking van een typecast stuurt weg van het oorspronkelijke probleem waarbij bepaalde data een bepaald format zou moeten hebben (en daarbij heb ik het dus nog niet eens over het type van die data).
Bij mijn aanpak pas ik "filter input, escape output" zo vaak en zo volledig mogelijk toe, waarbij ik een groot bewustzijn voor mijzelf creeer van alles wat INPUT en OUTPUT is. Hierbij kan ik veel meer security-zaken samenvatten en op een uniforme manier tacklen. Dit gaat dus om een integrale aanpak, en voert verder dan het veilig(er) maken van mijn queries.
Quote:
Alleen een string escapen geeft schijnveiligheid.
Dat alleen is soms niet genoeg inderdaad. Een mooi voorbeeld daarvan (van hoe het niet moet) is PDO - ik ken developers die denken dat omdat ze van (gesimuleerde) prepared statements gebruik maken, ze daarom geen invoer meer hoeven te controleren. Sterker nog, ze zijn zich daarvan niet eens bewust dat ze dat niet doen... Ze snappen gewoon geen biet van de achterliggende materie. Je bent wat mij betreft dan niet veilig en netjes aan het programmeren, maar gewoon wat apentrucs aan het uitvoeren.
EDIT: dit dus:
Gewijzigd op 07/02/2015 20:26:36 door Thomas van den Heuvel
Quote:
Hypothetische stelling: als je je invoer goed en volledig zou filteren, zouden functies als _real_escape_string() niet eens nodig zijn :). (Discuss.... :S)
Soms staat er heel legitiem een ' in een tekst.
Daar kan en mag je niet op filteren. Daar zul je dus altijd een escape op los moeten laten.
Het gaat daar dus niet eens om sql-injectie, maar gewoon om 'hoe voer ik een apostroph in'
Deze stelling was ook min of meer als "grap" bedoeld, maar nu gaat er iemand serieus op in :(.
Damn...
Gewijzigd op 08/02/2015 00:33:18 door Thomas van den Heuvel
Je code is veilig van query stacking zolang je de MySQL extensie blijft gebruiken. Als je de MySQLi extensie gebruikt (wat je hoort te doen als je jezelf serieus wilt nemen) ben je veilig zolang je mysqli_multi_query() (of de OO versie) niet gebruikt.
Een andere tabel updaten gaat je niet lukken met, je kan wel $spgewicht injecteren en zo andere velden aanpassen.
Als $schaapId ook te beinvloeden is door de gebruiker kan hij/zij zorgen dat ALLES aangepast wordt.
Voorbeelden van SQL-injection zoals die strip zijn niet heel handig in combinatie met MySQL en het PHP-platform.
Interessant en gaat specifiek over MySQL:
https://www.blackhat.com/presentations/bh-usa-09/DZULFAKAR/BHUSA09-Dzulfakar-MySQLExploit-PAPER.pdf
Hopelijk is het nadat je dat allemaal verteerd hebt duidelijk waarom je de tabel tbWeeg niet aangepast krijgt.