Possible Security Flaw [#475058]

In openlayers.layers.layers.inc in the function: openlayers_layers_process_geo_data_layers(), there is a query that does not use placeholders correctly. I have updated it from what it was to this (committing code soon):

  // Get table description
  $table_desc = geo('table_desc', $layer);
  $attribute_cols = array();
  // Go through columns
  foreach ($table_desc as $cname => $cinfo){
    if ($cinfo['type'] == 'geometry'){
      $geometry_col = $cname;
      $srid = $cinfo['srid'];
    }
    else {
      $attribute_cols[] = $cname;
    }
  }
  
  // Get geometry text
  $query = "SELECT asText(%s) as %s, " . implode(',',$attribute_cols) . " from {%s}";
  $res = db_query($query, $geometry_col, $geometry_col, $layer);

I do realize that these are column names and I think it may be impossible for them to not be safe. But, I would like to work to make this correct.

Comments

Comment #1

phayes CreditAttribution: phayes commented 28 May 2009 at 15:54

Sorry about that. I did make the assumption that they would be safe -- but it was an assumption, not a well research conclusion - so better safe than sorry. Thanks zzolo.

Comment #2

phayes CreditAttribution: phayes commented 28 May 2009 at 15:59

Of course, this will change again soon as soon as geo_data has an API so we don't have to 'fondle' the data directly (in the words of vauxia). :-)

Comment #3

Heine CreditAttribution: Heine commented 28 May 2009 at 16:26

Not so fast :)

Let's take a look at %s shall we?

db_query("SELECT n.nid FROM {node} n WHERE n.title = '%s'", $title);

The %s is replaced with the value of db_escape_string($title), which prepends (for MySQL) backslashes to the following characters: \x00, \n, \r, \, ', " and \x1a. If that didn't happen, a quote in $title could close the 'data' context and insert the rest of the string in the SQL command context as you can see in the example below where UNION SELECT becomes part of the command context.

// Suppose title = "data context' UNION SELECT .... -- "

// Escaped, MySQL looks for n.title equals "data context' UNION SELECT .... -- "
SELECT n.nid FROM {node} n WHERE n.title = 'data context\' UNION SELECT .... -- '

// Not escaped, MySQL looks for n.title equals "data context" and executes a UNION join
SELECT n.nid FROM {node} n WHERE n.title = 'data context' UNION SELECT .... -- '

When you remove the quotes around %s, the string will still get escaped, but it becomes part of the command context anyway.

If you have:

db_query("SELECT asText(%s) REST_OF_QUERY", $whatever);

Then when whatever is crafted correctly:

$whatever = "somecol), s.sid FROM sessions s ... WHERE s.uid = 1 -- "

Then the query becomes

SELECT asText(somecol), s.sid FROM sessions s ... WHERE s.uid = 1 -- ) REST_OF_QUERY

Never use %s without quotes. If you need to rely on user input to know a column name, look it up in an array with known column names or use db_escape_table.

Comment #4

phayes CreditAttribution: phayes commented 29 May 2009 at 00:54

It all comes down to this code:

$table_desc = geo('table_desc', $layer);

the function is geo, which returns table descriptions, including column names. This function is not defined by us - it is defined by by the geo module. Currently geo does not let users define column names, so we are safe for now. Although I doubt it, geo may sometime in the future allow users to define column names - in which case our safe function suddenly becomes a glaring security hole. Is there any standard Drupal way of dealing with this? Where an upstream module causes a security issue inside a downstream module? Would this be a good place to implement a test?

Sidenote: Theoretically admins could upload a shapefile that have column names which contain a SQL injection attack -- weird, but possible?

Comment #5

zzolo CreditAttribution: zzolo commented 30 May 2009 at 19:04

Much obliged, Heine. db_escape_table will be the answer here since we are dealing with variable column names. Also, thanks for going in depth about how the placeholders work. It's very helpful.