From 9e024ee50432625ee58863307a5e281db5247621 Mon Sep 17 00:00:00 2001 From: Bogdan Date: Sun, 7 Jun 2026 00:02:40 +0200 Subject: [PATCH] fix: escapeIdentifiers and protectIdentifiers SQL injection bypass Implemented strict regex validation for SQL function calls and string literals in BaseConnection::isSafeToBypassEscape() to prevent SQL injection vulnerabilities. Updated tests to match the new secure behavior and added explicit SQL injection test cases. --- system/Database/BaseConnection.php | 39 ++++++++++-- tests/system/Database/BaseConnectionTest.php | 67 ++++++++++++++++---- 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 273e4c60c4d5..d88506041b55 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -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 @@ -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))*\))(?:\s+(?:AS\s+)?(?:[a-zA-Z0-9_.]+|"[^"]*"|\'[^\']*\'|`[^`]*`))?$/is', $item)) { + return true; + } + } + + return false; + } + /** * Returns escaped table name with alias. */ @@ -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; } diff --git a/tests/system/Database/BaseConnectionTest.php b/tests/system/Database/BaseConnectionTest.php index 4d6d1f76173d..8d416da0c038 100644 --- a/tests/system/Database/BaseConnectionTest.php +++ b/tests/system/Database/BaseConnectionTest.php @@ -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"', ], @@ -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"', ], ]; } @@ -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"'], ]; }