[D7] SQL layer: $match_operator is vulnerable to injection attack

Looks great, thanks!

Just wondering, why is this marked as 7.60 target?

Assigning to Fabianx for review

I guess this patch is relatively safe to do - seems like it could only cause problems if people are using the $operator parameter very improperly. So I don't have a strong opinion about doing it in 7.60 vs. a regular bugfix release.

1. +              $this->changed = TRUE;
   +              $this->arguments = array();
   +              // Provide a string which will result into an empty query result.
   +              $this->stringVersion = '( AND 1 = 0 )';
   +
   +              // Conceptually throwing an exception caused by user input is bad
   +              // as you result into a WSOD, which depending on your webserver
   +              // configuration can result into the assumption that your site is
   +              // broken.
   +              // On top of that the database API relies on __toString() which
   +              // does not allow to throw exceptions.
   +              trigger_error('Invalid characters in query operator: ' . $condition['operator'], E_USER_ERROR);
   +              return;
   

   I tested this out and it doesn't seem to be doing what it's intending to do:

   • First, I wound up with a WSOD + error page anyway (with message Recoverable fatal error: Method DatabaseCondition::__toString() must return a string value in SelectQuery->__toString() (line 1553 of /.../drupal/includes/database/select.inc)). I tried doing $this->changed = FALSE and $this->stringVersion = '( 1 = 0 )' instead and then it worked as expected (although the latter probably depends on the exact nature of the query that's running in order to actually work).
   • I think the code comment about not being able to throw exceptions here is wrong. I tried throwing an exception here instead and it seemed to work fine for me.
   • I don't completely agree with the goal of avoiding a WSOD. Well I do in theory, but it's kind of silly because we're only avoiding it in a very narrow case... If someone passes in user input that doesn't contain one of the disallowed characters but still isn't a valid operator (for example, "ELEPHANT") you'll get a WSOD + error page then anyway since the database query will fail.

   So overall this would be a lot simpler if we just threw an exception here - any good reason not to do that? It's probably worth a followup issue to clean this up for Drupal 8 also.

   By the way, in case anyone is wondering (I was) passing the $condition['operator'] to trigger_error() doesn't replace the SQL injection with XSS since Drupal filters the message in https://api.drupal.org/api/drupal/includes%21errors.inc/function/_drupal....

2. +    // Convert errors to exceptions for testing purposes below.
   +    set_error_handler(function ($severity, $message, $filename, $lineno) {
   +      throw new ErrorException($message, 0, $severity, $filename, $lineno);
   +    });
   

   Don't think that would work on PHP 5.2. This may not be necessary anyway if we change it to just throw an exception as suggested above.

3. +    // Attempt SQLi via union query with no unsafe characters.
   +    module_enable(array('user'));
   +    // $this->resetAll();
   

   Not sure what's going on here, but the User module is required so I don't think it needs to be enabled. And the commented out line can be removed too.

4. I just did a retest and it looks like we aren't getting a clean failure with the test-only patch (only CI errors) ... perhaps that is a consequence of some of the above?

Nice spots! I think in D8 we have a workaround for the __toString issue (but I may be wrong), and throwing an exception during __toString() would still give the same issue in D7, but it seems you manually tested that it didn't?

Yes, it seemed to work fine when I tried it quickly. I think maybe what happens is that the exception is getting thrown earlier (before the query object is converted to a string). Not sure if that's a different behavior than Drupal 8.

Looks good, without the patch, the new sql injection test fails.

with the patch, the new sql injection test passes.

Is there a priority higher than Critical? lol !

Since we're probably pretty close to a 7.60 release and there is a bit of risk with this patch, I think we should hold it off for Drupal 7.60 rather than pushing this in now a few days after it was marked RTBC. Will try to review it soon though.

I think there is a problem with the newer approach of checking the operator inside the condition() method. It won't protect against SQL injection in cases where the condition was modified later on (after it was created). An example of code that modifies the conditions later on can be found in TaxonomyTermController::buildQuery(). If that were susceptible to SQL injection (it isn't, but if it were) the patch in #18 wouldn't protect against it.

So I think it is better to go back to the approach in #13.

And nice find (in #15) that this code can be called from within a __toString() method after all. However, it looks like that is not a common scenario (most database queries won't be converted to a string before being compiled) so it would be a shame to lose the nice InvalidQueryConditionOperatorException entirely. I think maybe we can throw the InvalidQueryConditionOperatorException and use it most of the time, but catch it within the __toString() methods and explicitly trigger a fatal error only then?

I have attached a patch that does this. For triggering the fatal error, I borrowed some ideas from #2562487: Get better error reporting from __toString but decided it would be better (both for backwards-compatibility and for making the code a bit more self-documenting) to do it a little differently in Drupal 7. So I introduced a new function drupal_trigger_fatal_error() that can be used to force the fatal error.

Hopefully we can get some reviews of this and get the issue fixed soon.

Note: The interdiff is against #13.

I like the try catch blocks.
This latest patch looks really good as far as the interdiff is concerned.

I applied the patch, it applied cleanly.
cleared caches,
Ran a few manual tests, everything looks good.

Bumping to 7.61. This didn't make it into 7.60.

Reroll of patches from #27

This has already been pretty thoroughly reviewed (and is a backport).

I think this is looking pretty good (still trying to get a clean "green board" for the test runs though).

One thing that put me off a little bit is that the test only patch mostly doesn't fail in quite the way we might expect.

This was pretty much covered in #2492967-35: SQL layer: $match_operator is vulnerable to injection attack where @larowlan mentioned that the DB API won't allow a "little bobby tables"-style SQLi where a whole additional query is injected. So the test which tries to use this as an operator:

$injection = "IS NOT NULL); INSERT INTO {test} (name) VALUES ('test12345678'); -- ";

...fails with an exception like:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You
    have an error in your SQL syntax; check the manual that corresponds to your
    MySQL server version for the right syntax to use near 'INSERT INTO
    `simpletest673306TEST` (NAME) VALUES ('TEST12345678'); --

..even without the actual changes in the patch here. Of course, that's not a bad thing in itself.

Looking more closely at the test, this doesn't matter too much though as we're positively checking for the right exception being thrown once it's been added in the patch:

    catch (InvalidQueryConditionOperatorException $e) {
      $this->pass('SQL injection attempt via condition arguments should result in a database exception.');
    }

So the test passes mean the new check is catching the injection attempts and throwing the new exception.

In other words, I think this LGTM (but I took a little while checking through exactly what's going on in the tests).

There was also one comment in the previous issues though about - perhaps being a valid character in operators in SQLite. I want to have another look at that before committing this.

#2492967-58: SQL layer: $match_operator is vulnerable to injection attack is where SQLite operators are mentioned.

IIUC the proposal was to add -- alongside UNION in the blocklist, and remove the single - from the blocked list of characters.

However, that doesn't seem to have happened and D9 still has a pretty similar implementation to the patch we're considering here:

https://git.drupalcode.org/project/drupal/-/blob/9.3.0-beta3/core/lib/Dr...

It's conceivable that an operator that works (only) in SQLite may be broken by this additional check, but I don't think that's worth holding up this patch. A change to the blocklist(s) would be something to address in D9 and backport.

Adding credit from the original issue.

Thank you!

@mcdruid, thanks, great job on all the past few months especially for php 8.0 support (and 8.1) and previously the great work on the sqlite and MySQL 8 support!