Proposed commit message:

Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx, bircher, mr.baileys, webchick, Berdir, Fabianx, catch, pwolanin: SQL layer: $match_operator is vulnerable to injection attack

Problem/Motivation

#2490420: EntityAutocomplete element settings allows sql injection and for arbitrary user-supplied data to be passed into unserialize() was not only arbitrary code execution via unserialize, but even if it had used JSON it would still have led to a SQL injection attack.

We try to reduce those attacks as much as possible, but the simple code of:

  // [...]
  $match_operator = !empty($_GET['match_operator']) ? $_GET['match_operator'] : 'CONTAINS';
  $entity_labels = $handler->getReferenceableEntities($string, $match_operator, 10);

is making core vulnerable to a SQL injection attack.

It is well known that

a) one should escape user input (a check_plain() would not be enough though)
b) not trust user input (yes, need a white list)
c) use FORM-API with an options element (views exposed filters)

etc. as best practice, but it has been common security hardening in Drupal 8 to make such attacks as hard as possible.

And in this case nothing in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... tells that $match_operator MUST not be user input.

So it is not even that hard to make that mistake, when copying e.g. from core's autocomplete handler / controller ...

Proposed resolution

- Harden the SQL layer some more

There is several possibilities:

a) preg_replace('\W', '', $op)
b) a white list of known operators

Both have the problem that the list of operators is dynamic, e.g. there is "BETWEEN SYMMETRIC" in POSTGRESQL and whitespace is even used within other valid database operators.

A possibility how to solve that would be to introduce a:

SafeDatabaseOperator('BETWEEN SYMMETRIC') class and allow an object instead of just a string to escape the escaping / limited white list of most common operators.

That would be a backwards compatible API change though.

Could throw a helpful Exception for the operators whitelist case however to point to the docs how to get arbitrary operators working.

Also could of course use a variable / setting / ... to override list of allowed operators.

Tagging for potential backport to D7.

Remaining tasks

- Discuss

User interface changes

- None

API changes

- Maybe allow $match_operator to be string|object
- Maybe restrict list of operators

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Critical because security

If you are coming here because you need to add more operators

  • Look at the issues related to this issue to see if any are for the operator you need - if they are, test that patch and contribute to the work there. If it doesn't exist...
  • Create a new issue about the operator you need AND make that issue related to this issue
  • Patch your site to add the operators that you need
  • Post the patch to your new issue
CommentFileSizeAuthor
#87 2492967-87-D7-test_only_should_fail.patch3.72 KBhgoto
#87 2492967-87-D7.patch5.4 KBhgoto
#73 interdiff.txt1.02 KBdawehner
#73 2492967-73.patch13.22 KBdawehner
#67 sql_layer-2492967-67.patch12.38 KBcilefen
#67 interdiff-2492967.txt2.78 KBcilefen
#64 sql_layer-2492967-64.patch12.6 KBcilefen
#64 interdiff-2492967.txt1.06 KBcilefen
#62 interdiff.txt1.64 KBdawehner
#62 2492967-62.patch12.83 KBdawehner
#59 interdiff-2492967-57-59.txt1.84 KBbircher
#59 2492967-59.patch12.75 KBbircher
#57 interdiff.txt740 bytesdawehner
#57 2492967-57.patch12.7 KBdawehner
#55 2492967-55.patch12.7 KBdawehner
#55 interdiff.txt6.33 KBdawehner
#49 2492967-49.patch11.98 KBhussainweb
#49 interdiff-47-49.txt443 byteshussainweb
#47 interdiff.txt7.07 KBdawehner
#47 2492967-47.patch11.95 KBdawehner
#42 interdiff.txt3.45 KBdawehner
#42 2492967-42.patch5.49 KBdawehner
#39 interdiff.txt1.27 KBchx
#39 2492967_39.patch4.74 KBchx
#35 sql-match-2492967.pass_.patch5.07 KBlarowlan
#35 sql-match-2492967.fail_.patch3.94 KBlarowlan
#25 2492967_sanitize_sql_operators_25.patch1.73 KBmr.baileys
#25 2492967-23-25-interdiff.txt1.28 KBmr.baileys
#23 2492967_sanitize_sql_operators_23.patch1.3 KBgreggles
#19 2492967_sanitize_sql_operators.patch1.21 KBwebchick
#15 2492967_sanitize_sql_operators.patch1.2 KBBerdir
#12 2492967_sanitize_sql_operators.patch1.2 KBgreggles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

I found a nice meta parent for this issue.

larowlan’s picture

In the db layer, there is a chance for each driver to map operations. Presently mysql does nothing. This could be used to sanitize too.

googletorp’s picture

Priority: Major » Critical
Issue summary: View changes
googletorp’s picture

Since it's a sec issue I'm bumping up to critical.

webchick’s picture

AFAIK this is only security hardening, there's no actual known vulnerability, is that correct? If so, I think this would only be major.

Berdir’s picture

Priority: Critical » Major
Issue tags: -Security +Security improvements

That's correct.

It's also nothing new in 8.x, the exact same behavior exists in 7.x. I think the tag for this is Security improvements :)

Fabianx’s picture

Can we update https://www.drupal.org/node/1207020 please with that tag?

It is getting confusing with the sec tags ...

Fabianx’s picture

Issue tags: +revisit before release candidate

Tagging to revisit before release candidate, this is not getting much love right now :/ ( I know everyone is busy with other things ... )

greggles’s picture

re: #7 I updated that page. Hope it's better :)

webchick’s picture

@Fabianx: Can you clarify why this is one of the issues you want to block D8's release until it's revisited? To me this looks like straight security hardening that can happen at any time, including after D8.0.0's release. What am I missing?

Fabianx’s picture

#10: The fact that I would have very likely made the same mistake as the original issue, which my gut feeling tells me could mean that contrib could run into this as well.

The problem is that we have hidden the DB layer too good by now, but still pass through things. If I am doing a db_select() I know I deal with the database, if I use an EntityFieldQuery I know it _could_ at least be passed through to the database.

If 'revisit before release candidate' is not good, is there another tag for 'should really really really get in, but ultimatively not block release', yet?

greggles’s picture

Status: Active » Needs review
FileSize
1.2 KB

I really agree with Fabianx's perspective here. This kind of hardening is likely to save us from dozens of sqli issues. That feels worth making it a major to revisit before RC.

Here's a first-pass based on larowlan's idea in #2. I did some grep/awk to find operators in core, but I'm sure I missed many.

Status: Needs review » Needs work

The last submitted patch, 12: 2492967_sanitize_sql_operators.patch, failed testing.

catch’s picture

#12 looks like a decent way to handle this.

The big concern about #2490420: EntityAutocomplete element settings allows sql injection and for arbitrary user-supplied data to be passed into unserialize() is that it started out as an unserialize() user input/code injection bug.

Then a patch was put up to fix that bug by switching to json_encode()/decode() - and was RTBCed.

Then I CNW-ed it for test coverage.

Then only later did anyone spot the SQL injection vulnerability - which had been there all along and was not fixed by the 'fix'.

This shows just how easy it is to 1. introduce SQL injection like this 2. Not spot it once it's there.

As others have said we're multiple levels removed from SQL, and we're used to db_select() and EntityFieldQuery completely protecting from SQL injection with values, field and table names - just not conditions.

Also when larowlan asked about SQL injection, both he and I had to manually read through the database layer to double check that the condition operator really isn't escaped. There's no explicit documentation about whether it is or not - we have Connection::escapeTable() which sometimes people fail to use when building dynamic queries with string concat (which is discouraged anyway) - but you can see that being used by SelectQuery etc. so at least there's an API for it.

There's absolutely zero way ensure a condition operator is safe in core - you can only do so by making your own whitelist of operators in the custom code building the query based on what the expected user input is.

There's also a risk of breaking custom or contrib code here if it's using unusual operators (or we forget a common one that's not used in core) - so I think this could only go into a minor release, not a patch release.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Reuploading the patch with the syntax error fixed.

Status: Needs review » Needs work

The last submitted patch, 15: 2492967_sanitize_sql_operators.patch, failed testing.

Status: Needs work » Needs review

webchick queued 15: 2492967_sanitize_sql_operators.patch for re-testing.

YOUR MOM failed during invocation of run-tests.sh --clean. :P

Status: Needs review » Needs work

The last submitted patch, 15: 2492967_sanitize_sql_operators.patch, failed testing.

webchick’s picture

Actually that seems to be legit:

$ drush si --account-pass=admin -y
You are about to DROP all tables in your '8x' database. Do you want to continue? (y/n): y
Fatal error: Call to undefined function Drupal\Core\Database\Driver\mysql\upper() in /Users/webchick/Sites/8.x/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php on line 190
Drush command terminated abnormally due to an unrecoverable error.   [error]
Error: Call to undefined function
Drupal\Core\Database\Driver\mysql\upper() in
/Users/webchick/Sites/8.x/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php,
line 190

Let's try this. No credit, please. Interdiff: s/upper/strtoupper/

webchick’s picture

Status: Needs work » Needs review

Ahem.

Status: Needs review » Needs work

The last submitted patch, 19: 2492967_sanitize_sql_operators.patch, failed testing.

greggles’s picture

Thanks, that'll teach me to write patches outside my IDE :)

So, having just 3 remaining fails indicates to me that this is a pretty legit approach.

Possible improvements:
* moving the switch out of this function so that it can be used for other purposes (e.g. input validation).
* A better documentation page to explain what to do.
* Support for other db drivers.
* A variable based system to whitelist additional operators - feels just as secure and will make adding operators a bit more flexible.

greggles’s picture

Status: Needs work » Needs review
FileSize
1.3 KB

OK, here's an updated patch. I looked at the three failing tests to try to find the conditions they used and added them.

I still think this needs some work as described in #22, but want to see how feasible the approach is.

Status: Needs review » Needs work

The last submitted patch, 23: 2492967_sanitize_sql_operators_23.patch, failed testing.

mr.baileys’s picture

This should take care of the remaining test failure for mysql.

greggles’s picture

Thanks for picking this up, mr.baileys - tests all pass. Seems great.

As catch said

There's also a risk of breaking custom or contrib code here if it's using unusual operators (or we forget a common one that's not used in core) - so I think this could only go into a minor release, not a patch release.

I think that also makes this a good candidate to go in before the next release (whether beta or RC or...).

catch’s picture

Issue tags: -revisit before release candidate +rc target, +API change
catch’s picture

Issue tags: +Needs tests

Also needs tests.

Fabianx’s picture

Status: Needs review » Needs work
Issue tags:

As much as I would love to just RTBC this, we need two things here:

  • a) Tests that SQL injection is protected against properly (I suggest to just use some higher level up API)
  • b) A way to extend the list of operators, maybe a simple $settings['white_list_operators']['FOO'] = TRUE and then an isset() check? - Currently the Exception links to this issue and this issue has no docs on this, but a patch, which links to this issue, which has no ... INFINITE LOOP DETECTED!

Hence CNW.

greggles’s picture

Issue summary: View changes

regarding #29.b - I just updated the issue summary to explain what to do. I think this is a reasonable request of site builders and a better solution than a $settings type solution.

pwolanin’s picture

So, it seems like the patch needs work since it should extend the same protection to each driver?

Possibly we should make the default implementation of this live in the base, abstract class?

I also think there are approaches other than patching core to change the list if needed - you can extend and override the core class & method.

effulgentsia’s picture

Priority: Major » Critical
Issue tags: -rc target

I discussed this with @catch, @webchick, @alexpott, and @xjm on a triage call of "revisit before release candidate" issues (see #27). It took some followup discussion since then to decide if we should promote this to Critical, and we decided on yes, because even though it's "only" a hardening, we feel it's a very important one due to there being insufficient docs on what kind of validation is expected to be the caller's responsibility, and getting it wrong can lead to SQL injection. We think it should also be Critical when moved to the 7.x queue for backport, but leave that decision to David_Rothstein when the time comes.

catch’s picture

Yes I think given the condition method has three parameters, two of which are escaped (column, value) and one that has no sanitization at all (operator), it's quite confusing whether user input for $operator is safe or not.

After #2490420: EntityAutocomplete element settings allows sql injection and for arbitrary user-supplied data to be passed into unserialize() I had to follow the code all the way down to convince myself there really was no protection, and Heine was the only person on that issue that spotted the SQL injection at all (i.e. it had been RTBC with the json encode and I only knocked it back for tests).

pwolanin also pointed out we did #829464: orderby() should verify direction [DONE] and escape fields for orderBy() which is very similar.

I like greggles' idea from the issue summary.

We'd have the preg for raw strings.

Then an Operator class + interface which we do an instanceof check for and pull the actual string out from that for anything unusual.

larowlan’s picture

Assigned: Unassigned » larowlan

Working on tests

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests, -API change
FileSize
3.94 KB
5.07 KB

So I can't get the test to fail with an insert query because of #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it

And I can't get it to fail with a union query because we already uppercase operators in Condition::mapConditionOperator

But I can get it to fail with an uppercase table name, so those are vulnerable.

But based on this being fairly uncommon in Drupal, I'm not sure this is critical - #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it shut the door on nasty stuff and the Condition::mapConditionOperator upper-casing shut the door on most Union queries.

Fail patch shows uppercase table names vulnerable.

Regex approach based on discussion with @chx.

This will cause issues if someone has defined a stored function/procedure outside that character range, in which case they'll need a custom driver. I think that is an edge-case we shouldn't worry about.

benjy’s picture

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -188,6 +188,12 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
+            $condition['operator'] = preg_replace('/[^0-9a-zA-Z _=<>!]/', '', $condition['operator']);

Does this cover all available MySQL operators, I see additional operators in the docs but i'm not sure if they apply to our notion of operators 1 to 1.

https://dev.mysql.com/doc/refman/5.0/en/non-typed-operators.html

The last submitted patch, 35: sql-match-2492967.fail_.patch, failed testing.

The last submitted patch, 35: sql-match-2492967.fail_.patch, failed testing.

chx’s picture

You need to look at https://dev.mysql.com/doc/refman/5.0/en/comparison-operators.html but I have a perhaps simpler approach: nuke a few dangerous characters and be done.

Fabianx’s picture

I like #39 a lot and can't see how you could inject things anymore with all those things replaced, but should we not also replace ';' in case that other protection is not there?

catch’s picture

I like #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it as hardening but I'd also really like to not rely on that behaviour anywhere in core.

So #39 + Fabian's suggestion in #50 of also replacing ; looks great.

dawehner’s picture

Talked with catch about this issue. Its kinda problematic that we still execute the query, we should stop doing that, because we currently rely on the fact that this results in an invalid query. This is also what the test coverage deals with.

Instead trigger an error for now (we could also go with an exception but the DB layer is problematic, just say __toString()), and ensure
that the SQL query is always empty "(1 = 0)".

Fabianx’s picture

https://dev.mysql.com/doc/refman/5.0/en/comparison-operators.html has several operators that have () - unless I am missing something.

Can we have a unit test to ensure all known operators continue to work at least?

pwolanin’s picture

It seems like in part this protects because the condition class wraps in it in parentheses? So an injection needs to close that paren, or not?

Is there a risk that that could change, or maybe we are missing other ways to construct a UNION query?

At the least, we should probably specifically block the UNION keyword here too.

catch’s picture

https://dev.mysql.com/doc/refman/5.0/en/comparison-operators.html has several operators that have () - unless I am missing something.

So in that list is IN() and NOT IN() - for a condition we'd do ->condition('foo', ['some', 'stuff'], 'NOT IN') so no parenthesis in the operator we actually pass as an argument.

But yes it'd be worth adding tests for any common operators we don't actually use in core, or at least some documentation as to why it's not a problem.

I don't think something like STRCMP() would work either before or after this patch - you'd need to use an expression anyway?

chx’s picture

If an operator needs a () then mapConditionOperator needs to add it around the argument. Note the substantial difference between expr IN (value1,value2... and GREATEST(value1, value2 -- the Condition class only supports the former, where the fieldname is before the operator. For functions (the latter kind) you need an expression.

Also, you can't cheat by adding the ( yourself as there's no support for the closing ):

$condition_fragments[] = ' (' . $connection->escapeField($condition['field']) . ' ' . $operator['operator'] . ' ' . $operator['prefix'] . implode($operator['delimiter'], $placeholders) . $operator['postfix'] . ') ';

There's no way to add a ) from the operator.

So there's no need or support for () in the operator.

dawehner’s picture

Here is some start for some unit test coverage, feel free to expand it.

Status: Needs review » Needs work

The last submitted patch, 47: 2492967-47.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
443 bytes
11.98 KB

Just trying to fix the test.

The last submitted patch, 47: 2492967-47.patch, failed testing.

pwolanin’s picture

Patch seems ok, but maybe I'm missing something.

Looks like:
+ public function testCompileWithSqlInjectionForOperator($operator)

Is doing nothing except testing mocks? I don't think that adds any value.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Never mind - re-reading, I see we need the mocks to pass into compile().

I think this change is sufficient to protect contrib since it seems infeasible to white-list every possible operator for any possible back-end.

hussainweb’s picture

I was just trying to fix the test. I think we still need to give a proper comment for the new test case. It could be helpful later. Can it be done on commit?

dawehner’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
    @@ -0,0 +1,146 @@
    +  /**
    +   * @covers ::compile
    +   */
    +  public function testSimpleCondition() {
    ...
    +  /**
    +   * @dataProvider dataProviderTestCompileWithKnownOperators()
    +   */
    +  public function testCompileWithKnownOperators($expected, $field, $value, $operator, $expected_arguments = NULL) {
    

    We could add a @covers, but otherwise the testname IMHO already covers everything

  2. +++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
    @@ -0,0 +1,146 @@
    +  public function dataProviderTestCompileWithKnownOperators() {
    

    Here we could also comment that the commented out ones don't work at the moment if you just use them, but this doesn't have to be done now

dawehner’s picture

Small cleanup of the tests, just comments / reordering.

plach’s picture

Interdiff looks good, aside from...

+++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
@@ -48,36 +48,21 @@ public function testSimpleCondition() {
+   *   (optional The expected set arguments.

... the missing parenthesis.

dawehner’s picture

Ups.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -188,6 +188,19 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
+            if (preg_match('/[-\'"();]/', $condition['operator'], $match)) {

+++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
@@ -0,0 +1,167 @@
+    $data[] = ["IS NOT NULL) UNION ALL SELECT name, pass FROM {users_field_data} -- "];
+    $data[] = ["IS NOT NULL) UNION ALL SELECT name FROM {TEST_UPPERCASE} -- "];

There might be queries where -- is not needed, e.g.

SELECT * FROM foo WHERE x $operator 1

could still be transformed to

SELECT * FROM foo WHERE x = 1 UNION ALL SELECT PASS FROM user WHERE uid = 1

with the attack of:

= 1 UNION ALL SELECT password FROM user WHERE uid =

which would fail the test.

So two things I would think would be good:

a) blacklist 'UNION' as proposed by pwolanin it is not a valid operator in any DB we know

b) I would still more favor a whitelist reg-exp:

My proposal would be:

Only allow [A-Z] space =,!,<,> as characters.

Hmmm, https://www.sqlite.org/lang_expr.html

shows way more, e.g. '-' as valid, which we blacklist, so we should probably blacklist '--' instead.

So maybe we keep the blacklist but first use a fixed keyword list, e.g.

['UNION', '--']

and then a list of dangerous characters.

bircher’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
12.75 KB
1.84 KB

I just tried it out and looked at the other lines in around the lines where we introduce the check for unsafe characters. It turns out that at the end of the else block where we do that the operator always gets wrapped in '(' ... ')' so if we filter for parentheses we would ensure at least a broken query.

But I added a patch to also stop a UNION operator.

bircher’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

didn't mean to do that...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -188,6 +188,19 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
+            if (stripos($condition['operator'], 'UNION') || preg_match('/[-\'"();]/', $condition['operator'], $match)) {

+++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
@@ -0,0 +1,167 @@
+    $data[] = ["IS NOT NULL) ;INSERT INTO {test} (name) VALUES ('test12345678'); -- "];

Given this is a very low level API I think it is worth doing mirco-optimisations. We can use strpbrk() instead of preg_match(). Local testing showed it to be about 300 times quicker.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.83 KB
1.64 KB

Great finding @alexpott!

Two points on top of the one from alex.

  • We need to use !== FALSE otherwise using "UNION" as first bit of the operator would be fine.
  • Ensured that it actually works by expanding the test coverage.
Crell’s picture

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -188,6 +188,19 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
+            // Remove potentially dangerous characters.
+            // If something passed in an invalid character stop early, so we
+            // don't rely on a broken SQL statement when we would just replace
+            // those characters.
+            if (stripos($condition['operator'], 'UNION') !== FALSE || strpbrk($condition['operator'], '[-\'"();') !== FALSE) {
+              $this->changed = TRUE;
+              $this->arguments = [];
+              // Provide a string which will result into an empty query result.
+              $this->stringVersion = '( AND 1 = 0 )';
+
+              trigger_error('Invalid characters in query operator: ' . $condition['operator'], E_USER_ERROR);
+              return;
+            }

I don't think we do this anywhere else in the DB layer. Why trigger_error()? The rest of the DB layer throws exceptions for invalid queries; folding to some no-op is not something we should be introducing. If the query is invalid, just throw an exception.

cilefen’s picture

cilefen’s picture

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -188,6 +188,13 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
+            // Remove potentially dangerous characters.
+            // If something passed in an invalid character stop early, so we
+            // don't rely on a broken SQL statement when we would just replace
+            // those characters.
+            if (stripos($condition['operator'], 'UNION') !== FALSE || strpbrk($condition['operator'], '[-\'"();') !== FALSE) {
+              throw new InvalidQueryException('Invalid characters in query operator: ' . $condition['operator']);
+            }

Oh, the comment will need changing in this paradigm.

Status: Needs review » Needs work

The last submitted patch, 64: sql_layer-2492967-64.patch, failed testing.

cilefen’s picture

I changed the test to look for the exception.

cilefen’s picture

Status: Needs work » Needs review

The last submitted patch, 64: sql_layer-2492967-64.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 67: sql_layer-2492967-67.patch, failed testing.

The last submitted patch, 67: sql_layer-2492967-67.patch, failed testing.

dawehner’s picture

Haha, its not that we haven't thought about it :)

The following code from the database system prevents you from throwing an exception:


  /**
   * {@inheritdoc}
   */
  public function execute() {
    // If validation fails, simply return NULL.
    // Note that validation routines in preExecute() may throw exceptions instead.
    if (!$this->preExecute()) {
      return NULL;
    }

    $args = $this->getArguments();
    return $this->connection->query((string) $this, $args, $this->queryOptions);
  }

... you cannot throw an exception in __toString(), well we could catch it in all of __toString() in the database system and convert it there into an error.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.22 KB
1.02 KB

To be clear, there is another point. This is coming from user input, so if you can trigger exceptions with user input, you have problems,
given that some webservers/proxy servers then assume that your site is down

Fabianx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I think this is good to go. Thanks @all!

--

Added a proposed commit message:

Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx, bircher, mr.baileys, webchick, Berdir, Fabianx, catch, pwolanin: SQL layer: $match_operator is vulnerable to injection attack
dawehner’s picture

Please also include crell, given that he made a review with a fair point, which is absolute not obvious and resulted into better documentation.

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 5d0aa3d and pushed to 8.0.x. Thanks!

cilefen’s picture

Did this commit get through?

googletorp’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community

Alex forgot to push the commit, putting back to RTBC.

webchick’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Hey, I can do that! :)

Committed and pushed to 8.0.x. Thanks!

Back to 7.x.

  • webchick committed 5a46f6a on 8.0.x
    Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx...

  • webchick committed 5a46f6a on 8.1.x
    Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx...
kporras07’s picture

Is this still relevant for D7? If yes, could you point me to where to look? I'd like to help in this issue, but I can't find how to reproduce it or where to find the code to be updated.

Thanks in advance,

Fabianx’s picture

#82: Yes it is relevant to D7.

https://api.drupal.org/api/drupal/includes!database!database.inc/functio... and its sub implementations is probably the code to look at.

Just need to ensure the same filtering is in place.

  • webchick committed 5a46f6a on 8.3.x
    Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx...

  • webchick committed 5a46f6a on 8.3.x
    Issue #2492967 by dawehner, cilefen, greggles, larowlan, hussainweb, chx...
Fabianx’s picture

Issue tags: +Drupal 7.60 target
hgoto’s picture

I'd like to go ahead on backport of this patch.

I recognize that there are 5 classes/functions changed in the patch #73 for D8.

  • Drupal\Core\Database\Query\Condition
  • Drupal\system\Tests\Database\DatabaseWebTestBase
  • Drupal\system\Tests\Database\QueryTest
  • Drupal\Tests\Core\Database\ConditionTest
  • database_test_schema()

If I understand it correctly, We can backport four among them. The exception is ConditionTest. It's difficult to backport it because ConditionTest uses test mocks with PHPUnit_Framework_TestCase::prophesize() (Prophecy\Prophecy\ObjectProphecy) and D7 Simpletest doesn't have such functionality, as far as I know. (I may be wrong... If I'm wrong, please tell me.)

A mapping of matching classes/functions between D8 and D7 is as following.

D8 -> D7:

  • Drupal\Core\Database\Query\Condition -> DatabaseCondition
  • Drupal\system\Tests\Database\DatabaseWebTestBase -> DatabaseTestCase
  • Drupal\system\Tests\Database\QueryTest -> DatabaseQueryTestCase
  • Drupal\Tests\Core\Database\ConditionTest
  • database_test_schema() -> database_test_schema()

Under this understanding, I created a patch for D7. I'd like this to be reviewed.

(Should we create a new issue following the latest backport policy before going ahead?)

The last submitted patch, 87: 2492967-87-D7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 87: 2492967-87-D7-test_only_should_fail.patch, failed testing.

The last submitted patch, 87: 2492967-87-D7-test_only_should_fail.patch, failed testing.

The last submitted patch, 87: 2492967-87-D7-test_only_should_fail.patch, failed testing.

The last submitted patch, 87: 2492967-87-D7-test_only_should_fail.patch, failed testing.

Fabianx’s picture

Status: Needs work » Patch (to be ported)

Yes, lets create a new issue for that, please :)

hgoto’s picture

Fabianx’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Patch (to be ported) » Fixed

Yes, closing. Thank you!

xjm’s picture

Status: Fixed » Closed (fixed)
MustangGB’s picture

Issue tags: -Drupal 7.60 target