Table, Column and Alias names in SQL queries should be quoted. I created a content type called match (for a game clan site) and the signup admin page threw up an SQL error because "match" is a reserved word in MySQL.

I fixed this for my specific case by updating _signup_date_admin_sql() in includes/date.inc with the following...

function _signup_date_admin_sql($content_type) {
  // Get the date field information for this content type.
  $field = signup_date_field($content_type);

  // In the case where the same CCK date field is being reused on multiple
  // content types, we'll potentially be JOINing on the same tables and
  // columns for different content types.  To defend against duplicate table
  // names or ambiguous columns in the query, use the content type to alias.
  $alias = $content_type;

  // See what fields to SELECT.
  $fields[] = $alias .'.'. $field['database']['columns']['value']['column'];
  if (isset($field['database']['columns']['timezone']['column'])) {
    $fields[] = $alias .'.'. $field['database']['columns']['timezone']['column'];
  }
  $table = '{'. $field['database']['table'] .'} `'. $alias . '`';
  return array(
    'fields' => $fields,
    'joins' => array("LEFT JOIN $table ON `$alias`.vid = n.vid"),
  );
}

Notice the additions of the backticks (`) around the alias.

However, having a quick glance over some of the code, I think this could be a widespread issue. I am not familiar with the internals if drupal enough to know if there is a function you can call to check names, or if similar updates are going to have to be made to all your database code.

CommentFileSizeAuthor
#1 350872_sql_table_alias_quotes.1.patch1.3 KBdww

Comments

dww’s picture

Version: 6.x-2.x-dev » 6.x-1.x-dev
Assigned: Unassigned » dww
Status: Active » Needs review
StatusFileSize
new1.3 KB

Thanks for the report. I hadn't considered this case. After some testing, it appears you don't need the quotes around the alias name when it's used to specify a given field. So:

match.field_name

is ok, but

LEFT JOIN {table} match

is not.

This one function is the only place where we're using admin-supplied data inside SQL queries like this, so I'm fairly confident that the attached patch is all we need.

Even though the admin interface prevents content type names with special characters (only lowercase, numbers, and underscore are allowed), I figured it doesn't hurt to be extra paranoid and call db_escape_table() on the content type name before using it as a table alias. Since this isn't a vulnerability at all, just defensive programming, I can mention it here and post it in a public patch like this. If there were actually an SQL injection bug here, I would be going through the security team...

Anyway, please give this patch a try and confirm it solves the problems for you. Works on my site, but it's nice to get confirmation.

Thanks,
-Derek

p.s. The 6.x-2.x-dev series is completely unsupported. You shouldn't be using that version at all. See the 6.x-2.x-dev release notes for more.

dww’s picture

Status: Needs review » Fixed

Committed to HEAD and DRUPAL-6--1.

dww’s picture

Oh, and DRUPAL-5--2, too. ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.