Trivial SQL injection(s) with SelectQuery

Agreed. Mixed escaping of items is not a good idea as certain form API elements showed. I'd vote to consistently escape items passed to the query builder.

As would I, with this caveat: #715132: Over aggressive escaping of table names

EDIT: Screw that, we obviously want to use http://api.drupal.org/api/function/db_escape_table/7. But the point below is still true.

Also, allowing not validated user input as table/columns into a query sounds like a recipe for disaster to me anyway. For example, even if the table name is escaped, it would still allow access to every table in your system and maybe reading of data like passwords and so on. So we need to document/clarify that it is *never* safe to use user submitted values directly for building a query. You always need to use a whitelist approach.

Yeah I'm not really sure how it's possible to protect people entirely from this, seems more a documentation issue, for the points Berdir raised. Although db_escape_table() obviously makes sense.

Under no circumstances do we call db_escape_table() from within the query builder. Calling the appropriate connection object's escapeTable() method, however, is a good idea.

We should also make a note of this fact in the appropriate handbook pages at least.

table_sort is first candidate for injections

Ok, attached is a first patch. I escaped the table alias and the column name in all places I could find. I tried to do the escaping as late as possible (read: in __toString()/compile()).

Let's see what the test bot thinks about this. Not sure about the docs part yet...

Subscribe

In openid.module:

  db_delete('openid_association')
    ->condition('created + expires_in', REQUEST_TIME, '<')
    ->execute();

escapeTable() will turn "created + expires_in" into "createdexpires_in".

In database_test.test:

    $query->havingCondition('COUNT(task)', 2, '>=');

escapeTable() will turn "COUNT(task)" into "COUNTtask".

In short, don't use escapeTable() to escape fields.

Re-rolled without offending lines.

Yep, and the other one is a havingCondition() in the tests. So we need to convert the above into where() and the test case to having().

Or we could re-think if we really want this... As stated above, it doesn't make things secure but it might confuse developers unless it's very well documented.

EDIT: #12 only escapes the alias, which is basically the Or...

EDIT: #12 only escapes the alias, which is basically the Or...

I don't understand. What do you mean?

Anyway, I re-rolled it with updated documentation (havingCondition didn't even have a PHPDoc) and substitution of where() and having() for condition() and havingCondition(). I am +1 for doing this.

Not immediately relevant to this issue, but related:

QueryConditionInterface::condition was lying in saying that the default operator is =; it's actually IN or =. I don't think havingCondition should have a default operator of =. If $values is an array, it should use IN. I made these changes as well.

Also, I have no idea why havingCondition and SelectQuery::condition count the number of args and pass it around, seeing as, as far as I can tell, that argument is never used. So, I cut it out. I'm prepared to be corrected, or asked to create a new issue!

Wrong patch in #15. This is correct.

Forgot to include patch #8 in my patch. I should not write patches and listen to sessions at the same time.

Fix syntax.

One more time.

+++ includes/database/select.inc
@@ -942,11 +939,29 @@ class SelectQuery extends Query implements SelectQueryInterface {
   /* Implementations of QueryConditionInterface for the HAVING clause. */
 
-  public function havingCondition($field, $value = NULL, $operator = '=') {
-    if (!isset($num_args)) {
-      $num_args = func_num_args();
-    }
-    $this->having->condition($field, $value, $operator, $num_args);
+  /**
+   * Helper function to build most common HAVING conditional clauses.
+   *
+   * This method can take a variable number of parameters. If called with two
+   * parameters, they are taken as $field and $value with $operator having a value
+   * of IN if $value is an array and = otherwise.
+   *

I think we currently only document interfaces, not the implementations (I'm not 100% sure how this will change with #718596: Lacking standards for documenting classes, interfaces, methods). So if havingCondition() isn't documented in SelectQueryInterface, we should add it there.

What I meant in #13 was, that your patch in #12 means not to escape column names and #21 means to escape column names. Both are possible approaches as we can't have a 100% secure implementation anyway. So we need to decide which one we want to proceed. Any comments on this?

114 critical left. Go review some!

I think we should escape column names in condition() and havingCondition(). If a developer wants to circumvent that, there are where() and having(). Here is an updated patch with the PHPDoc in the right place.

#24: 769554-24.patch queued for re-testing.

The only change from the previous patch was adding havingCondition to QueryConditionInterface. Could this cause installation to fail? I'm not as familiar with PHP interfaces.

It should be added to SelectQueryInterface. QueryConditionInterface is then used internally in SelectQuery.

Ok, did that now.

i'm fine with this as long we don't regress on #715132: Over aggressive escaping of table names

#715132: Over aggressive escaping of table names should not be affected, but we can add tests for that, I think.

Removed the .htaccess part that you accidently added, no other changes.

Well #34 fixes a half dozen brain dead bugs I'm embarrassed were even still in there (lack of havingCondition docs, $num_args, etc.). I agree with #11, though, that we should not be using escapeTable() on a field. There's on guarantee that the escaping rules will always be the same and on every database. Let's introduce an escapeField() method and use that instead, even if its body is the same as escapeTable()'s. That should be a cleaner, more forward-looking approach.

As far as I can tell, the last patch didn't escape any fields. Maybe we want to start doing that? Anyway, here's a patch that adds escapeField().

Hm. OK, I misread the previous patch then. Yeah, we should be escaping field names, for the same reason we should be escaping table names. Let's add that. Also, I'm pretty sure field names can't (or at least really shouldn't) contain periods, so we can remove that from the allowed character list for fields.

Rerolled per #37

I think #38 just removes the period from escapeField.

Oh, I must have misread the patch... I saw the escapeField() function and assumed it had already been implemented. My bad.

Hmm? Removing the period from the allowed character list was part of the request in #37, so that's good.

But, we also want to start using escapeField to escape fields. That's all that's missing from the patch now.

Escaped fields in conditions and SelectQuery, also added a db_escape_field() (similiar to db_escape_table).

Also, I'm pretty sure field names can't (or at least really shouldn't) contain periods, so we can remove that from the allowed character list for fields.

Field names not, but we obviously also want to allow to specify the table in a condition and for that, you need the . :)

$query->condition('table.field');

This caused all kind of funny exceptions because it removed the ....

I like it. Fixes a couple of issues all at once. Rock on!

*Turns to Dries, who is sitting next to him at the CommerceGuys table at a conference.* All yours.

Thanks! Committed to CVS HEAD. Now: lunch time!