Live data from Hacker News

SQL injection search

github.com

71–80 of 114 posts

Re: SQL injection search

#71
post #10

I don't know much about PHP but I happened to rewrite some old forms a couple of years ago. The original author had relied on a technique called "magic quotes" ( http://php.net/manual/en/security.magicquotes.php ) which automatically sanitized user input. When we upgraded our version of PHP "magic quotes" had been deprecated and dropped. It would be interesting to know if some of these developers are relying on "magi…

Yeah no, magic quotes didn't ever sanitize.

Re: SQL injection search

#72

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…

    use PDO;
    use PDOException;

    /**
     * Used for interacting with the database. Usage:
     * 
     * $db = Database::get();
     * $db->call( ... );
     * 
*/ 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 getDataStore()->prepare( $sql ); //$this->log( "SQL: $sql" ); for( $i = 1; $i 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-...

Re: SQL injection search

#73

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…

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?

Re: SQL injection search

#74
post #73

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…

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?

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-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.

Re: SQL injection search

#75
post #73

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…

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?

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.

Re: SQL injection search

#77
post #17
post #13

Nice example, but not all are insecure. For example, the second one here is: $result = mysql_query('DELETE FROM saves WHERE id = '.(int)$_GET['delete']);

The search obviously doesn't find all cases, but is a good start. While there's nothing technically wrong with the example given, I might argue that since that won't work in all cases, it might be better to enforce a more rigorous policy of SQL query cleansing, or using bound params. Although this example is so simple I might not. Then again, the fact that $_GET is even available at the location the query is taking p…

Pretty much every language will make getting direct user input then passing it to a database easy. What generally makes this less easy (or at least less intuitive) is a framework. Don't compare the likes of Rails or Django to PHP. Compare Laravel4 or Symfony2.

That doesn't mean PHP doesn't deserve some stick, it does, but most of it's current reported problems spawn from backwards compatibility. Nobody should be using mysql_*, they should be using prepared statements via PDO.

They could solve this by deleteing all the functions you're not supposed to use, but a whole bunch of legacy PHP would stop working. I'm fairly sure this would illicit more hate than the current method of slowly deprecating.

Re: SQL injection search

#79

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…

"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?

Re: SQL injection search

#80
post #13

Nice example, but not all are insecure. For example, the second one here is: $result = mysql_query('DELETE FROM saves WHERE id = '.(int)$_GET['delete']);

That's an example of hazardously bad programming practices. You're one mistake away from complete disaster. You should be sure that it takes more than one mistake to expose you to that sort of risk. Casting to int is not a general purpose escaping system, and further, if you miss even one of these your entire application can be trashed. Using mysql_query at all is a sign there's something severely wrong with your app…

There is an opposing viewpoint, i.e. if you actually need an int, casting to int is one of the most reasonable ways of getting it.
Post reply on HN