Skip to content

fix: prevent SQL injection bypass in escapeIdentifiers and protectIdentifiers #10283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gr8man wants to merge 1 commit into
base: develop
Choose a base branch
from
+88 −18
Open

fix: prevent SQL injection bypass in escapeIdentifiers and protectIdentifiers #10283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions system/Database/BaseConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,8 +1281,9 @@ public function protectIdentifiers($item, bool $prefixSingle = false, ?bool $pro
// Added exception for single quotes as well, we don't want to alter
// literal strings.
if (strcspn($item, "()'") !== strlen($item)) {
/** @psalm-suppress NoValue I don't know why ERROR. */
return $item;
if ($this->isSafeToBypassEscape($item)) {
return $item;
}
}

// Do not protect identifiers and do not prefix, no swap prefix, there is nothing to do
Expand Down Expand Up @@ -1433,6 +1434,34 @@ public function escapeIdentifier($item): string
. $this->escapeChar;
}

/**
* Checks if an identifier with parentheses or quotes is a safe function call or a string literal.
*/
private function isSafeToBypassEscape(string $item): bool
{
$item = trim($item);

// String literals starting and ending with single quotes
if ($item[0] === "'" && preg_match('/^\'(?:[^\']|\'\')*\'$/s', $item)) {
return true;
}

// String literals (for PostgreSQL it can be double quotes)
if ($this->escapeChar !== '"' && $item[0] === '"' && preg_match('/^"(?:[^"]|"")*"$/s', $item)) {
return true;
}

// SQL functions or subqueries (e.g. MAX(id), (SELECT ...)) with an optional alias
if (str_contains($item, '(')) {
// Regex matching balanced parentheses (from start to end or with a safe alias)
if (preg_match('/^(?:[a-zA-Z0-9_.]+\s*)?(?P<parens>\((?:[^()]+|(?&parens))*\))(?:\s+(?:AS\s+)?(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item)) {
return true;
}
}

return false;
}

/**
* Returns escaped table name with alias.
*/
Expand Down Expand Up @@ -1467,11 +1496,7 @@ public function escapeIdentifiers($item)
return $item;
}

// Avoid breaking functions and literal values inside queries
if (ctype_digit($item)
|| $item[0] === "'"
|| ($this->escapeChar !== '"' && $item[0] === '"')
|| str_contains($item, '(')) {
if (ctype_digit($item) || $this->isSafeToBypassEscape($item)) {
return $item;
}

Expand Down
67 changes: 56 additions & 11 deletions tests/system/Database/BaseConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,12 +367,16 @@ public static function provideProtectIdentifiers(): iterable
'table.column' => [false, true, true, 'users.id', '"test_users"."id"'], // Prefixed because it has segments
'table.column prefix' => [true, true, true, 'users.id', '"test_users"."id"'],
'table.column AS' => [
false, true, true,
false,
true,
true,
'users.id AS user_id',
'"test_users"."id" AS "user_id"', // Prefixed because it has segments
],
'table.column AS prefix' => [
true, true, true,
true,
true,
true,
'users.id AS user_id',
'"test_users"."id" AS "user_id"',
],
Expand All @@ -384,42 +388,78 @@ public static function provideProtectIdentifiers(): iterable
'function column' => [false, true, true, 'SUM(id)', 'SUM(id)'],

'function column AS' => [
false, true, true,
false,
true,
true,
'COUNT(payments) AS myAlias',
'COUNT(payments) AS myAlias',
],
'function column AS prefix' => [
true, true, true,
true,
true,
true,
'COUNT(payments) AS myAlias',
'COUNT(payments) AS myAlias',
],

'function quoted table column AS' => [
false, true, true,
false,
true,
true,
'MAX("db"."payments") AS "payments"',
'MAX("db"."payments") AS "payments"',
],

'quoted column operator AS' => [
false, true, true,
false,
true,
true,
'"numericValue1" + "numericValue2" AS "numericResult"',
'"numericValue1"" + ""numericValue2" AS "numericResult"', // Cannot process correctly
],
'quoted column operator AS no-protect' => [
false, false, true,
false,
false,
true,
'"numericValue1" + "numericValue2" AS "numericResult"',
'"numericValue1" + "numericValue2" AS "numericResult"',
],

'sub query' => [
false, true, true,
'(SELECT SUM(payments.amount) FROM payments WHERE payments.invoice_id=4) AS amount_paid)',
false,
true,
true,
'(SELECT SUM(payments.amount) FROM payments WHERE payments.invoice_id=4) AS amount_paid)',
'(SELECT SUM(payments.test_amount) FROM payments WHERE payments.invoice_id=4) AS "amount_paid)"',
],
'sub query with missing `)` at the end' => [
false, true, true,
'(SELECT MAX(advance_amount) FROM "orders" WHERE "id" > 2',
false,
true,
true,
'(SELECT MAX(advance_amount) FROM "orders" WHERE "id" > 2',
'"(SELECT MAX(advance_amount) FROM ""orders"" WHERE ""id"" >" 2',
],

'SQLi: Trailing operators after valid function' => [
false,
true,
true,
'COUNT(id) OR 1=1',
'COUNT(id) OR "1=1"',
],
'SQLi: Unbalanced parenthesis attack' => [
false,
true,
true,
'id) OR (1=1',
'"id) OR" "(1=1"',
],
'SQLi: String literal bypass attempt' => [
false,
true,
true,
"'admin' OR 1=1",
'"\'admin\' OR" "1=1"',
],
];
}
Expand All @@ -446,6 +486,11 @@ public static function provideEscapeIdentifiers(): iterable
// $item, $expected
'simple' => ['test', '"test"'],
'with dots' => ['com.sitedb.web', '"com"."sitedb"."web"'],

// Security Tests: SQL Injection Bypasses in escapeIdentifiers
'SQLi: Trailing operators after valid function' => ['COUNT(id) OR 1=1', '"COUNT(id) OR 1=1"'],
'SQLi: Unbalanced parenthesis attack' => ['id) OR (1=1', '"id) OR (1=1"'],
'SQLi: String literal bypass attempt' => ["'admin' OR 1=1", '"\'admin\' OR 1=1"'],
];
}

Expand Down