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