Preventing SQL injecting in a database class

Question

I'm building a database class and thought it'd be a good idea to incorporate some form of SQL injection prevention (duh!). Here's the method that runs a database query:

class DB
{
    var $db_host    = 'localhost';
    var $db_user    = 'root';
    var $db_passwd  = '';
    var $db_name    = 'whatever';

    function query($sql)
    {
        $this->result = mysql_query($sql, $this->link);
        if(!$this->result)
        {
           $this->error(mysql_error());
        } else {
            return $this->result;
        }
    }
}

There's more in the class than that but I'm cutting it down just for this. The problem I'm facing is if I just use mysql_real_escape_string($sql, $this->link); then it escapes the entire query and leads to a SQL syntax error. How can I dynamically find the variables that need to be escaped? I want to avoid using mysql_real_escape_string() in my main code blocks, i'd rather have it in a function.

Thanks.

Answer 1

The whole idea of preventing SQL injection attacks is to prevent users from running their own SQL. Here, it seems like you want to allow users to run their own SQL, so why limit them?

Or if you're not not allowing users to pass SQL into the query() method. This is too low of a level to implement escaping parameters. As you've realized, you want to escape parameters only, not the entire SQL statements.

If you parametrize the SQL, then you could escape just the parameters. In this case, I'm assuming the users can affect the values of the parameters, and not the SQL.

Take a look at PDO's bindParam() or Zend_DB's query() methods to get an idea of how this is implemented in other database interfaces.

Answer 2

To do this the right way, you have to sanitize the stuff as you're building the query, or put that into your class to pass in parameters independent of the core query.

Answer 3

The problem is that by the time you have built an SQL query it is too late to be able to prevent injection by finding variables - otherwise it'd already be built into PHP.

The escaping needs to be done much earlier when you build the queries. You could use a query building class.

However I would recommend a different approach - which is to have a layer that provides a database table as an object, here is an example user object which is derived from the base db entity class which provides a complete interface to a database using active record pattern, and the iterator pattern.

I'll illustrate this with some examples; the neat thing here is iterators as you can abstract away a lot more and have some quite generic classes further on down the line to extract the data.

To create a user record using the above approach:

$user = new DbUser();
$user->create();
$user->set_email('[email protected]');
$user->write();

To read a user record:

$user = new DbUser();
$user->set_email('[email protected]');
if ($user->load_from_fields())
{
}

To iterate through records:

$user_iterator = DbUser::begin();
if ($user_iterator->begin())
{
    do
    {
         $user = $user_iterator->current();
         echo $user->get_email();
    } while ($user_iterator->next());
}

Answer 4

Hello,

There are two approaches to prevent an SQL injection attack: Blacklist based approach and Whitelist based approach.

Blacklist approach implies that you need to check the whole query string and identify unwanted code and remove it. This is hell lot of difficult. Instead, use the white-list approach by using parametrized SQL. This way you will be sure that the only query that will be executed will be the one which you intentionally built using your code, and any injection attempt will fail as all the injection queries will be a part of parameter and hence wont be executed by the database. You are trying to find a way of preventing the injection after the query has already been built up, which indirectly means a blacklist based approach.

Try using parametrized SQL in your code, its one of the secure coding principles adapted globally.

Answer 5

Either parametrized or try building the DB class in order to pass all the values for WHERE ( and whatever may use values ) using a something like :

$db->where(x,y);

ie.

$db->where('userid','22');

And in the class corpus use something like

function where(var x, var y) // method
{
    $this->where .= x . ' = '.mysql_real_escape_string(y);
}

Of course this needs cleaning to support multiple WHERE inputs.

Answer 6

Me, I solved this problem by adding parameters to the query function. I found that codeigniter did it pretty nicely, so I adapted it to my own tastes.

Example:

$result = Database::query('INSERT INTO table (column1,column2,column3) VALUES(?,?,?)',array($value1,$value2,$value3));



public static $bind_marker = '?';
public static function query($query, $binds = FALSE)
    {
        if($binds !== FALSE)
        {
            $query = self::compile_binds($query,$binds);
        }
        // $query now should be safe to execute
}

private static function compile_binds($query, $binds)
    {
        if(strpos($query, self::$bind_marker) === FALSE)
        {
            return $query;
        }

        if(!is_array($binds))
        {
            $binds = array($binds);
        }

        $segments = explode(self::$bind_marker, $query);

        if(count($binds) >= count($segments))
        {
            $binds = array_slice($binds, 0, count($segments)-1);
        }

        $result = $segments[0];
        $i = 0;
        foreach($binds as $bind)
        {
            if(is_array($bind))
            {
                $bind = self::sanitize($bind);
                $result .= implode(',',$bind);
            }
            else
            {
                $result .= self::sanitize($bind);
            }

            $result .= $segments[++$i];
        }

        return $result;
    }

public static function sanitize($variable)
{
    if(is_array($variable))
    {
        foreach($variable as &$value)
        {
            $value = self::sanitize($value);
        }
    }
    elseif(is_string($variable))
    {
        mysql_real_escape_string($variable);
    }
    return $variable;
}

The major addition I added from codeigniter's version is I can use an array as a parameter which is useful for using "IN":

$parameters = array
    (
        'admin',
        array(1,2,3,4,5)
    );

$result = Database::query("SELECT * FROM table WHERE account_type = ? AND account_id IN (?)",$parameters);

Answer 7

The whole point of code-injection prevention is that you want to differentiate between your sql and sql that users injected. You can't do that any more at this lowest level. Lets give an example:

select * from users where username='test' and password='itisme' or '4'='4'

This seems perfect valid sql, but it may also be an sql injected version of:

"select * from users where username='test' and password='" . "itisme' or '4'='4". "'"

So you have to do it further up in your code, or use wrappers as the others suggested.

Answer 8

Why reinvent the wheel ? Just extend PDO and use parameterized queries. If you don't know what it is, read the doc which contains a lot of examples to get started.