fix(manager): validate table identifiers before they reach raw SQL

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;

core/tests/Unit/Security/TableNameWhitelistTest.php

@@ -0,0 +1,112 @@
+<?php
+
+/*
+|--------------------------------------------------------------------------
+| Table identifier validation
+|--------------------------------------------------------------------------
+|
+| a=54 took the table to OPTIMIZE or TRUNCATE straight out of $_REQUEST and put it into raw SQL,
+| and the backup manager passed its checkbox list on to pg_dump on a command line. Both now go
+| through Database::isValidTableName().
+|
+| The pattern is the whole guard. Nothing that matches it can leave the identifier it is
+| substituted into, which is why existence is not checked as well: an unknown table is a failing
+| statement, not an injection, and a catalogue lookup would tie every optimize to schema read
+| rights the hosting may not grant.
+|
+| @since 3.5.8
+*/
+
+use EvolutionCMS\Database;
+use Illuminate\Database\Capsule\Manager as Capsule;
+
+function tableNameDatabase(string $prefix): Database
+{
+    $db = new Database();
+    $db->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)');
+    });
+});

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

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