[PHP] SQLi simulator - Softwareontwikkeling

zondag 25 oktober 2015 13:26

Acties:

0 Henk 'm!

Topicstarter

Hey

Ik ben een boek aan het lezen over secure coding in PHP. En ik wil even uittesten hoe effectief mysql_real_escape_string is tegen SQLi. Daarvoor wil ik even een klein 'testlabje' maken.

Deze code zou een SQL foutmelding moeten weergeven, maar dat doet het niet.

code:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title></title>
    </head>
    <body>        
        <?php
            if (isset($_GET["btnSend"])) {
                $conn = new mysqli("localhost", "root", "");
                
                if ($conn->connect_error) {
                    echo "<br>[Message] Error occured while connecting.";
                }
                $string = $_GET["txtTest"];
                
                $query = "USE testing";
                $conn->query($query);
                
                $query = "SELECT * FROM table1 WHERE ID=$string";
                $result = $conn->query($query);
                if ($result->num_rows > 0) {
                    while ($row = $result->fetch_assoc()) {
                        echo "<br>ID: ".$row["ID"]."<br>texty: ".$row["texty"];
                    }
                }
            }
        ?>
        
        <form method="get">
            <input type="text" name="txtTest" />
            <input type="submit" name="btnSend" value="Submit" />
        </form>
    </body>
</html>

Het geeft alleen de error:
Notice: Trying to get property of non-object in C:\xampp\htdocs\PhpProject2\index.php on line 43
Als ik een '-icoontje invoeg, geen SQL fout denk ik.

Doet het dit omdat ik enkel weergeef als er meer als 0 resultaten/rijen zijn?

Maar, hoe kan ik nu een code schrijven die wel degelijk SQLi-kwetsbaar is?
Hier filter ik niks ofzo, dus zou het dat eigenlijk moeten zijn.

Bedankt!

zondag 25 oktober 2015 13:32

Acties:

0 Henk 'm!

johnkeates

Als je een fout dat op regel 20 dan krijg je die sowieso wel te zien, daar doet je if statement wat verder op niks aan. Ga eens in je php configuratie kijken wat de error reporting instellingen zijn, dan weten we meer.

zondag 25 oktober 2015 13:44

Acties:

0 Henk 'm!

CH4OS

It's a kind of magic

De fout gaat over regel 43, je code hier komt maar tot 35... Het stuk code wat dus werkelijk fout gaat, laat je niet zien, hoe moeten wij dan zeggen wat er fout gaat?

zondag 25 oktober 2015 16:50

Acties:

0 Henk 'm!

Antrax

ONiel schreef op zondag 25 oktober 2015 @ 13:26:
Deze code zou een SQL foutmelding moeten weergeven, maar dat doet het niet.
code:
1
2
$string = $_GET["txtTest"];
$query = "SELECT * FROM table1 WHERE ID=$string";
Het geeft alleen de error:
Notice: Trying to get property of non-object in C:\xampp\htdocs\PhpProject2\index.php on line 43

Zoals CptChaos al zegt staat niet heel je code in het draatje. Wat ik je wel kan adviseren is om $query aan te passen gezien deze niet echt goed gebruikt wordt.

bv;

code:

1	$query = "SELECT * FROM table1 WHERE ID=" . $string . "";

.Gertjan.: Ik ben een zelfstandige alcoholist, dus ik bepaal zelf wel wanneer ik aan het bier ga!

zondag 25 oktober 2015 16:57

Acties:

0 Henk 'm!

ONiel

Topicstarter

Sorry ik heb de error gekopieerd van de verkeerde pagina.

code:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title></title>
    </head>
    <body>        
        <?php
            if (isset($_GET["btnSend"])) {
                $conn = new mysqli("localhost", "root", "");
                
                if ($conn->connect_error) {
                    echo "<br>[Message] Error occured while connecting.";
                }
                $string = $_GET["txtTest"];
                
                $query = "USE testing";
                $conn->query($query);
                
                $query = "SELECT * FROM table1 WHERE ID=".$string."";
                $result = $conn->query($query);
                if ($result->num_rows > 0) {
                    while ($row = $result->fetch_assoc()) {
                        echo "<br>ID: ".$row["ID"]."<br>texty: ".$row["texty"];
                    }
                }
            }
        ?>
        
        <form method="get">
            <input type="text" name="txtTest" />
            <input type="submit" name="btnSend" value="Submit" />
        </form>
    </body>
</html>

Error:
Notice: Trying to get property of non-object in C:\xampp\htdocs\PhpProject2\sqlitest.php on line 22

zondag 25 oktober 2015 17:00

Acties:

0 Henk 'm!

ByteMe_

Debuggen, zet onder die $query bijv :

code:

1	echo $query;

En wat is de output?

[ Voor 14% gewijzigd door ByteMe_ op 25-10-2015 17:02 ]

Mijn laatste reviews: Xiaomi Mi 9T | Mpow H12

zondag 25 oktober 2015 17:43

Acties:

0 Henk 'm!

ONiel

Topicstarter

RonaldDesigns schreef op zondag 25 oktober 2015 @ 17:00:
Debuggen, zet onder die $query bijv :
code:
1
echo $query;
En wat is de output?

De echo van $query: SELECT * FROM table1 WHERE ID='
Even ter verduidelijking: Mijn script werkt perfect wanneer de input een integer is, alleen niet als het iets anders is. Dus, hoe maak ik een simulatie van PHP waar ' wel gewoon een SQL error geeft?

zondag 25 oktober 2015 17:46

Acties:

0 Henk 'm!

CH4OS

It's a kind of magic

iRicardo schreef op zondag 25 oktober 2015 @ 16:50:
[...]

Zoals CptChaos al zegt staat niet heel je code in het draatje. Wat ik je wel kan adviseren is om $query aan te passen gezien deze niet echt goed gebruikt wordt.

bv;
code:
1
$query = "SELECT * FROM table1 WHERE ID = ' " . $string . " ' ";

Heb de query even aangepast, als het een string is...

zondag 25 oktober 2015 17:49

Acties:

0 Henk 'm!

Antrax

CptChaos schreef op zondag 25 oktober 2015 @ 17:46:
[...]
Heb de query even aangepast, als het een string is...

oh ja zondag he

.Gertjan.: Ik ben een zelfstandige alcoholist, dus ik bepaal zelf wel wanneer ik aan het bier ga!

zondag 25 oktober 2015 17:50

Acties:

0 Henk 'm!

CH4OS

It's a kind of magic

iRicardo schreef op zondag 25 oktober 2015 @ 17:49:
[...]

oh ja zondag he

Kan de beste overkomen.

zondag 25 oktober 2015 17:51

Acties:

0 Henk 'm!

NMe

Quia Ego Sic Dico.

Als je een boek over "secure coding" aan het lezen bent, wat ben je dan überhaupt aan het doen met mysql_real_escape_string? Alle mysql_*-functies zijn allang deprecated en worden in PHP7 zelfs helemaal niet meer ondersteund. Waarom zou je onderzoek doen naar een dooie techniek? Steek die tijd liever in het leren over parameterized queries en/of PDO.

'E's fighting in there!' he stuttered, grabbing the captain's arm.
'All by himself?' said the captain.
'No, with everyone!' shouted Nobby, hopping from one foot to the other.

zondag 25 oktober 2015 17:56

Acties:

+1 Henk 'm!

phex

Ik weet niet welk boek je leest maar stop met wat je nu aan het doen bent en lees dit:

http://php.net/manual/en/pdo.prepared-statements.php

Vervolgens check of dat in je boek voorkomt en anders mag ie direct de shredder in.

zondag 25 oktober 2015 17:57

Acties:

0 Henk 'm!

ONiel

Topicstarter

NMe schreef op zondag 25 oktober 2015 @ 17:51:
Als je een boek over "secure coding" aan het lezen bent, wat ben je dan überhaupt aan het doen met mysql_real_escape_string? Alle mysql_*-functies zijn allang deprecated en worden in PHP7 zelfs helemaal niet meer ondersteund. Waarom zou je onderzoek doen naar een dooie techniek? Steek die tijd liever in het leren over parameterized queries en/of PDO.

Ik gebruik mysqli_real_escape_string. Volgens mij wordt dat nog steeds veel gebruikt.

zondag 25 oktober 2015 18:00

Acties:

0 Henk 'm!

phex

Het probleem met mysqli_real_escape_string is dat je dan zelf inputs in een query aan het lijmen bent. Een prepared statement heeft enkel placeholders voor de inputs, en bij het assignen geef je aan wat voor type input het moet zijn.

Daar wordt je code leesbaarder en minder foutgevoellig van.

Dat het nog veel gebruikt wordt zegt niks over de kwaliteit of veiligheid.

zondag 25 oktober 2015 18:02

Acties:

0 Henk 'm!

NMe

Quia Ego Sic Dico.

ONiel schreef op zondag 25 oktober 2015 @ 17:57:
[...]

Ik gebruik mysqli_real_escape_string. Volgens mij wordt dat nog steeds veel gebruikt.

Alleen door mensen die niet weten wat ze doen. Prepared statements/parameterized queries zijn veel minder foutgevoelig.

'E's fighting in there!' he stuttered, grabbing the captain's arm.
'All by himself?' said the captain.
'No, with everyone!' shouted Nobby, hopping from one foot to the other.

maandag 26 oktober 2015 14:20

Acties:

0 Henk 'm!

ONiel

Topicstarter

Oké, dan zal ik daar meer over lezen. (Het komt voor in het boek dat ik lees).
Bedankt!

maandag 26 oktober 2015 14:53

Acties:

0 Henk 'm!

Janoz

Moderator Devschuur®

!litemod

Jij verwacht een fout op regel 21. Die fout is er wel, maar je doet er niks mee. Doordat de fout opgetreden is wordt $result gelijk aan false, en niet een result object. Wanneer je vervolgens op de volgende regel hier het aantal rijen op wilt vragen krijg je inderdaad de foutmelding "Notice: Trying to get property of non-object". 'false' is immers een boolean, en geen object.

Ken Thompson's famous line from V6 UNIX is equaly applicable to this post:
'You are not expected to understand this'

maandag 26 oktober 2015 14:57

Acties:

0 Henk 'm!

Elijan9

phex schreef op zondag 25 oktober 2015 @ 18:00:
Het probleem met mysqli_real_escape_string is dat je dan zelf inputs in een query aan het lijmen bent. Een prepared statement heeft enkel placeholders voor de inputs, en bij het assignen geef je aan wat voor type input het moet zijn.

Als je SQL injectie wilt simuleren zoals de TS en wilt observeren wat je met "mysqli_real_escape_string" hiertegen kan doen, dan is het wel zo logisch om vooral geen prepared statements te gebruiken

Geen idee welk boek het is, maar ongetwijfeld volgt er ergens de conclusie dat prepared statements uiteindelijk het veiligst zijn

(Overigens kosten prepared statements wel enige performance, met name wanneer een query volledig dynamisch opgebouwd moet worden en niet herhaaldelijk uitgevoerd wordt. Persoonlijk zou ik dat voor lief nemen.)

War is when the young and stupid are tricked by the old and bitter into killing each other. - Niko Bellic

dinsdag 27 oktober 2015 22:13

Acties:

0 Henk 'm!

ONiel

Topicstarter

@Elijan: Ik ben Architect's Guide to PHP Security aan het lezen. En veel pagina's van PHP.net en OWASP.

Even een vraag,

code:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title></title>
    </head>
    <body>        
        <?php
            mysqli_report(MYSQLI_REPORT_ALL);
            
            if (isset($_GET["btnSend"])) {
                $host = "127.0.0.1";
                $user = "root";
                
                $conn = new mysqli($host, $user, "");
                
                if ($conn->connect_error) {
                    echo "<br>Connection error.";
                }
                
                $conn->query("USE testing");
                $stmt = $conn->prepare("INSERT INTO table1 (texty) VALUES (?)");
                if (!$stmt) {
                    echo "<br>Unknown error.";
                }
                
                $stmt->bind_param('s', $string);
                
                $string = $_GET["txtTest"];
                $stmt->execute();
            }
        ?>
        
        <form method="get">
            <input type="text" name="txtTest" />
            <input type="submit" name="btnSend" value="Submit" />
        </form>
    </body>
</html>

zijn prepared statemants hier goed gebruikt? Is mijn code nu SQLi-vrij? Kan ik nog iets doen, om het nóg veiliger te maken? En, als ik een query wil doen, die geen enkele user-input verwerkt (zoals: "use database_name"), moet ik dan ook een prepared statement gebruiken?

Bedankt!

dinsdag 27 oktober 2015 22:26

Acties:

0 Henk 'm!

krvabo

MATERIALISE!

NMe schreef op zondag 25 oktober 2015 @ 18:02:
[...]

Alleen door mensen die niet weten wat ze doen. Prepared statements/parameterized queries zijn veel minder foutgevoelig.

offtopic:
Bullshit. Mysqli_real_escape_string wordt alleen gebruikt door mensen die niet weten wat ze doen?

Dat prepared statements minder foutgevoelig zijn voor onoplettende devvers wil ik je wel geven, want als je iets fout doet dan krijg je dat meteen te horen. Als je een competente programmeur bent die gewoon oplet wat ie doet dan is mysqli_real_escape_string net zo veilig. Het probleem is juist dat er mensen bijzitten die niet consistent zijn, of achteraf hun database gaan zitten wijzigen. Dat zijn dingen die niet horen, en daardoor gaat het fout. Dat ligt niet aan de methode van werken, dat ligt aan de programmeur.

Zolang je query-beveiliging op een consistent moment doet en gewoon jezelf aanleert consequent te zijn dan is het meer dan prima om mysqli_real_escape_string te gebruiken. Je persoonlijke voorkeur zegt niets over de veiligheid.

Pong is probably the best designed shooter in the world.
It's the only one that is made so that if you camp, you die.

dinsdag 27 oktober 2015 22:52

Acties:

0 Henk 'm!

NNF

Volgens mij beweert NMe ook niet dat het niet veilig is, alleen dat het minder foutgevoelig is, en daar valt weinig tegenin te brengen. Waarom zou je in hemelsnaam nog handmatig je input gaan zitten escapen als er een methode beschikbaar is die gewoon beter is?

woensdag 28 oktober 2015 01:46

Acties:

0 Henk 'm!

NMe

Quia Ego Sic Dico.

Sorry, maar als je bewust handmatig gaat lopen escapen (met dus het volste risico dat je het een keer per ongeluk vergeet) dan ben je gewoon dom bezig. Dat heeft niks met persoonlijke voorkeur te maken maar met het afdichten van risico's. Als je onder het mom van "persoonlijke voorkeur" dat soort risico's bewust neemt dan is dat een vorm van incompetentie of misplaatste trots ("mij gebeurt dat niet!!!1").

'E's fighting in there!' he stuttered, grabbing the captain's arm.
'All by himself?' said the captain.
'No, with everyone!' shouted Nobby, hopping from one foot to the other.

woensdag 28 oktober 2015 03:32

Acties:

0 Henk 'm!

krvabo

MATERIALISE!

Nouja, nofi, maar inderdaad, dat gebeurt me dus niet (en het is ook niet alsof ik het pas een paar jaar doe).

Je argument is dat het onveilig is als je het vergeet. Ja, raar. Als je samenwerkt met mensen die het normaal niet gebruiken dan is het inderdaad waarschijnlijk niet de beste methode. Als je zelf aan dat project werkt, of met mensen samenwerkt die dit ook gewoon zijn, en je al jaren lang op die manier werkt dan is er gewoon letterlijk niets mis mee. Als je vergeet een deur op slot te draaien dan maakt dat ook je slot geen onveilige methode om een deur dicht te houden.

Als je net gaat leren over veiligheid? Sure, gebruik prepared statements, dan zit je goed. Er is geen groot argument tegen prepared statements (iets meer code, miniscuul beetje trager, maar uitstekende methode). Zeggen dat iemand incompetent is omdat ze mysqli_real_escape_string gebruiken? Dan heb je gewoon echt geen gelijk.

Pong is probably the best designed shooter in the world.
It's the only one that is made so that if you camp, you die.

woensdag 28 oktober 2015 12:09

Acties:

0 Henk 'm!

NMe

Quia Ego Sic Dico.

Denk wat je wil denken, ik hou het op mijn "misplaatste trots"-argument. Dat het je nog niet is gebeurd wil niet zeggen dat het je niet kan gebeuren, niemand is perfect. Je laat jezelf wagenwijd open voor gaten in je beveiliging terwijl je die gaten geheel automatisch voorkomt met prepared statements. Je hebt het steeds over argumenten tégen het een of het ander, maar er is gewoon geen argument vóór zelf escapen.

Je vergelijking met een slot gaat overigens ook helemaal niet op. Ook een voordeur die niet op slot zit is niet zomaar open te breken, zeker niet als je ook nog dievenklauwen hebt. Maar het belangrijkste verschil is dat je maar een of twee deuren hebt die toegang geven tot je huis, terwijl je door zélf te gaan escapen een potentiële deur maakt van elke parameter. Da's een beetje hetzelfde als elke ochtend als je je huis verlaat duizend verschillende deuren moeten afsluiten. Daar kies je toch niet voor?

ONiel schreef op dinsdag 27 oktober 2015 @ 22:13:
zijn prepared statemants hier goed gebruikt?

Ja. Maar je kent eerst een niet-bestaande variabele toe aan je statement (regel 27) en kent daarna pas een waarde toe aan die variabele (29). Tenzij een functie met references werkt gaat dit niet werken en zelfs als dat wel zo is levert dit onleesbare code op. Wissel regels 27 en 29 dus even om.

Is mijn code nu SQLi-vrij?

Dit stukje code in elk geval wel.

Kan ik nog iets doen, om het nóg veiliger te maken?

In dit simpele stukje code? Niet echt.

En, als ik een query wil doen, die geen enkele user-input verwerkt (zoals: "use database_name"), moet ik dan ook een prepared statement gebruiken?

Moet niet, maar ik zou het wel doen. Mocht je query ooit veranderen waardoor je wel parameters nodig hebt dan hoef je niet alles om te gooien omdat je voor "gewone" query's een andere structuur gebruikt.

USE databasenaam is trouwens een raar voorbeeld, want die query zou ik überhaupt niet gebruiken. Daar heb je namelijk een functie voor.

[ Voor 39% gewijzigd door NMe op 28-10-2015 12:17 ]

'E's fighting in there!' he stuttered, grabbing the captain's arm.
'All by himself?' said the captain.
'No, with everyone!' shouted Nobby, hopping from one foot to the other.

woensdag 28 oktober 2015 12:17

Acties:

0 Henk 'm!

Avalaxy

@krvabo: Ik snap niet waarom je prepared statements trager noemt. Ze zijn juist veel sneller. Het probleem met string concattenation is dat je database geen parameters ziet en dus elke variant van je query als een nieuwe query beschouwt. Dit betekent dat er voor elke query een nieuw execution plan gemaakt moet worden om te kijken hoe deze het efficiëntst uitgevoerd kan worden. Elke keer weer opnieuw. Als je gewoon parameters gebruikt zoals bij prepared statements dan kun je het execution plan uit de cache trekken en direct uitvoeren, dat is veel sneller en waar een statement cache voor bedoeld is.

woensdag 28 oktober 2015 13:50

Acties:

0 Henk 'm!

krvabo

MATERIALISE!

Het is 'trager' (nauwelijks merkbaar overigens) omdat er een extra roundtrip gemaakt moet worden naar de DB-server.

Je hebt gelijk mbt query cache, maar het is afhankelijk van je omgeving en scope of die cache dan wel bestaat. Als je een klein project met weinig hits hebt op een shared server dan bestaat die cache waarschijnlijk niet (omdat er veel meer queries worden gebruikt dan de jouwe. Heb je een project waar elke paar minuten dezelfde query wordt uitgevoerd (of meerdere keren per execution) dan zul je er voordeel van hebben. (Nogmaals, ik heb niets tegen prepared statements, ik raad het ook altijd aan als beste optie, en gebruik het ook waar ik er voordeel van kan hebben)

NMe: Oneens, maar denk wat je wil denken. Ik persoonlijk vind het beter leesbaar met string concat, en daar zal niet iedereen / weinig mensen het mee eens zijn. Het maakt me echt niet uit.
Ik had het verder ook niet over een voordeur, maar 'een deur', met een klink aan iedere kant. Een deur die je bij de installatie op slot draait, en niet 'iedere ochtend'.

Prepared statements zijn over het algemeen de betere keuze, ja. Ik heb geen argumenten _tegen_ het gebruik van prepared statements, of _voor_ het gebruik van real_escape_string. Ik ageer tegen je stelling dat 'mensen die real_escape_string gebruiken incompetent zijn'. Dat is rücksichtslos en pertinent onjuist.

Pong is probably the best designed shooter in the world.
It's the only one that is made so that if you camp, you die.

woensdag 28 oktober 2015 14:08

Acties:

0 Henk 'm!

Voutloos

Er zijn dus betere technieken, waarbij security voor wat betreft query parameters een gegeven is. Als je die niet gebruikt, met als argument dat je altijd op een bepaalde manier hebt gewerkt negeer je dus nieuwere / superieure tools.

Dat zou ik op een slechte dag ook wel als incompetent kunnen benoemen. Als je het per se wil wil ik het wel afzwakken naar 'handmatig werk dat niet meer van deze tijd is'.

{signature}

woensdag 28 oktober 2015 14:17

Acties:

0 Henk 'm!

johnkeates

Het valt me op dat er nog niemand geopperd heeft dat zelf rauwe queries duwen misschien ook niet perse de beste oplossing is, ongeacht je schoonmaakpraktijken met escaping of prepared statements, en dat je met een licht laagje abstractie een stuk verder komt. Sure, als je complexe queries wil schrijven om dat dat optimaal gebruik maakt van je DB moet je dat vooral doen, maar waarom een simpel DAO-idee, table data gateway pattern of een row data gateway pattern niet wat praktischer is t.o.v. zelf hard koppelen met mysql(i) in PHP lees ik zo nog niet...

SQL Injections krijg je volgens mij tegenwoordig vooral in slecht geprogrammeerde applicaties, waarbij ik er van uit ga dat je zodra je een interessant doelwil bent je waarschijnlijk ook genoeg data zit te verwerken om daadwerkelijk iets te hebben wat anderen willen stelen. Op dat punt ga je toch niet meer zelf direct tegen je database programmeren of met procedural PHP aan de slag? Of je moet een simpel gastenboek hebben... maar dan is er weer niks interessants te halen