This repository was archived by the owner on Aug 13, 2026. It is now read-only.
[security] GHSA-g8pm-xq73-xqq8: Fix blind SQL injection in GDPR search ORDER BY - #1141
Closed
pimcore-deployments wants to merge 1 commit into
Closed
[security] GHSA-g8pm-xq73-xqq8: Fix blind SQL injection in GDPR search ORDER BY#1141pimcore-deployments wants to merge 1 commit into
pimcore-deployments wants to merge 1 commit into
Conversation
…xqq8) Quote the sort identifier before it is placed into the ORDER BY clause of the GDPR data-object and asset search queries. The value comes from the ExtJS `sort` parameter via QueryParams::extractSortingSettings with no allowlist; as an ORDER BY identifier it cannot be bound as a parameter, so it is now quoted with quoteIdentifier() to neutralize injection. Co-Authored-By: Claude <noreply@anthropic.com>
|
Contributor
|
Moved to the enterprise repo: pimcore/ee-admin-ui-classic-bundle#391 (base The commit was cherry-picked unchanged — identical diff, original authorship preserved. Closing here in favour of that PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Vulnerability
Blind SQL injection (CWE-89) in the GDPR search endpoints
GET /admin/gdpr/data-object/search-data-objectsandGET /admin/gdpr/asset/search-assets. An authenticated user with the low-privilegegdpr_data_extractorpermission controls thesort[].propertyvalue, whichQueryParams::extractSortingSettings()returns as-is (only remappingclassname/typetosubtype, with no field allowlist).Root cause: that attacker-controlled property was passed directly into the
ORDER BYclause:in
src/GDPR/DataProvider/DataObjects.phpandsrc/GDPR/DataProvider/Assets.php. Identifiers inORDER BYcannot be bound as parameters, and theWHERE id = :idclause is bound separately, so the sort value is a working injection sink independent of the filter. It escalates to a full read of the database (e.g.userspassword hashes) via an error-basedextractvaluepayload, since MariaDB evaluates theORDER BYexpression even for a single-row result.This is a new, previously unpatched sink, distinct from the already-fixed filter/date SQL-injection paths.
Fix
The sort identifier is now quoted with
\Pimcore\Db::get()->quoteIdentifier()before being placed intoORDER BY, in both data providers:Quoting escapes any embedded backticks and wraps the value as a single identifier, so an injected expression such as
extractvalue(...)or(SELECT 1)becomes an unknown-column reference the database rejects rather than SQL it executes. This matches the existing convention in this repository —QueryParams::getFilterCondition()already usesquoteIdentifier()for dynamic identifiers — and the Pimcore coding guideline that dynamic column names which cannot be bound must be passed throughquoteIdentifier(). The sort direction ($order) was already constrained toASC/DESCbyextractSortingSettings()and is unchanged.Legitimate sorting is preserved: real column names (
id,type,subtype,filename, ...) quote to themselves and sort as before; only values that are not valid identifiers are now rejected.The sibling GDPR provider
PimcoreUsersis not affected: it sorts viaUser\Listing::setOrderKey(), which handles identifier quoting in the listing DAO, not through a raworderBy()call. The otherextractSortingSettings()callers reach the ORM listing layer rather than a hand-built query builder.Backward compatibility
No BC break. Both
DataObjectsandAssetsare annotated@internal, so they are outside the BC promise; the publicsearchData()signature is unchanged, and legitimate column sorts return identical results. The only behavioral change is that malformed/malicious sort identifiers now raise a DB error instead of being executed — the intended security effect.Tests
Tests added:
tests/Model/GDPR/GdprSearchSortInjectionTest.phpThe test exercises both providers. The injection cases pass
(SELECT 1)as the sort property (valid SQL, not a column): against the vulnerable code it is emitted asORDER BY (SELECT 1)and executes without error, so the expected exception is never thrown and the test fails; against the fix it is quoted toORDER BY `(SELECT 1)`and rejected as an unknown column, so the test passes. Two companion cases assert that legitimate sorts (id,filename) still succeed, confirming valid behavior is retained.The tests could not be executed in this environment (no Composer install / DB available in the sandbox); they are written to run under the existing Codeception
Modelsuite, which provides the DB connection the query builder needs.Security-Advisory: pimcore/pimcore/GHSA-g8pm-xq73-xqq8