8 ms·

This isn't a search for SQL injection, its a search for a couple things that you often find in older PHP code that is generally hacked together and likely to ha

This isn't a search for SQL injection, its a search for a couple things that you often find in older PHP code that is generally hacked together and likely to have SQL injection vulnerabilities for historical and cultural reasons. However it's perfectly easy to avoid SQL injection even using these things. $id = mysql_real_escape_string($_GET['id']); $res = mysql_query("SELECT foo FROM bar WHERE id='$id'"); That may be ugly, but it's bulletproof regarding injection.

siddboots 13y ago
Most of the search results show exploitable code. Regardless, this is a search for SQL injection vulnerability, even if not every search result falls under that category.
thangalin 13y ago
use PDO; use PDOException; /** * Used for interacting with the database. Usage: * <pre> * $db = Database::get(); * $db->call( ... ); * </pre> */ class Database extends Obj { private static $instance; private $dataStore; /** * Sets the connection that this class uses for database transactions. */ public function __construct() { global $dbhost; global $dbname; global $dbuser; global $dbpass; try { $this->setDataStore( new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) ); } catch( PDOException $ex ) { $this->log( $ex->getMessage() ); } } /** * Returns the singleton database instance. */ public function get() { if( self::$instance === null ) { self::$instance = new Database(); } return self::$instance; } /** * Call a database function and return the results. If there are * multiple columns to return, then the value for $params must contain * a comma; otherwise, without a comma, the value for $params is used * as the return column name. For example: * *- SELECT $params FROM $proc( ?, ? ); -- with comma *- SELECT $proc( ?, ? ) AS $params; -- without comma *- SELECT $proc( ?, ? ); -- empty * * @param $proc Name of the function or stored procedure to call. * @param $params Name of parameters to use as return columns. */ public function call( $proc, $params = "" ) { $args = array(); $count = 0; $placeholders = ""; // Key is zero-based (e.g., $proc = 0, $params = 1). foreach( func_get_args() as $key => $parameter ) { // Skip the $proc and $params arguments to this method. if( $key < 2 ) continue; $count++; $placeholders = empty( $placeholders ) ? "?" : "$placeholders,?"; array_push( $args, $parameter ); } $sql = ""; if( empty( $params ) ) { // If there are no parameters, then just make a call. $sql = "SELECT recipe.$proc( $placeholders )"; } else if( strpos( $params, "," ) !== false ) { // If there is a comma, select the column names. $sql = "SELECT $params FROM recipe.$proc( $placeholders )"; } else { // Otherwise, select the result into the given column name. $sql = "SELECT recipe.$proc( $placeholders ) AS $params"; } $statement = $this->getDataStore()->prepare( $sql ); //$this->log( "SQL: $sql" ); for( $i = 1; $i <= $count; $i++ ) { //$this->log( "Bind " . $i . " to " . $args[$i - 1] ); $statement->bindParam( $i, $args[$i - 1] ); } $statement->execute(); $result = $statement->fetchAll(); $this->decodeArray( $result ); return $result; } /** * Converts an array of numbers into an array suitable for usage with * PostgreSQL. * * @param $array An array of integers. */ public function arrayToString( $array ) { return "{" . implode( ",", $array ) . "}"; } /** * Recursive method to decode a UTF8-encoded array. * * @param $array - The array to decode. * @param $key - Name of the function to call. */ private function decodeArray( &$array ) { if( is_array( $array ) ) { array_map( array( $this, "decodeArray" ), $array ); } else { $array = utf8_decode( $array ); } } private function getDataStore() { return $this->dataStore; } private function setDataStore( $dataStore ) { $this->dataStore = $dataStore; } } Example usage: $db = Database::get(); $result = $db->call( "is_existing_cookie", "existing", $cookie_value ); return isset( $result[0] ) ? $result[0]["existing"] > 0 : false; Another example: private function authenticate() { $db = Database::get(); $db->call( "authentication_upsert", "", $this->getCookieToken(), $this->getBrowserPlatform(), $this->getBrowserName(), $this->getBrowserVersion(), $this->getIp() ); } Switching to PDO is better. Critiques welcome on Code Review SE. http://codereview.stackexchange.com/questions/26507/generic-method-for-database-calls http://codereview.stackexchange.com/questions/26507/generic-...
serge2k 13y ago
I'm not a PHP dev, but I have heard that mysql_real_escape_string is not a preferred method of preventing SQL injection anymore?
- krapp 13y ago
  Currently, the use of PDO is preferred and anything involving the mysql libraries should be avoided, and support for them is being deprecated in PHP anyway. I found this interesting, though, regarding specifically SQL injection when mysql_real_escape_string is used: http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string http://stackoverflow.com/questions/5741187/sql-injection-tha... basically the argument appears to boil down to mixed character sets causing escaping not to act as predicted. I can't speak to the validity of it though.
  - ecaron 13y ago
    Support for the mysql_ library is being deprecated. Support for the mysqli library is alive and well (although I personally think people should adopt PDO anyway.)
- bluetooth 13y ago
  It's not. The preferred method of preventing SQL injections is via prepared statements. mysql_real_escape_string is only suitable for strings (as the name implies). Something like SELECT * FROM table WHERE id=$_GET['field'] where $_GET['field'] has been passed through mysql_real_escape_string is still vulnerable. Using prepared statements forces php to send data to the DBMS in such a way that it cannot confuse user input from the actual SQL. This is due to the fact that preparing data forces you to give types to the data before you use it in a query. Escaping input (such as with mysql_real_escape_string) makes this confusion still possible.
hcarvalhoalves 13y ago
"mysql_real_escape_string" is the silliest function name ever. I assume there is a "mysql_escape_string" function that doesn't do what you expect it to do?
- ecaron 13y ago
  The difference between the two is _real_ reacts based on the current connection's character set, and the other doesn't. Yet another reason to jump on the PDO bandwagon is to avoid having to know that subtle difference.
- deleted 13y ago
  [deleted]
- noinput 13y ago
  Don't forget strstr(). So nice, they named it twice.
  - tjohns 13y ago
    In PHP's defense, strstr() originally comes from C: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/strstr.3.html https://developer.apple.com/library/mac/documentation/Darwin...
  - a1a 13y ago
    PHP's naming of functions is really horrible. For example the random usage of underscores: strtoupper(...) substr_compare(...) str_split(...) str_word_count(...)
- EdiX 13y ago
  They got the name from mysql's client library API. They just made a wrapper for it.
- brokenparser 13y ago
  Surprisingly, no one mentioned that mysql_real_escape_string still fails to properly escape a string when the character set has been changed using a SET NAMES or SET CHARACTER SET query. Never mind the big yellow warning sign in the manual. The search goes like this: https://github.com/search?q=mysql_query+%22SET+NAMES%22+mysql_real_escape_string&type=Code https://github.com/search?q=mysql_query+%22SET+NAMES%22+mysq...
Nodex 13y ago
That is NOT bulletproof.

siddboots 13y ago

Most of the search results show exploitable code. Regardless, this is a search for SQL injection vulnerability, even if not every search result falls under that category.

thangalin 13y ago

use PDO; use PDOException; /** * Used for interacting with the database. Usage: * <pre> * $db = Database::get(); * $db->call( ... ); * </pre> */ class Database extends Obj { private static $instance; private $dataStore; /** * Sets the connection that this class uses for database transactions. */ public function __construct() { global $dbhost; global $dbname; global $dbuser; global $dbpass; try { $this->setDataStore( new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) ); } catch( PDOException $ex ) { $this->log( $ex->getMessage() ); } } /** * Returns the singleton database instance. */ public function get() { if( self::$instance === null ) { self::$instance = new Database(); } return self::$instance; } /** * Call a database function and return the results. If there are * multiple columns to return, then the value for $params must contain * a comma; otherwise, without a comma, the value for $params is used * as the return column name. For example: * *- SELECT $params FROM $proc( ?, ? ); -- with comma *- SELECT $proc( ?, ? ) AS $params; -- without comma *- SELECT $proc( ?, ? ); -- empty * * @param $proc Name of the function or stored procedure to call. * @param $params Name of parameters to use as return columns. */ public function call( $proc, $params = "" ) { $args = array(); $count = 0; $placeholders = ""; // Key is zero-based (e.g., $proc = 0, $params = 1). foreach( func_get_args() as $key => $parameter ) { // Skip the $proc and $params arguments to this method. if( $key < 2 ) continue; $count++; $placeholders = empty( $placeholders ) ? "?" : "$placeholders,?"; array_push( $args, $parameter ); } $sql = ""; if( empty( $params ) ) { // If there are no parameters, then just make a call. $sql = "SELECT recipe.$proc( $placeholders )"; } else if( strpos( $params, "," ) !== false ) { // If there is a comma, select the column names. $sql = "SELECT $params FROM recipe.$proc( $placeholders )"; } else { // Otherwise, select the result into the given column name. $sql = "SELECT recipe.$proc( $placeholders ) AS $params"; } $statement = $this->getDataStore()->prepare( $sql ); //$this->log( "SQL: $sql" ); for( $i = 1; $i <= $count; $i++ ) { //$this->log( "Bind " . $i . " to " . $args[$i - 1] ); $statement->bindParam( $i, $args[$i - 1] ); } $statement->execute(); $result = $statement->fetchAll(); $this->decodeArray( $result ); return $result; } /** * Converts an array of numbers into an array suitable for usage with * PostgreSQL. * * @param $array An array of integers. */ public function arrayToString( $array ) { return "{" . implode( ",", $array ) . "}"; } /** * Recursive method to decode a UTF8-encoded array. * * @param $array - The array to decode. * @param $key - Name of the function to call. */ private function decodeArray( &$array ) { if( is_array( $array ) ) { array_map( array( $this, "decodeArray" ), $array ); } else { $array = utf8_decode( $array ); } } private function getDataStore() { return $this->dataStore; } private function setDataStore( $dataStore ) { $this->dataStore = $dataStore; } } Example usage: $db = Database::get(); $result = $db->call( "is_existing_cookie", "existing", $cookie_value ); return isset( $result[0] ) ? $result[0]["existing"] > 0 : false; Another example: private function authenticate() { $db = Database::get(); $db->call( "authentication_upsert", "", $this->getCookieToken(), $this->getBrowserPlatform(), $this->getBrowserName(), $this->getBrowserVersion(), $this->getIp() ); } Switching to PDO is better. Critiques welcome on Code Review SE. http://codereview.stackexchange.com/questions/26507/generic-method-for-database-calls http://codereview.stackexchange.com/questions/26507/generic-...

serge2k 13y ago

I'm not a PHP dev, but I have heard that mysql_real_escape_string is not a preferred method of preventing SQL injection anymore?

krapp 13y ago
Currently, the use of PDO is preferred and anything involving the mysql libraries should be avoided, and support for them is being deprecated in PHP anyway. I found this interesting, though, regarding specifically SQL injection when mysql_real_escape_string is used: http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string http://stackoverflow.com/questions/5741187/sql-injection-tha... basically the argument appears to boil down to mixed character sets causing escaping not to act as predicted. I can't speak to the validity of it though.
- ecaron 13y ago
  Support for the mysql_ library is being deprecated. Support for the mysqli library is alive and well (although I personally think people should adopt PDO anyway.)
bluetooth 13y ago
It's not. The preferred method of preventing SQL injections is via prepared statements. mysql_real_escape_string is only suitable for strings (as the name implies). Something like SELECT * FROM table WHERE id=$_GET['field'] where $_GET['field'] has been passed through mysql_real_escape_string is still vulnerable. Using prepared statements forces php to send data to the DBMS in such a way that it cannot confuse user input from the actual SQL. This is due to the fact that preparing data forces you to give types to the data before you use it in a query. Escaping input (such as with mysql_real_escape_string) makes this confusion still possible.

hcarvalhoalves 13y ago

"mysql_real_escape_string" is the silliest function name ever. I assume there is a "mysql_escape_string" function that doesn't do what you expect it to do?

ecaron 13y ago
The difference between the two is _real_ reacts based on the current connection's character set, and the other doesn't. Yet another reason to jump on the PDO bandwagon is to avoid having to know that subtle difference.
deleted 13y ago
[deleted]
noinput 13y ago
Don't forget strstr(). So nice, they named it twice.
- tjohns 13y ago
  In PHP's defense, strstr() originally comes from C: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/strstr.3.html https://developer.apple.com/library/mac/documentation/Darwin...
- a1a 13y ago
  PHP's naming of functions is really horrible. For example the random usage of underscores: strtoupper(...) substr_compare(...) str_split(...) str_word_count(...)
EdiX 13y ago
They got the name from mysql's client library API. They just made a wrapper for it.
brokenparser 13y ago
Surprisingly, no one mentioned that mysql_real_escape_string still fails to properly escape a string when the character set has been changed using a SET NAMES or SET CHARACTER SET query. Never mind the big yellow warning sign in the manual. The search goes like this: https://github.com/search?q=mysql_query+%22SET+NAMES%22+mysql_real_escape_string&type=Code https://github.com/search?q=mysql_query+%22SET+NAMES%22+mysq...

Nodex 13y ago

That is NOT bulletproof.