Take the 2-minute tour ×
Server Fault is a question and answer site for professional system and network administrators. It's 100% free, no registration required.

I have an application which is completely for internal use but I'm trying to make sure I have my bases covered on SQL injection. Unless someone else has some information to point me otherwise there is no way to dynamically specify the column listing within a statement without opening the query up to SQL injection as prepared statement bindparam cannot be used to specify a column name.

For example (not possible, would throw errors):

SELECT :column_name FROM tablename WHERE other_column_name = :value

In order to still dynamically allow the calling of the column name I directly have to pass in a variable (works, but is vulnerable to injection):

SELECT $column_name FROM tablename WHERE other_column_name = :value

In order to prevent injection I check the value of $column_name against a whitelist of all the column names within the table using PHP's in_array function. If it's in the list I go ahead with the query but if it's not I throw an exception.

As there are many different places in the application where a query like this is needed I added a function to my PDO wrapper to automatically enumerate these column listings using the data from the information_schema database:

public function schema_list($database_name = NULL, $table_name = NULL, $column_name = NULL)
{   
    $query = 'SELECT table_schema AS database_name, table_name, column_name 
        FROM `information_schema`.`columns` 
        WHERE table_schema = :database_name ';
    $parameters = array();

    // Use this object's database name unless manually specified
    if (!is_null($database_name)) {
        $parameters[':database_name'] = $database_name;
    } else {
        $parameters[':database_name'] = $this->database_detail['dbname'];
    }

    // Add details for table_name if specified
    if (!is_null($table_name)) {
        $query .= 'AND table_name = :table_name ';
        $parameters[':table_name'] = $table_name;
    }

    // Add details for column_name if specified
    if (!is_null($column_name)) {
        $query .= 'AND column_name = :column_name ';
        $parameters[':column_name'] = $column_name;
    }

    $result = $this->query_select($query, $parameters);

    if ($result['sth_count'] == 0) {
        return FALSE;
    } else {
        return $result;
    }
}

Of course this returns a multidimensional array of records, the constructor of the object which eventually needs to enumerate the column listing does:

protected $table_process_columnlist;    // List of valid columns for the process table

public function __construct(dblink $global_dblink = NULL, job $global_job = NULL)
{

// other constructor secret sauce //

// Initialize valid column list
$this->table_process_columnlist = array();
$schema_list = $this->dblink->schema_list(NULL, $this->table_process);
foreach ($schema_list['sth_result'] as $table_column) {
    $this->table_process_columnlist[] = $table_column['column_name'];
}

And is then used by:

public function __get($column)
{
    if (in_array($column, $this->table_process_columnlist)) {
        $query = 'SELECT '.$column.'  
            FROM '.$this->table_process.' 
            WHERE idprocess = :idprocess
            LIMIT 1';
        $parameters = array (
            ':idprocess' => $this->idprocess,
        );

        $result = $this->dblink->query_select($query, $parameters);

        return $result['sth_result'][0][$column];
    } else {
        $e_message = 'Could not get specified column';
        throw new ACException($e_message, 99);
    }
}

Keep in mind I'm not really using prepared statements to gain performance, they are only being used to for SQL injection protection. From what I've found in tests the information_schema table will only return a result set for items which the database handle user actually has access to so the schema_list function couldn't be used to enumerate schema layout of a database which the database user doesn't already have access to.

I'm using a public __get magic function because this class is only used internally in the application and is wrapped by other classes further up in the application.

Is this a sane way to prevent SQL injection while gaining the ability to dynamically specify the column names? Are there any security concerns with this method?

share|improve this question

1 Answer 1

up vote 0 down vote accepted

I didn't read your code, but yes, a white list is the correct solution here.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.