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
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
Comment | File | Size | Author |
---|---|---|---|
#73 | interdiff.txt | 1.02 KB | dawehner |
#73 | 2492967-73.patch | 13.22 KB | dawehner |
#67 | sql_layer-2492967-67.patch | 12.38 KB | cilefen |
#67 | interdiff-2492967.txt | 2.78 KB | cilefen |
#64 | sql_layer-2492967-64.patch | 12.6 KB | cilefen |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI found a nice meta parent for this issue.
Comment #2
larowlanIn the db layer, there is a chance for each driver to map operations. Presently mysql does nothing. This could be used to sanitize too.
Comment #3
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #4
googletorp CreditAttribution: googletorp at Reveal IT commentedSince it's a sec issue I'm bumping up to critical.
Comment #5
webchickAFAIK this is only security hardening, there's no actual known vulnerability, is that correct? If so, I think this would only be major.
Comment #6
BerdirThat'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 :)
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedCan we update https://www.drupal.org/node/1207020 please with that tag?
It is getting confusing with the sec tags ...
Comment #8
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedTagging to revisit before release candidate, this is not getting much love right now :/ ( I know everyone is busy with other things ... )
Comment #9
gregglesre: #7 I updated that page. Hope it's better :)
Comment #10
webchick@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?
Comment #11
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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?
Comment #12
gregglesI 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.
Comment #14
catch#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.
Comment #15
BerdirReuploading the patch with the syntax error fixed.
Comment #19
webchickActually that seems to be legit:
Let's try this. No credit, please. Interdiff: s/upper/strtoupper/
Comment #20
webchickAhem.
Comment #22
gregglesThanks, 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.
Comment #23
gregglesOK, 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.
Comment #25
mr.baileysThis should take care of the remaining test failure for mysql.
Comment #26
gregglesThanks for picking this up, mr.baileys - tests all pass. Seems great.
As catch said
I think that also makes this a good candidate to go in before the next release (whether beta or RC or...).
Comment #27
catchComment #28
catchAlso needs tests.
Comment #29
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedAs much as I would love to just RTBC this, we need two things here:
Hence CNW.
Comment #30
gregglesregarding #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.
Comment #31
pwolanin CreditAttribution: pwolanin as a volunteer commentedSo, 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.
Comment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #33
catchYes 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.
Comment #34
larowlanWorking on tests
Comment #35
larowlanSo 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.
Comment #36
benjy CreditAttribution: benjy at PreviousNext commentedDoes 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
Comment #39
chx CreditAttribution: chx commentedYou 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.
Comment #40
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI 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?
Comment #41
catchI 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.Comment #42
dawehnerTalked 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)".
Comment #43
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedhttps://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?
Comment #44
pwolanin CreditAttribution: pwolanin as a volunteer commentedIt 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.
Comment #45
catchSo 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?
Comment #46
chx CreditAttribution: chx commentedIf an operator needs a () then mapConditionOperator needs to add it around the argument. Note the substantial difference between
expr IN (value1,value2...
andGREATEST(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.
Comment #47
dawehnerHere is some start for some unit test coverage, feel free to expand it.
Comment #49
hussainwebJust trying to fix the test.
Comment #51
pwolanin CreditAttribution: pwolanin as a volunteer commentedPatch 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.
Comment #52
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedNever 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.
Comment #53
hussainwebI 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?
Comment #54
dawehnerWe could add a @covers, but otherwise the testname IMHO already covers everything
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
Comment #55
dawehnerSmall cleanup of the tests, just comments / reordering.
Comment #56
plachInterdiff looks good, aside from...
... the missing parenthesis.
Comment #57
dawehnerUps.
Comment #58
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThere 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.
Comment #59
bircherI 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.
Comment #60
bircherdidn't mean to do that...
Comment #61
alexpottGiven 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.
Comment #62
dawehnerGreat finding @alexpott!
Two points on top of the one from alex.
Comment #63
Crell CreditAttribution: Crell as a volunteer commentedI 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.
Comment #64
cilefen CreditAttribution: cilefen commentedRe: #63
Comment #65
cilefen CreditAttribution: cilefen commentedOh, the comment will need changing in this paradigm.
Comment #67
cilefen CreditAttribution: cilefen commentedI changed the test to look for the exception.
Comment #68
cilefen CreditAttribution: cilefen commentedComment #72
dawehnerHaha, its not that we haven't thought about it :)
The following code from the database system prevents you from throwing an exception:
... 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.
Comment #73
dawehnerTo 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
Comment #74
Fabianx CreditAttribution: Fabianx as a volunteer commentedI think this is good to go. Thanks @all!
--
Added a proposed commit message:
Comment #75
dawehnerPlease also include crell, given that he made a review with a fair point, which is absolute not obvious and resulted into better documentation.
Comment #76
alexpottCommitted 5d0aa3d and pushed to 8.0.x. Thanks!
Comment #77
cilefen CreditAttribution: cilefen commentedDid this commit get through?
Comment #78
googletorp CreditAttribution: googletorp at Reveal IT commentedAlex forgot to push the commit, putting back to RTBC.
Comment #79
webchickHey, I can do that! :)
Committed and pushed to 8.0.x. Thanks!
Back to 7.x.
Comment #82
kporras07 CreditAttribution: kporras07 at Manatí commentedIs 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,
Comment #83
Fabianx CreditAttribution: Fabianx as a volunteer commented#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.
Comment #86
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #87
hgoto CreditAttribution: hgoto as a volunteer commentedI'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 becauseConditionTest
uses test mocks withPHPUnit_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?)
Comment #93
Fabianx CreditAttribution: Fabianx as a volunteer commentedYes, lets create a new issue for that, please :)
Comment #94
hgoto CreditAttribution: hgoto as a volunteer commented@Fabianx thank you! I created a new one and posted a patch there.
#2802159: [D7] SQL layer: $match_operator is vulnerable to injection attack
Maybe, we can close this D8 issue?
Comment #95
Fabianx CreditAttribution: Fabianx as a volunteer commentedYes, closing. Thank you!
Comment #96
xjmComment #97
MustangGB CreditAttribution: MustangGB commented