From 2d63d51237c34db29cc9d8dacd81400483f0eb27 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 29 Jun 2026 10:27:01 +0200 Subject: [PATCH] fix: harden database backup imports --- app/Http/Controllers/UploadController.php | 44 +--- app/Livewire/Project/Database/ImportForm.php | 76 +++++++ app/Support/DatabaseBackupFileValidator.php | 209 ++++++++++++++++++ .../DatabaseBackupUploadValidationTest.php | 178 +++++++++++++++ .../Unit/Livewire/Database/S3RestoreTest.php | 8 +- 5 files changed, 471 insertions(+), 44 deletions(-) create mode 100644 app/Support/DatabaseBackupFileValidator.php diff --git a/app/Http/Controllers/UploadController.php b/app/Http/Controllers/UploadController.php index 6c3dda402..d09435fd8 100644 --- a/app/Http/Controllers/UploadController.php +++ b/app/Http/Controllers/UploadController.php @@ -2,6 +2,7 @@ namespace App\Http\Controllers; +use App\Support\DatabaseBackupFileValidator; use Illuminate\Http\Request; use Illuminate\Http\UploadedFile; use Illuminate\Routing\Controller as BaseController; @@ -13,24 +14,7 @@ class UploadController extends BaseController { private const MAX_BYTES = 10 * 1024 * 1024 * 1024; // 10 GiB - private const ALLOWED_EXTENSIONS = [ - 'sql', - 'sql.gz', - 'gz', - 'zip', - 'tar', - 'tar.gz', - 'tgz', - 'dump', - 'bak', - 'bson', - 'bson.gz', - 'archive', - 'archive.gz', - 'bz2', - 'xz', - 'dmp', - ]; + private const ALLOWED_EXTENSIONS = DatabaseBackupFileValidator::ALLOWED_EXTENSIONS; public function upload(Request $request) { @@ -80,10 +64,7 @@ public function upload(Request $request) protected function saveFile(UploadedFile $file, string $resourceIdentifier) { - $originalName = $file->getClientOriginalName(); - $size = $file->getSize(); - - if (! self::hasAllowedExtension($originalName) || $size === false || $size > self::MAX_BYTES) { + if (! DatabaseBackupFileValidator::isUploadAllowed($file, self::MAX_BYTES)) { @unlink($file->getPathname()); return response()->json([ @@ -103,24 +84,7 @@ protected function saveFile(UploadedFile $file, string $resourceIdentifier) private static function hasAllowedExtension(string $name): bool { - $lower = strtolower($name); - $suffixes = array_map(fn ($ext) => '.'.$ext, self::ALLOWED_EXTENSIONS); - usort($suffixes, fn ($a, $b) => strlen($b) <=> strlen($a)); - - foreach ($suffixes as $suffix) { - if (! str_ends_with($lower, $suffix)) { - continue; - } - - $stem = substr($lower, 0, -strlen($suffix)); - if ($stem !== '' && ! str_ends_with($stem, '.')) { - return true; - } - - return false; - } - - return false; + return DatabaseBackupFileValidator::hasAllowedExtension($name); } private static function formatMaxSize(): string diff --git a/app/Livewire/Project/Database/ImportForm.php b/app/Livewire/Project/Database/ImportForm.php index ccc7b347d..722daa6df 100644 --- a/app/Livewire/Project/Database/ImportForm.php +++ b/app/Livewire/Project/Database/ImportForm.php @@ -14,6 +14,7 @@ use App\Models\StandaloneMysql; use App\Models\StandalonePostgresql; use App\Models\StandaloneRedis; +use App\Support\DatabaseBackupFileValidator; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Facades\Storage; @@ -451,10 +452,20 @@ public function runImport(string $password = ''): bool|string // Check if an uploaded file exists first (takes priority over custom location) if (Storage::exists($backupFileName)) { $path = Storage::path($backupFileName); + + // Reject malicious PostgreSQL payloads before transferring the file anywhere. + if ($this->isPostgresqlRestore() && DatabaseBackupFileValidator::fileContainsPostgresqlProgramExecution($path)) { + Storage::delete($backupFileName); + $this->dispatch('error', 'The uploaded backup contains disallowed PostgreSQL restore directives (COPY ... PROGRAM or psql shell commands) and was rejected.'); + + return true; + } + $tmpPath = '/tmp/'.basename($backupFileName).'_'.$this->resourceUuid; instant_scp($path, $tmpPath, $this->server); Storage::delete($backupFileName); $this->importCommands[] = "docker cp {$tmpPath} {$this->container}:{$tmpPath}"; + $this->addRestoreSafetyCheckCommand($this->importCommands, $tmpPath); } elseif (filled($this->customLocation)) { // Validate the custom location to prevent command injection if (! $this->validateServerPath($this->customLocation)) { @@ -465,6 +476,7 @@ public function runImport(string $password = ''): bool|string $tmpPath = '/tmp/restore_'.$this->resourceUuid; $escapedCustomLocation = escapeshellarg($this->customLocation); $this->importCommands[] = "docker cp {$escapedCustomLocation} {$this->container}:{$tmpPath}"; + $this->addRestoreSafetyCheckCommand($this->importCommands, $tmpPath); } else { $this->dispatch('error', 'The file does not exist or has been deleted.'); @@ -721,6 +733,7 @@ public function restoreFromS3(string $password = ''): bool|string // 6. Copy from helper to server, then immediately to database container $commands[] = "docker cp {$escapedHelperContainerPath} {$escapedServerTmpPath}"; $commands[] = "docker cp {$escapedServerTmpPath} {$escapedDatabaseContainerTmpPath}"; + $this->addRestoreSafetyCheckCommand($commands, $containerTmpPath); // 7. Cleanup helper container and server temp file immediately (no longer needed) $commands[] = "docker rm -f {$containerName} 2>/dev/null || true"; @@ -765,6 +778,69 @@ public function restoreFromS3(string $password = ''): bool|string return true; } + public function buildRestoreSafetyCheckCommand(string $tmpPath): ?string + { + $script = $this->buildPostgresRestoreScanScript($tmpPath); + + if ($script === null) { + return null; + } + + return "docker exec {$this->container} sh -c ".escapeshellarg($script); + } + + /** + * Build the POSIX shell snippet that aborts (exit 1) when a PostgreSQL + * backup contains directives leading to OS command execution. + * + * Hardened against bypasses: + * - decompresses gzip backups before scanning, + * - strips `--` line comments and flattens newlines so multi-line and + * comment-separated payloads (e.g. `FROM/**​/PROGRAM`) are caught, + * - matches a literal `\!` shell escape and `\o|`/`\g|` pipe redirects. + */ + public function buildPostgresRestoreScanScript(string $tmpPath): ?string + { + if (! $this->isPostgresqlRestore()) { + return null; + } + + $escapedTmpPath = escapeshellarg($tmpPath); + + // Token separator PostgreSQL treats as whitespace: real whitespace or a + // /* ... */ block comment (used to split keywords like FROM/**/PROGRAM). + $sep = '([[:space:]]|/\\*[^*]*\\*/)'; + + $pattern = implode('|', [ + "copy{$sep}+[^;]*(from|to){$sep}+program", + '(^|[[:space:]])\\\\!', + "(^|[[:space:]])\\\\(o|g){$sep}*\\|", + ]); + $escapedPattern = escapeshellarg($pattern); + + return "if (gunzip -cf {$escapedTmpPath} 2>/dev/null || cat {$escapedTmpPath}) | sed 's/--.*//' | tr '\n\r\t' ' ' | grep -Eiq {$escapedPattern}; then echo 'Blocked PostgreSQL restore: COPY ... PROGRAM and psql shell commands are not allowed.'; exit 1; fi"; + } + + private function addRestoreSafetyCheckCommand(array &$commands, string $tmpPath): void + { + $command = $this->buildRestoreSafetyCheckCommand($tmpPath); + + if ($command !== null) { + $commands[] = $command; + } + } + + private function isPostgresqlRestore(): bool + { + $morphClass = $this->resource->getMorphClass(); + + if ($morphClass === ServiceDatabase::class) { + return str_contains($this->resource->databaseType(), 'postgres'); + } + + return $morphClass === StandalonePostgresql::class || $morphClass === 'postgresql'; + } + public function buildRestoreCommand(string $tmpPath): string { $escapedTmpPath = escapeshellarg($tmpPath); diff --git a/app/Support/DatabaseBackupFileValidator.php b/app/Support/DatabaseBackupFileValidator.php new file mode 100644 index 000000000..c232462f6 --- /dev/null +++ b/app/Support/DatabaseBackupFileValidator.php @@ -0,0 +1,209 @@ +getClientOriginalName(); + $size = $file->getSize(); + + if ($size === false || $size > $maxBytes) { + return false; + } + + $extension = self::extensionFor($originalName); + if ($extension === null) { + return false; + } + + return self::contentMatchesExtension($file->getPathname(), $extension); + } + + /** + * Scan a stored backup file (decompressing gzip on the fly) for PostgreSQL + * restore directives that lead to OS command execution. + */ + public static function fileContainsPostgresqlProgramExecution(string $path): bool + { + $contents = self::readPossiblyGzippedText($path); + + if ($contents === null) { + return false; + } + + return self::containsPostgresqlProgramExecution($contents); + } + + public static function containsPostgresqlProgramExecution(string $sql): bool + { + $withoutComments = self::stripSqlComments($sql); + + if (preg_match('/^\s*\\\\(?:!|copy\b.*\bprogram\b)/mi', $withoutComments) === 1) { + return true; + } + + return preg_match('/\bcopy\b[\s\S]{0,2000}\b(?:from|to)\s+program\b/i', $withoutComments) === 1; + } + + private static function extensionFor(string $name): ?string + { + $lower = strtolower($name); + $suffixes = array_map(fn (string $ext) => '.'.$ext, self::ALLOWED_EXTENSIONS); + usort($suffixes, fn (string $a, string $b) => strlen($b) <=> strlen($a)); + + foreach ($suffixes as $suffix) { + if (! str_ends_with($lower, $suffix)) { + continue; + } + + $stem = substr($lower, 0, -strlen($suffix)); + if ($stem === '' || str_ends_with($stem, '.')) { + return null; + } + + $parts = array_filter(explode('.', $stem)); + if (array_intersect($parts, self::DANGEROUS_EXTENSIONS) !== []) { + return null; + } + + return ltrim($suffix, '.'); + } + + return null; + } + + private static function contentMatchesExtension(string $path, string $extension): bool + { + $sample = (string) file_get_contents($path, false, null, 0, 4096); + + return match ($extension) { + 'sql' => self::looksLikeText($sample) && ! self::containsPostgresqlProgramExecution($sample), + 'sql.gz', 'gz', 'tar.gz', 'tgz', 'bson.gz', 'archive.gz' => str_starts_with($sample, "\x1f\x8b"), + 'zip' => str_starts_with($sample, "PK\x03\x04") || str_starts_with($sample, "PK\x05\x06") || str_starts_with($sample, "PK\x07\x08"), + 'tar' => substr($sample, 257, 5) === 'ustar', + 'bz2' => str_starts_with($sample, 'BZh'), + 'xz' => str_starts_with($sample, "\xfd7zXZ\x00"), + 'dump', 'bak', 'archive', 'dmp' => str_starts_with($sample, 'PGDMP') + || (self::looksLikeText($sample) && ! self::containsPostgresqlProgramExecution($sample)), + 'bson' => self::looksLikeBson($path, $sample), + default => false, + }; + } + + private static function readPossiblyGzippedText(string $path): ?string + { + // Cap the scan so a huge legitimate dump cannot exhaust memory; the + // remote pre-restore scanner inspects the full file as a second layer. + $maxBytes = 50 * 1024 * 1024; + + $handle = @fopen($path, 'rb'); + if ($handle === false) { + return null; + } + $magic = (string) fread($handle, 2); + fclose($handle); + + if ($magic === "\x1f\x8b") { + $gz = @gzopen($path, 'rb'); + if ($gz === false) { + return null; + } + + $data = ''; + while (! gzeof($gz) && strlen($data) < $maxBytes) { + $chunk = gzread($gz, 1024 * 1024); + if ($chunk === false || $chunk === '') { + break; + } + $data .= $chunk; + } + gzclose($gz); + + return $data; + } + + return (string) file_get_contents($path, false, null, 0, $maxBytes); + } + + private static function looksLikeText(string $sample): bool + { + if ($sample === '' || str_contains($sample, "\0")) { + return false; + } + + return mb_check_encoding($sample, 'UTF-8') || mb_check_encoding($sample, 'ASCII'); + } + + private static function looksLikeBson(string $path, string $sample): bool + { + if (strlen($sample) < 5) { + return false; + } + + $documentLength = unpack('V', substr($sample, 0, 4))[1] ?? 0; + $fileSize = filesize($path) ?: 0; + + return $documentLength >= 5 && $documentLength <= $fileSize; + } + + private static function stripSqlComments(string $sql): string + { + $sql = preg_replace('/\/\*[\s\S]*?\*\//', ' ', $sql) ?? $sql; + + return preg_replace('/--[^\r\n]*/', ' ', $sql) ?? $sql; + } +} diff --git a/tests/Feature/DatabaseBackupUploadValidationTest.php b/tests/Feature/DatabaseBackupUploadValidationTest.php index a9d9886b8..1919b02b9 100644 --- a/tests/Feature/DatabaseBackupUploadValidationTest.php +++ b/tests/Feature/DatabaseBackupUploadValidationTest.php @@ -1,6 +1,28 @@ exitCode() === 1; +} function invokeHasAllowedExtension(string $name): bool { @@ -10,6 +32,28 @@ function invokeHasAllowedExtension(string $name): bool return $method->invoke(null, $name); } +function backupValidationImportFormWithResource(string $modelClass): ImportForm +{ + $component = new class extends ImportForm + { + public $resource; + }; + + $database = Mockery::mock($modelClass); + $database->shouldReceive('getMorphClass')->andReturn($modelClass); + $component->resource = $database; + + return $component; +} + +function makeTemporaryUpload(string $name, string $content): UploadedFile +{ + $path = tempnam(sys_get_temp_dir(), 'coolify-upload-test-'); + file_put_contents($path, $content); + + return new UploadedFile($path, $name, null, null, true); +} + test('hasAllowedExtension accepts supported extensions', function (string $name) { expect(invokeHasAllowedExtension($name))->toBeTrue(); })->with([ @@ -46,6 +90,140 @@ function invokeHasAllowedExtension(string $name): bool 'misleading double ext' => ['shell.php.sql-evil'], ]); +test('hasAllowedExtension rejects dangerous double extensions', function (string $name) { + expect(invokeHasAllowedExtension($name))->toBeFalse(); +})->with([ + 'php sql' => ['evil.php.sql'], + 'php gzip' => ['evil.php.gz'], + 'shell tar' => ['evil.sh.tar'], + 'php tar gzip' => ['shell.php.tar.gz'], + 'exe zip' => ['cmd.exe.zip'], + 'jsp sql' => ['evil.jsp.sql'], +]); + +test('backup validator rejects content that does not match the backup extension', function () { + $file = makeTemporaryUpload('payload.sql.gz', 'not actually gzip'); + + expect(DatabaseBackupFileValidator::isUploadAllowed($file, 10 * 1024 * 1024))->toBeFalse(); +}); + +test('backup validator accepts valid plain sql and gzip backup content', function () { + $plainSql = makeTemporaryUpload('backup.sql', "CREATE TABLE users (id integer);\n"); + $gzipSql = makeTemporaryUpload('backup.sql.gz', gzencode("CREATE TABLE users (id integer);\n")); + + expect(DatabaseBackupFileValidator::isUploadAllowed($plainSql, 10 * 1024 * 1024))->toBeTrue() + ->and(DatabaseBackupFileValidator::isUploadAllowed($gzipSql, 10 * 1024 * 1024))->toBeTrue(); +}); + +test('postgresql backup safety scanner detects program execution payloads', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeTrue(); +})->with([ + 'copy from program' => ["COPY pwned FROM PROGRAM 'id';"], + 'copy to program' => ["COPY pwned TO PROGRAM 'cat > /tmp/out';"], + 'copy with block comment' => ["COPY pwned FROM/**/PROGRAM 'id';"], + 'psql shell command' => ["\\! id\n"], + 'psql copy program' => ["\\copy pwned from program 'id'\n"], +]); + +test('postgresql backup safety scanner allows ordinary sql dumps', function () { + $dump = <<<'SQL' +-- PostgreSQL database dump +CREATE TABLE users (id integer, name text); +COPY users (id, name) FROM stdin; +1 Taylor +\. +SQL; + + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($dump))->toBeFalse(); +}); + +test('postgresql restore commands include a safety check before execution', function () { + $component = new class extends ImportForm + { + public function __get($property) + { + if ($property === 'resource') { + return new class + { + public function getMorphClass(): string + { + return StandalonePostgresql::class; + } + }; + } + + return parent::__get($property); + } + }; + $component->container = 'postgres-test'; + + $command = $component->buildRestoreSafetyCheckCommand('/tmp/restore_test'); + + expect($command) + ->toContain('docker exec postgres-test') + ->toContain('COPY ... PROGRAM') + ->toContain('/tmp/restore_test') + ->toContain('grep -Eiq'); +}); + +test('non postgresql restore commands do not include a safety check', function () { + $component = backupValidationImportFormWithResource('App\Models\StandaloneMysql'); + $component->container = 'mysql-test'; + + expect($component->buildRestoreSafetyCheckCommand('/tmp/restore_test'))->toBeNull(); +}); + +test('file scanner detects program execution payloads inside gzipped backups', function () { + $gzPayload = writeScanPayload("CREATE TABLE x();\nCOPY x FROM/**/PROGRAM 'id';\n", gzip: true); + + expect(DatabaseBackupFileValidator::fileContainsPostgresqlProgramExecution($gzPayload))->toBeTrue(); +}); + +test('file scanner allows ordinary gzipped dumps', function () { + $gzClean = writeScanPayload("CREATE TABLE x();\nCOPY x FROM stdin;\n1\\.\n", gzip: true); + + expect(DatabaseBackupFileValidator::fileContainsPostgresqlProgramExecution($gzClean))->toBeFalse(); +}); + +test('backup validator rejects plaintext .dump containing program execution', function () { + $file = makeTemporaryUpload('evil.dump', "COPY x FROM PROGRAM 'id';\n"); + + expect(DatabaseBackupFileValidator::isUploadAllowed($file, 10 * 1024 * 1024))->toBeFalse(); +}); + +test('remote postgresql scanner blocks bypass payloads', function (string $content, bool $gzip) { + $component = backupValidationImportFormWithResource(StandalonePostgresql::class); + $component->container = 'postgres-test'; + + $payload = writeScanPayload($content, $gzip); + $script = $component->buildPostgresRestoreScanScript($payload); + + expect(scannerBlocks($script))->toBeTrue(); +})->with([ + 'psql shell escape' => ["\\! id\n", false], + 'copy from program' => ["COPY x FROM PROGRAM 'id';\n", false], + 'copy with block comment' => ["COPY x FROM/**/PROGRAM 'id';\n", false], + 'copy split across lines' => ["COPY x FROM\nPROGRAM 'id';\n", false], + 'copy to program' => ["COPY x TO PROGRAM 'cat > /tmp/x';\n", false], + 'psql pipe redirect' => ["\\o | id\n", false], + 'gzipped comment bypass' => ["COPY x FROM/**/PROGRAM 'id';\n", true], +]); + +test('remote postgresql scanner allows legitimate restores', function (string $content, bool $gzip) { + $component = backupValidationImportFormWithResource(StandalonePostgresql::class); + $component->container = 'postgres-test'; + + $payload = writeScanPayload($content, $gzip); + $script = $component->buildPostgresRestoreScanScript($payload); + + expect(scannerBlocks($script))->toBeFalse(); +})->with([ + 'commented out payload' => ["-- COPY x FROM PROGRAM 'id'\nSELECT 1;\n", false], + 'copy from stdin' => ["COPY users FROM stdin;\n1\tTaylor\n\\.\n", false], + 'plain select' => ["SELECT * FROM users;\n", false], + 'gzipped clean dump' => ["CREATE TABLE users (id int);\n", true], +]); + test('MAX_BYTES constant is 10 GiB', function () { $constant = (new ReflectionClass(UploadController::class))->getConstant('MAX_BYTES'); expect($constant)->toBe(10 * 1024 * 1024 * 1024); diff --git a/tests/Unit/Livewire/Database/S3RestoreTest.php b/tests/Unit/Livewire/Database/S3RestoreTest.php index 4dfc7f51c..77695a8d7 100644 --- a/tests/Unit/Livewire/Database/S3RestoreTest.php +++ b/tests/Unit/Livewire/Database/S3RestoreTest.php @@ -34,7 +34,7 @@ function importFormWithResource(string $modelClass): ImportForm $result = $component->buildRestoreCommand('/tmp/test.dump'); - expect($result)->toContain('gunzip -cf /tmp/test.dump'); + expect($result)->toContain("gunzip -cf '/tmp/test.dump'"); expect($result)->toContain('psql -U ${POSTGRES_USER} -d ${POSTGRES_DB:-${POSTGRES_USER:-postgres}}'); }); @@ -46,7 +46,7 @@ function importFormWithResource(string $modelClass): ImportForm $result = $component->buildRestoreCommand('/tmp/test.dump'); expect($result)->toContain('mysql -u $MYSQL_USER'); - expect($result)->toContain('< /tmp/test.dump'); + expect($result)->toContain("< '/tmp/test.dump'"); }); test('buildRestoreCommand handles MariaDB without dumpAll', function () { @@ -57,7 +57,7 @@ function importFormWithResource(string $modelClass): ImportForm $result = $component->buildRestoreCommand('/tmp/test.dump'); expect($result)->toContain('mariadb -u $MARIADB_USER'); - expect($result)->toContain('< /tmp/test.dump'); + expect($result)->toContain("< '/tmp/test.dump'"); }); test('buildRestoreCommand always appends the MongoDB archive path', function (bool $dumpAll) { @@ -68,5 +68,5 @@ function importFormWithResource(string $modelClass): ImportForm $result = $component->buildRestoreCommand('/tmp/test.dump'); expect($result)->toContain('mongorestore'); - expect($result)->toContain('--archive=/tmp/test.dump'); + expect($result)->toContain("--archive='/tmp/test.dump'"); })->with([false, true]);