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"]);