From 230d23516407c2ceeb6ebce794975722c30a94a5 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Wed, 9 Sep 2026 13:10:54 +0200 Subject: [PATCH] fix(manager): validate table identifiers before they reach raw SQL a=54 read the table to OPTIMIZE or TRUNCATE straight from $_REQUEST and put it into a statement unquoted, so a manager holding settings + logs - without bk_manager, and therefore without the restore form that runs SQL by design - could append SQL of its own or truncate any table. The backup manager passed its checkbox list on to pg_dump on a command line the same way. Both now go through Database::isValidTableName(): a bare identifier carrying the configured prefix. The pattern is the whole guard, since nothing matching it can leave the identifier it is substituted into. Existence is deliberately not checked - an unknown table is a failing statement rather than an injection, and a catalogue lookup would tie every optimize to schema read rights the hosting may not grant. TRUNCATE keeps the raw expression because the builder would otherwise prefix an already prefixed name. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YSZQgDv5ASxQaiYd5C1nJR --- core/src/Database.php | 24 ++++ .../Unit/Security/TableNameWhitelistTest.php | 112 ++++++++++++++++++ manager/actions/bkmanager.static.php | 10 ++ .../processors/optimize_table.processor.php | 16 ++- 4 files changed, 156 insertions(+), 6 deletions(-) create mode 100644 core/tests/Unit/Security/TableNameWhitelistTest.php diff --git a/core/src/Database.php b/core/src/Database.php index b5b3414860..13db6829dc 100644 --- a/core/src/Database.php +++ b/core/src/Database.php @@ -240,6 +240,30 @@ public function getTableName($table) return $this->getFullTableName($table); } + /** + * Whether the value can be written into raw SQL as a table identifier: a bare name that + * carries the configured prefix. + * + * The pattern is the whole guard - no quote, space, semicolon or shell metacharacter gets + * through it, so the name cannot leave the identifier it is substituted into. Existence is + * deliberately not checked: an unknown table is a failing statement, not an injection, and + * asking the catalogue would make every call depend on schema read rights. + * + * @since 3.5.8 + * @param mixed $table + * @return bool + */ + public function isValidTableName($table) + { + if (!is_string($table) || !preg_match('/^[A-Za-z0-9_]+$/', $table)) { + return false; + } + + $prefix = (string) $this->getConfig('prefix'); + + return $prefix === '' || strncmp($table, $prefix, strlen($prefix)) === 0; + } + public function getValue($result) { $out = false; diff --git a/core/tests/Unit/Security/TableNameWhitelistTest.php b/core/tests/Unit/Security/TableNameWhitelistTest.php new file mode 100644 index 0000000000..edec0cbd22 --- /dev/null +++ b/core/tests/Unit/Security/TableNameWhitelistTest.php @@ -0,0 +1,112 @@ +addConnection([ + 'driver' => 'sqlite', + 'database' => ':memory:', + 'prefix' => $prefix, + ]); + $db->setAsGlobal(); + + return $db; +} + +afterEach(function () { + Capsule::connection()->disconnect(); +}); + +describe('Database::isValidTableName()', function () { + + test('accepts a bare prefixed identifier', function () { + $db = tableNameDatabase('evo_'); + + expect($db->isValidTableName('evo_users'))->toBeTrue() + ->and($db->isValidTableName('evo_manager_log'))->toBeTrue(); + }); + + test('rejects a name outside the prefix', function () { + expect(tableNameDatabase('evo_')->isValidTableName('other_app_sessions'))->toBeFalse(); + }); + + test('rejects everything that could end the identifier', function () { + $db = tableNameDatabase('evo_'); + + // OPTIMIZE TABLE / TRUNCATE take the name unquoted, and pg_dump takes it as a -t argument + // on a command line, so a break-out needs one of these characters to survive. + expect($db->isValidTableName('evo_users; DROP TABLE evo_users'))->toBeFalse() + ->and($db->isValidTableName('evo_users`, (SELECT 1)'))->toBeFalse() + ->and($db->isValidTableName("evo_users' UNION SELECT 1"))->toBeFalse() + ->and($db->isValidTableName('evo_users WHERE 1=1'))->toBeFalse() + ->and($db->isValidTableName('evo_users`id`'))->toBeFalse() + ->and($db->isValidTableName('evo_users$(id)'))->toBeFalse() + ->and($db->isValidTableName("evo_users\nDROP"))->toBeFalse() + ->and($db->isValidTableName('evo_db.evo_users'))->toBeFalse(); + }); + + test('rejects anything that is not a non-empty string', function () { + $db = tableNameDatabase('evo_'); + + expect($db->isValidTableName(['evo_users']))->toBeFalse() + ->and($db->isValidTableName(null))->toBeFalse() + ->and($db->isValidTableName(''))->toBeFalse(); + }); + + test('still refuses metacharacters when no prefix is configured', function () { + $db = tableNameDatabase(''); + + expect($db->isValidTableName('users'))->toBeTrue() + ->and($db->isValidTableName('users; DROP TABLE users'))->toBeFalse(); + }); + + test('does not query the database', function () { + $db = tableNameDatabase('evo_'); + $connection = $db->getConnection(); + $connection->enableQueryLog(); + + $db->isValidTableName('evo_users'); + + expect($connection->getQueryLog())->toBe([]); + }); +}); + +describe('call sites', function () { + + test('a=54 validates before optimizing or truncating', function () { + $source = file_get_contents(__DIR__ . '/../../../../manager/processors/optimize_table.processor.php'); + + expect($source) + ->toContain("isValidTableName(\$_REQUEST['t'])") + ->toContain("isValidTableName(\$_REQUEST['u'])") + // The name still has to bypass the builder's prefixing, so it stays an expression - + // the validation above is what makes that safe. + ->toContain("\DB::table(\DB::raw(\$_REQUEST['u']))"); + }); + + test('the backup manager drops invalid names from the checkbox list', function () { + $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php'); + + expect($source)->toContain('$db->isValidTableName($table)'); + }); +}); diff --git a/manager/actions/bkmanager.static.php b/manager/actions/bkmanager.static.php index c76b81ba5c..6cbfcfbf47 100755 --- a/manager/actions/bkmanager.static.php +++ b/manager/actions/bkmanager.static.php @@ -81,6 +81,16 @@ EvolutionCMS()->webAlertAndQuit("Please select a valid table from the list below."); } + // The names reach pg_dump on a command line and the dumpers inside raw SQL, so keep only + // the bare prefixed identifiers. + $db = EvolutionCMS()->getDatabase(); + $tables = array_values(array_filter($tables, static function ($table) use ($db) { + return $db->isValidTableName($table); + })); + if (!$tables) { + EvolutionCMS()->webAlertAndQuit("Please select a valid table from the list below."); + } + /* * Code taken from Ralph A. Dahlgren MySQLdumper Snippet - Etomite 0.6 - 2004-09-27 * Modified by Raymond 3-Jan-2005 diff --git a/manager/processors/optimize_table.processor.php b/manager/processors/optimize_table.processor.php index 9c7ba4ea40..cc0926077a 100755 --- a/manager/processors/optimize_table.processor.php +++ b/manager/processors/optimize_table.processor.php @@ -6,28 +6,32 @@ EvolutionCMS()->webAlertAndQuit($_lang["error_no_privileges"]); } +$db = EvolutionCMS()->getDatabase(); + if (isset($_REQUEST['t'])) { - if (empty($_REQUEST['t'])) { + // The name goes into raw SQL, so it has to be a bare prefixed identifier. + if (!$db->isValidTableName($_REQUEST['t'])) { EvolutionCMS()->webAlertAndQuit($_lang["error_no_optimise_tablename"]); } // Set the item name for logger $_SESSION['itemname'] = $_REQUEST['t']; - if(EvolutionCMS()->getDatabase()->getConfig('driver') != 'pgsql'){ - EvolutionCMS()->getDatabase()->optimize($_REQUEST['t']); - } + if ($db->getConfig('driver') != 'pgsql') { + $db->optimize($_REQUEST['t']); + } } elseif (isset($_REQUEST['u'])) { - if (empty($_REQUEST['u'])) { + if (!$db->isValidTableName($_REQUEST['u'])) { EvolutionCMS()->webAlertAndQuit($_lang["error_no_truncate_tablename"]); } // Set the item name for logger $_SESSION['itemname'] = $_REQUEST['u']; - \DB::table(\DB::raw($_REQUEST['u']))->truncate(); + // Raw so the builder does not prefix an already prefixed name; safe now that it is a validated identifier. + \DB::table(\DB::raw($_REQUEST['u']))->truncate(); } else { EvolutionCMS()->webAlertAndQuit($_lang["error_no_optimise_tablename"]);