From fdc8c0be729d293fd0ec1ce5be54d5a5d91eb971 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Wed, 9 Sep 2026 13:46:37 +0200 Subject: [PATCH] fix(manager): run pg backups without a shell The PostgreSQL branches of the backup manager built psql and pg_dump command lines by concatenation and handed them to exec(). The snapshot name from the restore form and the table names from the checkbox list went in unquoted, so bk_manager - a permission whose intended reach is arbitrary SQL through the restore form, not arbitrary shell - was enough to append a command of its own. The password rode in argv, where ps and /proc//cmdline show it to every local account. All three branches now go through DatabaseBackupService, which invokes the client with an argument list and the password in the environment. No shell parses either, so an argument holding a semicolon stays one argument and nothing depends on escaping being right. This is the same form createSnapshot() already used, which also means these paths start working on Windows, where a leading PGPASSWORD= assignment was a cmd.exe syntax error. The dump no longer lands in assets/backup/temp.php. That path is inside the web root and the default ht.access excludes assets/ from every rule it has, so a full dump sat there readable by anyone who guessed the name; it now goes to the snapshot directory under a name that is not guessable. The deny rule written into that directory gained the Apache 2.4 form - Order/Deny alone is 2.2 syntax and 2.4 without mod_access_compat answers 500, leaving the directory served. The snapshot to restore is resolved against the snapshot directory and rejected if it lands outside, which also closes the traversal on the MySQL and SQLite branches, where the name reached import_sql_from_file() unchecked. The drop-tables checkbox is honoured again; it built an argument the command never used. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YSZQgDv5ASxQaiYd5C1nJR --- core/src/Services/DatabaseBackupService.php | 198 ++++++++++++++---- .../Unit/Security/BackupManagerShellTest.php | 122 +++++++++++ manager/actions/bkmanager.static.php | 52 +++-- 3 files changed, 310 insertions(+), 62 deletions(-) create mode 100644 core/tests/Unit/Security/BackupManagerShellTest.php diff --git a/core/src/Services/DatabaseBackupService.php b/core/src/Services/DatabaseBackupService.php index 9838dcb22a..22e3e2295f 100644 --- a/core/src/Services/DatabaseBackupService.php +++ b/core/src/Services/DatabaseBackupService.php @@ -6,6 +6,14 @@ class DatabaseBackupService { + /** + * Apache deny rule for the directories dumps land in. `Order deny,allow` on its own is 2.2 + * syntax: on 2.4 without mod_access_compat it is a 500, which leaves the directory served + * rather than denied, so both forms are written and each is guarded by its module. + */ + public const DENY_HTACCESS = "\n Require all denied\n\n" + . "\n Order deny,allow\n Deny from all\n\n"; + protected string $basePath; public function __construct(?string $basePath = null) @@ -87,7 +95,7 @@ protected function prepareSnapshotPath($snapshotPath) $htaccess = $path . '/.htaccess'; if (!is_file($htaccess)) { - file_put_contents($htaccess, "order deny,allow\ndeny from all\n"); + file_put_contents($htaccess, self::DENY_HTACCESS); } if (!is_writable($path)) { @@ -164,46 +172,116 @@ protected function createDriverSnapshot($driver, $database, $filePath) protected function createPostgresSnapshot($database, $filePath) { - $config = evo()->getDatabase()->getConfig(); - $password = isset($config['password']) ? (string) $config['password'] : ''; + $config = $this->databaseConfig(); $host = isset($config['host']) ? (string) $config['host'] : ''; - $username = isset($config['username']) ? (string) $config['username'] : ''; $tempFilePath = $this->buildTempSnapshotFilePath((string) $filePath); file_put_contents($tempFilePath, $this->buildSqlHeader('--', (string) $database, $host)); - $handle = fopen($tempFilePath, 'ab'); + if (!$this->runPostgresDump(['--clean', '--inserts', '--no-owner', '--no-privileges'], $tempFilePath)) { + if (is_file($tempFilePath)) { + unlink($tempFilePath); + } - if ($handle === false) { + return false; + } + + if (is_file((string) $filePath)) { + unlink((string) $filePath); + } + + return rename($tempFilePath, (string) $filePath); + } + + /** + * Dumps the given tables to a file under the snapshot directory and returns its path, or null + * when the dump failed. The caller owns the file from there on. + * + * @since 3.5.8 + * @param array $tables + * @param bool $dropTables + * @return string|null + */ + public function dumpPostgresTables(array $tables, $dropTables = true) + { + $config = $this->databaseConfig(); + $database = isset($config['database']) ? (string) $config['database'] : ''; + $host = isset($config['host']) ? (string) $config['host'] : ''; + $snapshotPath = $this->resolveSnapshotPath(); + + $this->prepareSnapshotPath($snapshotPath); + + // Under the snapshot directory rather than the old assets/backup/temp.php: that path sat + // in the web root with nothing denying it, so a full dump was readable by anyone who + // guessed the name. Here the directory carries a deny rule and the name is not guessable. + $tempFilePath = $this->buildTempSnapshotFilePath($snapshotPath . 'download.sql'); + + file_put_contents($tempFilePath, $this->buildSqlHeader('--', $database, $host)); + + $arguments = ['--inserts', '--no-owner', '--no-privileges']; + + if ($dropTables) { + $arguments[] = '--clean'; + } + + foreach ($tables as $table) { + $arguments[] = '--table'; + $arguments[] = (string) $table; + } + + if (!$this->runPostgresDump($arguments, $tempFilePath)) { if (is_file($tempFilePath)) { unlink($tempFilePath); } + return null; + } + + return $tempFilePath; + } + + /** + * Replays a SQL file into the database. + * + * @since 3.5.8 + * @param string $path + * @return bool + */ + public function restorePostgresFile($path) + { + $path = (string) $path; + + if (!is_file($path)) { return false; } - // No shell is involved here, and that is the point. The previous form - // was `PGPASSWORD=… pg_dump … >> file`, and a leading VAR=value - // assignment is POSIX shell syntax that cmd.exe rejects outright with - // "'PGPASSWORD' is not recognized", so this backup could never succeed - // on Windows. Passing the password as an environment entry and the - // arguments as a list works the same way on every platform, and has - // the side benefit that nothing has to be quoted for a shell. - $process = new Process( - [ - 'pg_dump', - '--host', $host, - '--username', $username, - '--dbname', (string) $database, - '--clean', - '--inserts', - '--no-owner', - '--no-privileges', - ], - null, - ['PGPASSWORD' => $password] - ); - $process->setTimeout(null); + $process = $this->buildPostgresProcess('psql', ['--file', $path]); + + try { + $process->run(); + } catch (\Throwable $exception) { + return false; + } + + return $process->isSuccessful(); + } + + /** + * Streams a pg_dump run onto the end of the given file. + * + * @param array $arguments + * @param string $tempFilePath + * @return bool + */ + protected function runPostgresDump(array $arguments, $tempFilePath) + { + $handle = fopen($tempFilePath, 'ab'); + + if ($handle === false) { + return false; + } + + $process = $this->buildPostgresProcess('pg_dump', $arguments); try { // Streamed rather than buffered: a dump is as large as the @@ -218,29 +296,65 @@ protected function createPostgresSnapshot($database, $filePath) } catch (\Throwable $exception) { fclose($handle); - if (is_file($tempFilePath)) { - unlink($tempFilePath); - } - return false; } fclose($handle); clearstatcache(true, $tempFilePath); - if (!$process->isSuccessful() || !is_file($tempFilePath) || filesize($tempFilePath) <= 0) { - if (is_file($tempFilePath)) { - unlink($tempFilePath); - } + return $process->isSuccessful() && is_file($tempFilePath) && filesize($tempFilePath) > 0; + } - return false; - } + /** + * Builds a PostgreSQL client invocation. + * + * No shell is involved here, and that is the point. The previous form was + * `PGPASSWORD=... pg_dump ... >> file`, and a leading VAR=value assignment + * is POSIX shell syntax that cmd.exe rejects outright with "'PGPASSWORD' + * is not recognized", so this backup could never succeed on Windows. + * Passing the password as an environment entry and the arguments as a list + * works the same way on every platform, keeps the password out of the + * process list, and has the side benefit that nothing has to be quoted for + * a shell - an argument holding a semicolon stays one argument. + * + * @param string $binary + * @param array $arguments + * @return Process + */ + /** + * The connection settings the pg client is invoked with. + * + * @return array + */ + protected function databaseConfig() + { + return (array) evo()->getDatabase()->getConfig(); + } - if (is_file((string) $filePath)) { - unlink((string) $filePath); - } + protected function buildPostgresProcess($binary, array $arguments) + { + $config = $this->databaseConfig(); + $password = isset($config['password']) ? (string) $config['password'] : ''; + $host = isset($config['host']) ? (string) $config['host'] : ''; + $username = isset($config['username']) ? (string) $config['username'] : ''; + $database = isset($config['database']) ? (string) $config['database'] : ''; - return rename($tempFilePath, (string) $filePath); + $process = new Process( + array_merge( + [ + (string) $binary, + '--host', $host, + '--username', $username, + '--dbname', $database, + ], + array_values($arguments) + ), + null, + ['PGPASSWORD' => $password] + ); + $process->setTimeout(null); + + return $process; } protected function buildTempSnapshotFilePath($filePath) diff --git a/core/tests/Unit/Security/BackupManagerShellTest.php b/core/tests/Unit/Security/BackupManagerShellTest.php new file mode 100644 index 0000000000..c4ca9a4d39 --- /dev/null +++ b/core/tests/Unit/Security/BackupManagerShellTest.php @@ -0,0 +1,122 @@ + 'db.example.test', + 'username' => 'evo', + 'database' => 'evo_site', + 'password' => 'sup3r-s3cret', + ]; + + protected function databaseConfig() + { + return $this->config; + } + + public function inspectProcess($binary, array $arguments) + { + return $this->buildPostgresProcess($binary, $arguments); + } +} + +describe('pg client invocation', function () { + + test('the password travels in the environment, not on the command line', function () { + $process = (new InspectableBackupService())->inspectProcess('pg_dump', ['--clean']); + + expect($process->getEnv())->toBe(['PGPASSWORD' => 'sup3r-s3cret']) + // argv is world readable through ps and /proc//cmdline. + ->and($process->getCommandLine())->not->toContain('sup3r-s3cret'); + }); + + test('an argument carrying shell metacharacters stays a single argument', function () { + $table = 'evo_users; rm -rf /'; + $line = (new InspectableBackupService())->inspectProcess('pg_dump', ['--table', $table]) + ->getCommandLine(); + + // Both escaping styles Symfony uses - sh and cmd.exe - wrap the whole value in quotes, + // so the semicolon never reaches a command parser. + expect(str_contains($line, '"' . $table . '"') || str_contains($line, "'" . $table . "'")) + ->toBeTrue(); + }); + + test('the connection settings are passed as separate arguments', function () { + $line = (new InspectableBackupService())->inspectProcess('psql', ['--file', '/tmp/x.sql']) + ->getCommandLine(); + + expect($line) + ->toContain('db.example.test') + ->toContain('evo_site') + // The old form put credentials into a postgresql://user:password@host URI. + ->not->toContain('postgresql://'); + }); +}); + +describe('deny rule for dump directories', function () { + + test('covers both Apache generations', function () { + expect(DatabaseBackupService::DENY_HTACCESS) + ->toContain('mod_authz_core') + ->toContain('Require all denied') + // Order/Deny alone is 2.2 syntax: on 2.4 without mod_access_compat it is a 500, + // which serves the directory instead of denying it. + ->toContain('Deny from all'); + }); +}); + +describe('backup manager call sites', function () { + + test('no shell is spawned any more', function () { + $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php'); + + expect($source) + ->not->toContain('exec($dump_request') + ->not->toContain('PGPASSWORD=') + ->toContain('->restorePostgresFile(') + ->toContain('->dumpPostgresTables('); + }); + + test('no dump is written under the web root', function () { + $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php'); + + // assets/backup/temp.php is served by Apache: the default ht.access excludes assets/ + // from every rule it has, so a dump left there is one guessed URL away. + expect($source)->not->toContain("EVO_BASE_PATH . 'assets/backup/temp.php'"); + }); + + test('the snapshot to restore has to resolve inside the snapshot directory', function () { + $source = file_get_contents(__DIR__ . '/../../../../manager/actions/bkmanager.static.php'); + + // basename() alone is not enough - the name is also read back as SQL, so the resolved + // path is compared against the directory it must sit in. + expect($source) + ->toContain('$filename = basename(') + ->toContain("preg_match('/^[A-Za-z0-9_.-]+\\.sql$/', \$filename)") + ->toContain('strncmp($path, $snapshotDir') + ->not->toContain("EvolutionCMS()->getConfig('snapshot_path') . \$_POST['filename']"); + }); +}); diff --git a/manager/actions/bkmanager.static.php b/manager/actions/bkmanager.static.php index c76b81ba5c..89ec74d01c 100755 --- a/manager/actions/bkmanager.static.php +++ b/manager/actions/bkmanager.static.php @@ -40,12 +40,10 @@ } else { switch ($driver) { case 'pgsql': - $tempfile_path = EVO_BASE_PATH . 'assets/backup/temp.php'; - file_put_contents($tempfile_path, file_get_contents($_FILES['sqlfile']['tmp_name'])); - - $dump_request = 'PGPASSWORD="'.EvolutionCMS()->getDatabase()->getConfig('password').'" psql --host '.EvolutionCMS()->getDatabase()->getConfig('host').' --username ' . EvolutionCMS()->getDatabase()->getConfig('username') . ' --dbname ' . $dbase . ' < '.$tempfile_path; - exec($dump_request, $data, $data_second); - unlink($tempfile_path); + // Read straight from the upload directory: the copy this replaces landed a full + // dump in assets/backup/, where Apache serves it. + (new EvolutionCMS\Services\DatabaseBackupService(EVO_BASE_PATH)) + ->restorePostgresFile($_FILES['sqlfile']['tmp_name']); break; default: import_sql_from_file($_FILES['sqlfile']['tmp_name']); @@ -56,12 +54,25 @@ header('Location: index.php?r=9&a=93'); exit; } elseif ($mode == 'restore2') { - $path = EvolutionCMS()->getConfig('snapshot_path') . $_POST['filename']; + // The name came from the form unchecked. It used to be concatenated into a shell command, + // and it still names a file that is read back as SQL, so it has to resolve to a snapshot + // inside the snapshot directory and nowhere else. + $snapshotDir = rtrim(str_replace('\\', '/', (string) realpath(EvolutionCMS()->getConfig('snapshot_path'))), '/'); + $filename = basename((string) get_by_key($_POST, 'filename', '', 'is_scalar')); + $path = str_replace('\\', '/', (string) realpath($snapshotDir . '/' . $filename)); + + if ($snapshotDir === '' + || !preg_match('/^[A-Za-z0-9_.-]+\.sql$/', $filename) + || $path === '' + || strncmp($path, $snapshotDir . '/', strlen($snapshotDir) + 1) !== 0) { + EvolutionCMS()->webAlertAndQuit('Invalid snapshot file.'); + } + if (file_exists($path)) { switch ($driver) { case 'pgsql': - $dump_request = 'PGPASSWORD="'.EvolutionCMS()->getDatabase()->getConfig('password').'" psql --host '.EvolutionCMS()->getDatabase()->getConfig('host').' --username ' . EvolutionCMS()->getDatabase()->getConfig('username') . ' --dbname ' . $dbase . ' < '.$path; - exec($dump_request, $data, $data_second); + (new EvolutionCMS\Services\DatabaseBackupService(EVO_BASE_PATH)) + ->restorePostgresFile($path); break; default : import_sql_from_file($path); @@ -90,16 +101,15 @@ @set_time_limit(120); // set timeout limit to 2 minutes switch ($driver) { case 'pgsql': - $tempfile_path = EVO_BASE_PATH . 'assets/backup/temp.php'; - $clean = ''; - if ($_POST['droptables'] == 'on') { - $clean = '--clean'; - } - $table_str = ' -t ' . implode(' -t ', $tables); + // The table names went onto a command line, and the password went into the + // connection URI where the process list shows it. Both are arguments now. + $tempfile_path = (new EvolutionCMS\Services\DatabaseBackupService(EVO_BASE_PATH)) + ->dumpPostgresTables($tables, isset($_POST['droptables'])); - $dump_request = 'pg_dump postgresql://' . EvolutionCMS()->getDatabase()->getConfig('username') . ':'.EvolutionCMS()->getDatabase()->getConfig('password').'@'.EvolutionCMS()->getDatabase()->getConfig('host').'/' . $dbase . ' --clean --inserts --no-owner --no-privileges '. $table_str .'> ' . $tempfile_path; + if ($tempfile_path === null) { + EvolutionCMS()->webAlertAndQuit('Unable to Backup Database'); + } - exec($dump_request, $data, $data_second); dumpSql($tempfile_path); break; case 'sqlite': @@ -133,9 +143,11 @@ mkdir(rtrim(EvolutionCMS()->getConfig('snapshot_path'), '/')); @chmod(rtrim(EvolutionCMS()->getConfig('snapshot_path'), '/'), 0777); } - if (!is_file(EvolutionCMS()->getConfig('snapshot_path').".htaccess")) { - $htaccess = "order deny,allow\ndeny from all\n"; - file_put_contents(EvolutionCMS()->getConfig('snapshot_path').".htaccess", $htaccess); + if (\is_file(EvolutionCMS()->getConfig('snapshot_path').".htaccess")) { + file_put_contents( + EvolutionCMS()->getConfig('snapshot_path').".htaccess", + EvolutionCMS\Services\DatabaseBackupService::DENY_HTACCESS + ); } if (!is_writable(rtrim(EvolutionCMS()->getConfig('snapshot_path'), '/'))) { EvolutionCMS()->webAlertAndQuit(parsePlaceholder($_lang["bkmgr_alert_mkdir"], ['snapshot_path' => EvolutionCMS()->getConfig('snapshot_path')]));