[PHP/MySQL] Vermeidung von SQL-Injections

theHacker · 1 Juni 2006

Ich poste hier mal meine Funktion (hier vereinfacht, da ich eigentlich eine Klasse benutze), die ich immer für Datenbank-Abfragen verwende. Bei dieser Funktion wird jeder Parameter, der an die Datenbank übergeben wird, vorher escaped, d.h. es sind keine SQL-Injections möglich.

PHP:

function mysql_queryf($queryformat /* ,$queryarg1,$queryarg2,.... */)
{
    $queryargs=func_get_args();
    
    $vars=array();
    for($i=1;$i<count($queryargs);$i++)
        $vars[]=mysql_real_escape_string($queryargs[$i]);
        
    $querystring=vsprintf($queryformat,$vars);
    return mysql_query($querystring);
}

Benutzt wird sie so, wie man es von xprintf() gewohnt ist:

PHP:

// früher mit mysql_query() direkt:
$res=mysql_query("SELECT `field` FROM `table` WHERE `userid`=".$_POST['userid']." AND `pw`='".$_POST['pw']."'");

// besser mit mysql_queryf():
$res=mysql_queryf("SELECT `field` FROM `table` WHERE `userid`=%u AND `pw`='%s'",$_POST['userid'],$_POST['pw']);

MrToiz · 2 Juni 2006

Ich habe mir die Funktion noch leicht modifiziert

PHP:

function mysql_queryf()
{
	$args	= func_get_args();
	$query	= array_shift($args);
	// Padding falls %s am Ende der Query
	$query	.= ' ';
	// %s die nicht in Quotes stehen ersetzen durch "%s"
	$query	= preg_replace('/([^\'"])%s([^\'"])/', '\\1"%s"\\2', $query);
	// Padding wieder entfernen (eigentlich unnötig aber egal ;))
	$query	= substr($query, 0, -1);
	$args	= array_map('mysql_real_escape_string', $args);
	return mysql_query(vsprintf($query, $args));
}

// Benutzung
$result	= mysql_queryf('SELECT field FROM table WHERE userid = %d AND pw = %s', $_POST['userid'], $_POST['pw']);

Bin zwar noch nicht ganz zufrieden so (z.B. kann man jetzt kein VARCHAR mehr auf NULL setzen), aber immerhin spart es schonmal 2 Zeichen pro String, der in der Query verwendet wird (beim Tippen zumindest)

Gruß,
Xgame

P.S.: Ich ziehe %d gegenüber %u vor, da sonst negative Zahlen auf einmal doch zu positiven werden, aber das ist Geschmackssache...

theHacker · 2 Juni 2006

Xgame schrieb:
P.S.: Ich ziehe %d gegenüber %u vor, da sonst negative Zahlen auf einmal doch zu positiven werden, aber das ist Geschmackssache...

Wenn ich weiß, dass mein Integer negativ werden kann, nehm ich freilich auch %d. Aber bei UserIDs, wo ich mir für -1 etc. keine Sonderbedeutung ausgedacht hab, kann man immer von positiven IDs ausgehen.

Paladin · 16 April 2007

Nur für mich, damit ich besser schlafen kann. Ich habe die Funktion mal 'verkürzt', könnt ihr mir sagen, ob die Bedingung (vermeidung von Injections) noch sicher erfüllt ist.

PHP:

function mysql_query_safe()
{
	$args    = func_get_args();
	$query   = array_shift($args);
	$args    = array_map('mysql_real_escape_string', $args);
	return mysql_query(vsprintf($query, $args));
}

tleilax · 24 April 2007

Damit hier auch mal 'ne Antwort kommt:

Deine Variante ist auf jeden Fall immer noch sicher. Und dazu noch eigentlich überaus schick.

Das einzige, was halt zu Problemen führen könnte, sind NULL-Werte, aber das wird bei den wenigsten selbstgeschriebenen SQL-Klassen bedacht.

topfkanne · 20 Oktober 2007

für was steht eigentlich das %s und %u ??

ich suche noch ne schöne mysql klasse zur einfachen handhabung und gegen sql injections... hab die im 1. post probiert aber irgendwie ging das nicht.

über google kann man auch kaum klassen finden :/

theHacker · 20 Oktober 2007

topfkanne schrieb:
für was steht eigentlich das %s und %u ??

Für String und unsigned-Integers. Siehe hierzu die Manual-Page zu sprintf().

DadyCool · 20 April 2008

Hi,

Also ich habe als erstes eine Klasse für den DB Zugriff angelegt: (habe einige Anregungen aus Foren zu Hilfe genommen)

PHP:

<?php

class Datenbankzugriff {

  //Variablendefinitionund-deklaration
    // Für die Datenbank Verbindung
    private $database = null;      
    private $sql_server = "localhost";
    private $sql_user = "xxx";
    private $sql_password = "xxx";
    private $sql_db_name = "entwicklung";  // Datenbankname

    // Für die Datenbankanfrage
    private $_sgl = null;
    private $_result = null;
    private $_errno = 0;
    private $_error = "";


    //Konstruktor
function __construct() {
  //Datenbankverbindungherstellen
  $this->database = mysql_connect($this->sql_server, $this->sql_user, $this->sql_password);
  if(false === $this->database) {
    //return false;
    echo "mysql_connect: ".mysql_error()."<br>";
    die();
  }

  if($this->database) {
    $this->db_select = mysql_select_db($this->sql_db_name,$this->database);
    if(false === $this->db_select) {
      //return false;
      echo "mysql_select_db: ".mysql_error()."<br>";
      die();
    }
  }

}
    //Destruktor
function __destruct() {
      // Verbindung zu Server trennen
    $close = @mysql_close($this->database);
    if(false === $close) {
      //return flase;
      echo "mysql_close ".mysql_error()."<br>";
    } else {
      echo "DB geschlossen";
    }


  }
    // mysql query
function mysql_queryf() {
    // Benutzung wie xprintf()
    // $result = mysql_queryf('SELECT field FROM table WHERE userid = %d AND pw = %s', $_POST['userid'], $_POST['pw']);
    $args    = func_get_args();
    $query    = array_shift($args);
    // Padding falls %s am Ende der Query
    $query    .= ' ';
    // %s die nicht in Quotes stehen ersetzen durch "%s"
    $query    = preg_replace('/([^\'"])%s([^\'"])/', '\\1"%s"\\2', $query);
    // Padding wieder entfernen (eigentlich unnötig aber egal ;))
    $query    = substr($query, 0, -1);
    $args    = array_map('mysql_real_escape_string', $args);
    $this->_result = mysql_query(vsprintf($query, $args));
    if(!$this->_result) {
      $this->_errno = mysql_errno();
      $this->_error = mysql_error();
    }
    $this->errorCheck();
  //  if (!isset($this->_result)) { $this->_result = 0; }
}
    // error
function error() {
    // Result-ID in einer tmp-Variablen speichern
    $tmp = $this->_result;

    // Variable in boolean umwandeln
    $tmp = (bool)$tmp;

    // Variable invertieren
    $tmp = !$tmp;

    // und zurückgeben
    return $tmp;
  }
    // Error beschreiben
function getError() {
    if($this->error()) {
        $str  = "Anfrage:\n".$this->_sql."\n";
        $str .= "Antwort:\n".$this->_error."\n";
        $str .= "Fehlercode: ".$this->_errno;
    } else {
        $str = "Kein Fehler aufgetreten.";
    }
   return $str;
  }
    // Result fetchen
function fetch() {
    if($this->error()) {
        echo "Es trat ein Fehler auf. Bitte überprüfen sie ihr\n";
        echo "MySQL-Query.\n";
        $return = null;
    } else {
        $return = mysql_fetch_array($this->_result);
    }
    return $return;
  }

function numRows() {
    if($this->error()) {
        $return = -1;
    } else {
        $return = mysql_num_rows($this->_result);
    }
    return $return;
  }

function errorCheck() {
	if($this->error()) {
        echo "<pre>\n";
        echo $this->getError();
        echo "</pre>\n";
        echo "</body>\n";
        echo "</html>\n";
        die();
    }
}

}

Würde mich über eure Kritik freuen

Jetzt habe ich mal eine allgemeine Frage zu euren SQL statments.

Definiert ihr für jede Abfrage ein neues SQL Statement, oder macht ihr z.B. Funktionen für jedes Statement?

z.b. habe ich mir folgendes überlegt: z.B. eine SQL Klasse mit Klassenmethoden...

PHP:

function gibAlleDatenseatze($FROM) {
  if (in_array($FROM, $this->ERLAUBTE_TABELLEN)) {
    $result = 'SELECT * FROM '.$FROM.'';
    return $result;
  } else {
    die("Fehlercode: 001");
  }
}

Aufruf:

PHP:

$db = new Datenbankzugriff();
$sql = new Sql();
$db->mysql_queryf($sql->gibAlleDatenseatze("testTabelle"));

in wie weit macht sowas sin? Wie geht ihr denn so vor?

DadyCool

ice-breaker · 20 April 2008

1. Klasssen werden gekapselt und machen bestimmt kein die, suche mal nach Exceptions
2. Naming-Konzept der privaten Attribute überdenken, entweder oder

3. Wenn du schon Attribute private machst, warum nicht noch den Methoden den richtigen access geben
4. Was ist wenn ich gerne per %s ein CURRENT_TIMESTAMP einfügen möchte? der darf nicht in Anführungszeichen
5. getError sollte ein assoziaitves Array zurückgeben statt einen String
6. Der Db-Zugang sollte im Konstruktor übergeben werden und nicht statisch da drinne stehen

7. das mit deinem gibAlleDatensätze geht in Richtung ORM, schau dir das mal an

Bedenke beim Schreiben einer Klasse, dass sie nicht nur für diesen einen Fall genutzt werden soll, sondern dass sie so variable ist, dass du sie ohne Änderung in jeder Software nutzen kannst, dann musst du das nicht jedesmal ändern/neu schreiben

DadyCool · 21 April 2008

@ice-breaker
Danke für deine Anregungen

ZeroCCC · 23 April 2008

-Und wenn du grade bei Punkt 1 bist kannst du mal die Fehlerbehandlung grundlegend überdenken. Du hast da einmal die() drin, einmal error() und zusätzlich echos. Du würdest bei weitem besser kommen würdest du das alles einheitlich mit Exceptions lösen.

-Was soll der quatsch im Destructor? Mit mysql_close() geh ich noch mit, aber die echos :-?

-unvorteilhaft ist auch du kannst nur 1 Query aufeinmal ausfrühren... kommt zwar relativ selten vor dass man Queries verschachteln muss, aber es kann vorkommen

-dir fehlen da irgendwie noch ein paar funktionen, es gibt noch mehr als nur mysql_num_rows :roll:

und zu gibAlleDatenseatze()... das ist in der Form quatsch. Wie oft verwendest du SELECT * FROM blub? Wenn du jetzt oft gesagt hast machst du was falsch. Es gibt solche Ansätze wie schon ice-breaker gepostet hat, aber die gehen in eine andere Richtung.

ice-breaker · 23 April 2008

ZeroCCC schrieb:
-unvorteilhaft ist auch du kannst nur 1 Query aufeinmal ausfrühren... kommt zwar relativ selten vor dass man Queries verschachteln muss, aber es kann vorkommen

ist doch mit PDO ebenso, das muss doch erst über PDO::MYSQL_ATTR_USE_BUFFERED_QUERY aktvieren, dass sich Querys "parallel" ausführen lassen

ZeroCCC · 23 April 2008

ice-breaker schrieb:
ist doch mit PDO ebenso, das muss doch erst über PDO::MYSQL_ATTR_USE_BUFFERED_QUERY aktvieren, dass sich Querys "parallel" ausführen lassen

Mag sein, hab mir pdo noch nie so genau angeschaut. Ich finds aber drotzdem unvorteilhaft.

DadyCool · 23 April 2008

habe es mal überarbeitet, was haltet ihr davon?

PHP:

require_once('Klassen/DBException.php');
require_once('Klassen/DBiterator.php');


/**
  * Abfrage und Manipulation einer Mysql-Datenbank.
  */
class MysqlDB {

  /**
  * Datenbankhandle.
   */
  private $resource = false;

/**
  * Datenbank Result.
   */
  private $result = '';


  /**
  * Konstruktor.
  */
  function __construct($host, $name, $user, $passwd)
  {
     // Mit der Datenbank verbinden
     if (!$this->resource = mysql_connect($host, $user, $passwd)) {
       throw new DBException();
     }
     // Datenbank auswählen
     if (!mysql_select_db($name, $this->resource)) {
      throw new DBException();
     }
   }
   
 /**
  * Dekonstruktor.
  */
  function __destruct()
  {
    if(false === mysql_close($this->resource)) {
      throw new DBException();
    }
  }
  
 /**
  * SQL die an die DB schicken.
  * Ergbniss an DBIteration geben
  */
  function query() {
    $args    = func_get_args();
    $query    = array_shift($args);
    // Padding falls %s am Ende der Query
    $query    .= ' ';
    // %s die nicht in Quotes stehen ersetzen durch "%s"
    $query    = preg_replace('/([^\'"])%s([^\'"])/', '\\1"%s"\\2', $query);
    // Padding wieder entfernen (eigentlich unnötig aber egal ;))
    $query    = substr($query, 0, -1);
    $args    = array_map('mysql_real_escape_string', $args);
    $this->result = mysql_query(vsprintf($query, $args));
     if(!$this->result) {
       throw new DBException();

    }
    return new DBiterator($this->result);
  }
}

PHP:

/**
 * Ursprung ist von Hinrich Donner
 * https://www.phpbar.de/
 **/


class DBException extends Exception {

  function __construct()
  {
    $message = sprintf('%04d: %s', mysql_errno(), mysql_error());
    parent::__construct($message, mysql_errno());
  }

}

PHP:

/**
 * Ursprung ist von Hinrich Donner
 * https://www.phpbar.de/
 **/


class DBiterator implements Iterator
  {
     protected $num_rows = 0;
     protected $rows = array();

     /**
      * Fetch
      */
     public function __construct($resource) {
        while ($row = mysql_fetch_assoc($resource))
        {
            $this->_rows[] = $row;
            ++ $this->num_rows;
         }

     }

     public function current()
     {
         return current($this->rows);
     }

     /**
      * Das nächste Element 
      */
     public function next()
     {
         return next($this->rows);
     }

     /**
      * Der Index des aktuellen Elements
      */
     public function key()
     {
         return key($this->rows);
     }

     /**
      * erste Element
      */
     public function rewind()
     {
         return reset($this->rows);
     }

     /**
      * aktuellen Elements 
      */
     public function valid()
     {
         return (bool) is_array($this->current());
     }

ice-breaker · 23 April 2008

wie wäre es mit Meldungen in den Exceptions, die den Fehler beschreiben?

und ohne die DBIterator-Klasse kann man kaum etwas über den Aufbau sagen

flaschenkind · 23 April 2008

ice-breaker schrieb:
3. Wenn du schon Attribute private machst, warum nicht noch den Methoden den richtigen access geben

Das heißt auch mal nen public dahinschreiben, wenns passt

Haste nämlich jetzt nicht. Hab mir den Rest auch gar nicht genau angeguckt, nur das fällt sofort auf.
Zeig doch auch nochmal die andern Klassen, die du dann einwirst

DadyCool · 23 April 2008

ice-breaker schrieb:
wie wäre es mit Meldungen in den Exceptions, die den Fehler beschreiben?

und ohne die DBIterator-Klasse kann man kaum etwas über den Aufbau sagen

habs bearbeitet, siehe oben

hasiLein · 31 Mai 2008

*edit* man kann sich auch anstellen :-?

ice-breaker · 31 Mai 2008

mal ganz ehrlich was soll der sch*** hier? :evil:

Wenn jemand irgendsolche dämlichen Videos sucht, kann er das auch in youtube tun.
Hier geht es um Programmierung und net um irgendwelche Videos wo jemand in nen paar min ne Schwachstelle findet !

Edit: k, wird von den mods geduldet...

[PHP/MySQL] Vermeidung von SQL-Injections

theHacker

sieht vor lauter Ads den Content nicht mehr

MrToiz

Well-known member

theHacker

sieht vor lauter Ads den Content nicht mehr

Paladin

Programmierer

tleilax

be forever curious

topfkanne

♪ ♫

theHacker

sieht vor lauter Ads den Content nicht mehr

DadyCool

Well-known member

ice-breaker

return void

DadyCool

Well-known member

ZeroCCC

wasn das?

ice-breaker

return void

ZeroCCC

wasn das?

DadyCool

Well-known member

ice-breaker

return void

flaschenkind

Well-known member

DadyCool

Well-known member

hasiLein

PHP Coder

ice-breaker

return void

[PHP/MySQL] Vermeidung von SQL-Injections

sieht vor lauter Ads den Content nicht mehr

Well-known member

sieht vor lauter Ads den Content nicht mehr

Programmierer

be forever curious

&#9834; &#9835;

sieht vor lauter Ads den Content nicht mehr

Well-known member

return void

Well-known member

wasn das?

return void

wasn das?

Well-known member

return void

Well-known member

Well-known member

PHP Coder

return void

♪ ♫