Hacker News Reader

Comment by postfuturist | Hacker News Reader

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.

View on HN

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

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

PHP is a simplistic wrapper around the C API's, so you get all of the legacy of C without any of its performance and memory size benefits.

PHP has some of the insanest defaults due to its C heritage. String functions deal with bytes not characters, so cannot be used safely with utf8 without setting the mbstring.func_overload setting to replace them with unicode-aware versions (except for str_pad, which always deals in bytes). Sort() defaults to binary sorting, and cannot be tricked in any way to sort utf8 in dictionary order if you're running your server on windows (and even on linux it requires an extra parameter on every call). Natsort(), which is supposed to sort like a human would, cannot be made to sort in dictionary order at all. The proper way to sort is by using the Collator class, which is not referenced from the sort() documentation, didn't exist before PHP 5.3, and is in the optional intl extension which is usually disabled by default.

Still better than mysql though, which has a very unique interpretation of unicode collation.

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

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.

They got the name from mysql's client library API. They just made a wrapper for it.

Don't forget strstr(). So nice, they named it twice.

In PHP's defense, strstr() originally comes from C: https://developer.apple.com/library/mac/documentation/Darwin...

PHP's naming of functions is really horrible.

For example the random usage of underscores: strtoupper(...) substr_compare(...) str_split(...) str_word_count(...)

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.

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

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.

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.

Reply on news.ycombinator.com