This was originally reported as a private security issue, but has been approved for handling in the public queue by the Drupal Security Team as "it is a known issue in public since 2015 that the $operator parameter to ->condition() is a potential sink from this issue".
Background information
- security.drupal.org private issue: https://git.drupalcode.org/security/185198-drupal-security/-/work_items/1 (included for reference. Please do not report access denied as an error.)
Problem/Motivation
The vulnerability is in web/core/modules/pgsql/src/EntityQuery/Condition.php line 18.
$condition['operator'] is raw string-concatenated into SQL. ConditionBase::condition() stores the operator with no validation. The base Sql\Condition::translateCondition() returns early for array values (line 115-116 of the base class), so no validation occurs upstream either. The field is properly escaped via escapeField() and values are parameterized, but the operator is unguarded.
Affected
PostgreSQL only. Requires a caller that passes untrusted input as the operator argument to an entity query condition on a case-insensitive field with an array value.
Not directly triggerable from Drupal core alone, but exploitable via contrib modules that expose this pattern.
Steps to reproduce
Described in the original private issue.
Proposed resolution
Add an operator whitelist before the SQL construction:
if (!in_array($condition['operator'], ['IN', 'NOT IN'], TRUE)) {
throw new \InvalidArgumentException(sprintf('Invalid operator "%s" for a case-insensitive array condition.', $condition['operator']));
}
Remaining tasks
- Provide a merge request with the fix and tests
User interface changes
none
Introduced terminology
none
API changes
none
Data model changes
none
Release notes snippet
The PostgreSQL entity query condition operator is now validated against an allowlist to prevent potential SQL injection.
Credit
Issue fork drupal-3605792
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3605792-backport
changes, plain diff MR !16680
- 3605792-sql-injection-via
changes, plain diff MR !16211
Comments
Comment #2
macsim commentedComment #3
quietone commentedComment #5
macsim commentedTests failures aren't related to the MR changes - see #3608308: Composer 2.10 causing Drupal recipe unpacking tests to fail
Comment #6
smustgrave commentedCredited those names mentioned in the summary.
Have to run the test-only locally because of my gitlab role.
2/3 of pass without the change which I'd expect looking at it but they're unit tests so no concern there (not like if they were functional). The 1 test that fails testInvalidOperatorThrowsException
.
The change in
core/modules/pgsql/src/EntityQuery/Condition.phpmakes sense based on the summary, arguably don't run postgresql on anything but defensive code makes sense visually.LGTM.
Comment #11
catchCommitted/pushed to main, 11.x, 11.4.x and 10.6.x, thanks!
Comment #14
larowlanReverted from 10.6.x - can we get a separate MR for 10.6.x? Thanks
https://git.drupalcode.org/project/drupal/-/pipelines/887455 shows the fails from 10.6.x HEAD
Comment #17
smustgrave commentedSince it's been 4 weeks went ahead and did the backport.
Moving to RTBC as it applied cleanly just needed to convert attributes to annotations.
Comment #18
ghost of drupal pastthis is a bc break and as such it can't be backported and also where's the change notice: there was absolutely nothing before that enforced IN. Less charitably, this is a regression which needs to be rolled back and fixed by escaping instead of restricting to IN.
Comment #19
macsim commentedOn the BC break and "escape instead of restrict":
The suggestion to escape the operator rather than whitelist it isn't feasible for SQL operators. Unlike string values, SQL operators cannot be parameterized, they must appear literally in the query string. There is no equivalent of
$statement->bindValue()for an operator position; a whitelist is the only approach that is actually secure.More importantly, before this patch, passing any operator other than
INorNOT INin this code path (array value + case_sensitive = FALSE on PostgreSQL) would already have produced invalid SQL — something likeLOWER(field) LIKE (LOWER(:val1), LOWER(:val2))— causing a database-level error. There is no valid, working use case that this change breaks; it only changes where the failure is surfaced (PHP exception vs. DB error).On backporting security fixes:
Drupal's security policy explicitly allows SA fixes to contain BC breaks when there is no safe alternative. A SQL injection vulnerability falls squarely in that category.
On the change notice:
That's a fair point — We should open one documenting that case-insensitive array conditions on PostgreSQL now require
INorNOT IN, and that any other operator will throw an\InvalidArgumentException.Comment #20
macsim commentedAdded a draft CR: https://www.drupal.org/node/3616349
Comment #21
ghost of drupal pastANY/SOME?
sure, then validate against letters and space.
Comment #22
macsim commentedANY/SOMEare not operators in the sense of$condition['operator']— in PostgreSQL they are quantifiers used alongside a comparison operator (e.g.,field = ANY(array_expression)where=is the operator). This code builds afield OPERATOR (val1, val2, ...)pattern, for whichINandNOT INare the only syntactically valid operators — not an arbitrary choice.A looser check would allow operators like
BETWEENorLIKEthat are structurally incompatible with this SQL pattern and would still cause a database error. The whitelist isn't restrictive, it's exact.Comment #24
longwaveRegarding the backport I agree with all points made by @macsim in #19.
Committed and pushed d7b083a6329 to 10.6.x. Thanks!
Also published the change record.