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

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

Command icon 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:

Comments

macsim created an issue. See original summary.

macsim’s picture

Issue tags: +Security
quietone’s picture

Title: [pgsql] SQL injection via unvalidated operator in case-insensitive entity query array conditions » SQL injection via unvalidated operator in case-insensitive entity query array conditions

macsim’s picture

Status: Active » Needs review

Tests failures aren't related to the MR changes - see #3608308: Composer 2.10 causing Drupal recipe unpacking tests to fail

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative, +Needs Review Queue Initiative

Credited 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

Failed asserting that exception of type "InvalidArgumentException" is thrown.

.

The change in core/modules/pgsql/src/EntityQuery/Condition.php makes sense based on the summary, arguably don't run postgresql on anything but defensive code makes sense visually.

LGTM.

  • catch committed 58c61cab on 10.6.x
    fix: #3605792 SQL injection via unvalidated operator in case-insensitive...

  • catch committed 132f412e on 11.4.x
    fix: #3605792 SQL injection via unvalidated operator in case-insensitive...

  • catch committed 1ae9d974 on 11.x
    fix: #3605792 SQL injection via unvalidated operator in case-insensitive...

  • catch committed f8c93a2f on main
    fix: #3605792 SQL injection via unvalidated operator in case-insensitive...
catch’s picture

Version: main » 10.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, 11.x, 11.4.x and 10.6.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

larowlan’s picture

Status: Fixed » Patch (to be ported)

Reverted 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

  • larowlan committed 155c2f76 on 10.6.x
    Revert "fix: #3605792 SQL injection via unvalidated operator in case-...