Ankündigung

Einklappen
Keine Ankündigung bisher.

SQL Injection PDO

Einklappen

Neue Werbung 2019

Einklappen
X
  • Filter
  • Zeit
  • Anzeigen
Alles löschen
neue Beiträge

  • SQL Injection PDO

    Hallo,

    seit Tagen bastle ich an einem einfachen script (zum einlernen), welches gegen SQL Injektion abgesichert sein soll. Hierbei habe ich mich stark eingelesen (unter anderen Links von hausl, Protestix und Hellbringer) und nun folgendes script mal erstellt und wollte Fragen, ob hier mal ein Profi ein Auge drauf werfen kann, ob das so gegen SQL Injection sicher ist oder nicht.

    Aufgabe ist es einfach, 3 Variablen, welche mit GET übergeben werden, in eine SQL Query zu packen welche dann per PDO prepared statments die Datenbank abfrägt.(1 x where mit genauen Bedingungen, 1 x like und einmal order). Das Script funktioniert gem. Teststellung, es geht ausschliesslich nur um die SQL injection, nicht um den Sinn der Abfrage

    Hier mal das script in der Hoffnung, dass es sicher ist.

    Aufruf wäre zum Beispiel seite.php?vorname=Björn&name=K&orderby=email

    PHP-Code:
    if (isset($_GET['vorname']))    $vorname=$_GET['vorname'];    else    $vorname='';
    if (isset(
    $_GET['name']))         $name=$_GET['name'];        else    $name='';

    try { 
    $conn=new PDO('mysql:host=localhost;dbname=db;charset=utf8',usr,passw,array(PDO::ATTR_ERRMODE => PDO::ERRMODE_WARNING)); }
        catch (
    PDOException $e) {  echo 'Verbindung fehlgeschlagen: ' $e->getMessage();
        exit;
    }

    if (
    $vorname!='' and $name!='')
    {
        
    $sql="select name, email from tabelle where vorname = :vorname and name like :name";

        
    $ordernames=array('name','email');    // definieren, welche Spalten nur für order gültig sind
        
    if (in_array($_GET['orderby'], $ordernames))
        
    $sql .= sprintf(" ORDER BY `%s` ASC"$_GET['orderby']);

        
    $statm$conn->prepare($sql);
        
    $statm->bindParam(':vorname',$vorname);
        
    $statm->bindValue(':name',"%".$name."%");

        
    $statm->execute();

        foreach (
    $statm as $row)
        {
            echo 
    htmlspecialchars($row['name'])." ".htmlspecialchars($row['email'])."<br />";
        }
    }
    $conn=null
    Gruß
    Falke07

  • #2
    PHP-Code:
     $sql .= sprintf(" ORDER BY `%s` ASC"$_GET['orderby']); 
    Da nimmst du wieder einen Wert von aussen in die Query -> SQL-Injection-Lücke.
    The string "()()" is not palindrom but the String "())(" is.

    Debugging: Finde DEINE Fehler selbst! | Gegen Probleme beim E-Mail-Versand | Sicheres Passwort-Hashing | Includes niemals ohne __DIR__
    PHP.de Wissenssammlung | Kein Support per PN

    Kommentar


    • #3
      Oh man, MIst.
      Und noch schlimmer: Ich bekomme orderby nicht in das Statement eingebunden. Mit folgendem Script, missachtet er einfach die Orderfunktion, wie wenn Sie nicht da wäre (auch ohne Fehlermeldung). Was mach ich da falsch? bei :name funktioniert es auch.

      PHP-Code:

          $sql
      ="select name, email from $t02 where vorname = :vorname and name like :name";

          
      $ordernames=array('name','email');    // definieren, welche Spalten nur für order gültig sind
          
      if (in_array($_GET['orderby'], $ordernames))
          
      $sql .=" ORDER BY :orderby";
          
      //$sql .= sprintf(" ORDER BY `%s` ASC", $_GET['orderby']);

          
      $statm$conn->prepare($sql);
          
      $statm->bindParam(':vorname',$vorname);
          
      $statm->bindValue(':name',"%".$name."%");
          
      $statm->bindValue(':orderby',$_GET['orderby']." ASC"); 

      Kommentar


      • #4
        Mit Prepared Statments kannst du nur Werte binden. Was ich mich jetzt aber frage, hast du das geschreiben? Das erste Beispiel ist OK. Die Zeile über dem ORDER BY, stellt sicher, dass eine gültiger Wert in den Query eingefügt wird.



        Kommentar


        • #5
          Zitat von erc Beitrag anzeigen
          Mit Prepared Statments kannst du nur Werte binden.
          Wie meinst du das? Du kannst auch per Referenz zum execute() Zeitpunkt binden https://stackoverflow.com/questions/...13428#14413428

          Oder ich hab dich falsch verstanden.
          The string "()()" is not palindrom but the String "())(" is.

          Debugging: Finde DEINE Fehler selbst! | Gegen Probleme beim E-Mail-Versand | Sicheres Passwort-Hashing | Includes niemals ohne __DIR__
          PHP.de Wissenssammlung | Kein Support per PN

          Kommentar


          • #6
            Zitat von erc Beitrag anzeigen
            Mit Prepared Statments kannst du nur Werte binden.
            und keine Feld- und Tabellennamen. Siehe auch MySQL PDO: Validieren und Maskieren von Feld- und Tabellennamen.

            Kommentar


            • #7
              Hallo,

              es ist das Ergebnis meiner tagelangen Leserei und durchkämpfen von unzähligen Seiten/Foren etc.

              Da die $sql- Erweiterung um
              PHP-Code:
               $statm->bindValue(':orderby',$_GET['orderby']." ASC"); 
              nun ja kein Wert ist, wird das der Fehler sein. Aber ich finde keine Lösung, den Zusatz: order by $_GET['orderby'] gegen SQL injection zu schützen

              Auch DAS funktioniert nicht: ($_GET['orderby'] beinhaltet den Spaltennamen, nach welchem sortiert werden soll)

              PHP-Code:
                  $sql="select name, email from $t02 where vorname = :vorname and name like :name";

                  
              $ordernames=array('name','email');    // definieren, welche Spalten nur für order gültig sind
                  
              if (in_array($_GET['orderby'], $ordernames))
                  
              $sql .=" ORDER BY :orderby ASC";
                  
              //$sql .= sprintf(" ORDER BY `%s` ASC", $_GET['orderby']);

                  
              $statm$conn->prepare($sql);
                  
              $statm->bindParam(':vorname',$vorname);
                  
              $statm->bindValue(':name',"%".$name."%");
                  
              $statm->bindParam(':orderby',$_GET['orderby']);

                  
              $statm->execute(); 
              . Ich verzweifle bald.

              Kommentar


              • #8
                Zitat von hausl Beitrag anzeigen
                Wie meinst du das?
                Ich meine aus SQL Sicht.

                Zitat von Falke07 Beitrag anzeigen
                Auch DAS funktioniert nicht: ($_GET['orderby'] beinhaltet den Spaltennamen, nach welchem sortiert werden soll)
                Ja, genau. Der Spaltenname ist aber kein Wert, sondern ebend ein Spaltenname (Identifier). Du kannst dir das auch so Vorstellen, als würdest du in den Query ORDER BY "name" und nicht ORDER BY name schreiben. Ersters führt zu gar keiner Sortierung, (stell dir vor alle Datensätze bekommen zusätzliche eine Spalte mit den Wert "name" dran und danach wird sortiert), zweiteres sortiert nach dem Inhalt der Spalte name.

                PS: falls es untergegangen ist, das erste Beispiel von dir ist OK.

                Kommentar


                • #9
                  Eine Möglichkeit wäre switch

                  PHP-Code:
                  switch ($_GET['orderby']) {
                      case 
                  "email":
                          
                  $sql .= " ORDER BY email ASC";
                          break;
                      case 
                  "name":
                          
                  $sql .= " ORDER BY name ASC";
                          break;
                      case 
                  "vorname":
                          
                  $sql .= " ORDER BY vorname ASC";
                          break;

                  Kommentar


                  • #10
                    Es kommen ja nur bestimmte Feldnamen für eine Sortierung in Frage. Für diese kannst du eine sog. Whitelist erstellen. Eine freie Eingabe der Namen ist wohl alles andere als eine gute Lösung.

                    Kommentar


                    • #11
                      OK,
                      wieder einen Schritt weiter. Tabellenname vs Wert. Aber wenn ich mir die Links durchlese stellt sich für mich folgende Frage:

                      Durch:
                      PHP-Code:
                         // Änderung: Leertasten entfernt, um Befehlskonstrukte einer SQL injektion zu stören. 
                         
                      $orderby=str_replace(" ","",$_GET['orderby']);
                          
                      $ordernames=array('name','email'); 
                          if (
                      in_array($orderby$ordernames))
                          
                      $sql .=" ORDER BY ".$orderby." ASC"
                      prüfe ich doch, ob $_GET['orderby'] die Namen enthält, welche ich über $ordernames zulasse. Sollte hier die if Anweisung keinen Treffer finden, wird $sql auch nicht um die Orderfunktion erweitert. Somit sollte doch der Versuch einer SQL injection über $_GET['orderby'] abgefangen werden oder nicht.

                      Ist doch wie in dem Beispiel mit switch. Wenn keine Bedingung erfüllt wird, wir auch nicht um order erweitert.

                      Kommentar


                      • #12
                        Du solltest bei in_array() auch noch den dritten Parameter verwenden, ansonsten gäbe es wieder Möglichkeiten so eine Whitelist zu umgehen.

                        Kommentar


                        • #13
                          OK,

                          dritter Parameter ist drin. THx.

                          Abschlußfrage (ursprüngliche Frage)

                          Kann ich somit davon ausgehen, dass das Script gegen SQL injection sicher ist?

                          Hier nochmal alles:
                          PHP-Code:
                          if (isset($_GET['vorname']))    $vorname=$_GET['vorname'];    else    $vorname='';
                          if (isset(
                          $_GET['name']))        $name=$_GET['name'];        else    $name='';

                          try { 
                          $conn=new PDO('mysql:host=localhost;dbname=db;charset=utf8',usr,passw,array(PDO::ATTR_ERRMODE => PDO::ERRMODE_WARNING)); }
                              catch (
                          PDOException $e) {  echo 'Verbindung fehlgeschlagen: ' $e->getMessage();
                              exit;
                          }

                          if (
                          $vorname!='' and $name!='')
                          {
                              
                          $sql="select name, email from tabelle where vorname = :vorname and name like :name";

                              
                          $orderby=str_replace(" ","",$_GET['orderby']);
                              
                          $ordernames=array('name','email');
                              if (
                          in_array($orderby$ordernames,true))
                              
                          $sql .=" ORDER BY ".$orderby." ASC";

                              
                          $statm$conn->prepare($sql);
                              
                          $statm->bindParam(':vorname',$vorname);
                              
                          $statm->bindValue(':name',"%".$name."%");

                              
                          $statm->execute();

                              foreach (
                          $statm as $row)
                              {
                                  echo 
                          htmlspecialchars($row['name'])." ".htmlspecialchars($row['email'])."<br />";
                              }
                          }
                          $conn=null
                          Gruß
                          Falke07

                          Kommentar


                          • #14
                            Würd sagen das schaut dahingehend soweit gut aus.

                            Aber der Code ist insgesamt noch ziemlich häßlich/unsauber. Schau mal das du Formatierungen einhältst, Standards (PSR) für Klammerung, Leerzeichen vor und nach dem = usw. Schau mal da, PSR1 und PSR2, solltest du dir angewöhnen. http://www.php-fig.org/psr/

                            Weiters hast du die DB Verbindung noch nicht zentral? Die wirst du doch in jedem Script brauchen, oder? Also, auslagern und zentralisieren. Eine bootstrap.php eigentlich sich für so etwas.

                            Dann .. warum einmal bindValue() und einmal bindParam()? Wozu? Erkläre mal

                            Dann: statt sowas:
                            PHP-Code:
                            if (isset($_GET['vorname']))    $vorname=$_GET['vorname'];    else    $vorname=''
                            Kannst du zB auch einfach machen:
                            PHP-Code:
                            $vorname trimfilter_input(INPUT_GET'vorname') ); 
                            Das trim() gibt einen String zurück, dh zu dem üblichen, zumeist gewünschten trimmen, wird auch im Fall des nicht vorhanden seins aus dem NULL von filter_input() ein Leerstring "" auf den du dann sogar mit === oder !== typsicher prüfen könntest.

                            Das $conn = null am Ende ist auch nicht nötig.

                            Und so weiter..
                            The string "()()" is not palindrom but the String "())(" is.

                            Debugging: Finde DEINE Fehler selbst! | Gegen Probleme beim E-Mail-Versand | Sicheres Passwort-Hashing | Includes niemals ohne __DIR__
                            PHP.de Wissenssammlung | Kein Support per PN

                            Kommentar


                            • #15
                              Hallo hausl,

                              super, das es mal sicherheitsbezogen gut aussieht.

                              Das mit den Code check ich mal ab und gelobe Besserung.

                              bindParam vs bindValue
                              ich habe hier bindValue für name verwendet, da die Variable für die like Funktion mit dem % Zeichen erweitert wird entgegen vorname. Hmmm, wenn Du so nachfrägst, less ich mir das lieber nochmal durch.

                              trim() kannte ich noch nicht. Hört sich aber gut an, vorallem wegen === , check ich auch mal ab.

                              $conn=null; um die Verdindung zu schliessen. Muss man das nichtmehr? Wollte sauber programmieren ,-(

                              Das waren klasse Tips von Dir. So kann ich mich in der Praxis weiterbilden. Das ist genau das, was ich brauche. Jemanden, der mir einfach sagt: nimm das anstatt das und mache das so anstatt so. Ist halt nicht so einfach mit doi it self, auch wenn es sehr viele Tutorial, Hilfsseiten, Handbücher etc. gibt. Die Praxis sieht immer etwas anders aus und da sind solche Tips Gold wert.. Vielen Dank hierfür

                              Gruß
                              Falke07

                              Kommentar

                              Lädt...
                              X