From 0a0d6cf0751f7169d480b6b9017de4bad2e264a2 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 26 Oct 2026 12:54:32 +0100 Subject: [PATCH] fix(postgres): block restore scan bypasses and stop on SQL errors (#12333) Co-authored-by: Claude Opus 5.5 --- app/Support/DatabaseBackupFileValidator.php | 24 ++-- .../DatabaseImportCommandBuilder.php | 35 +++-- .../project/database/import-form.blade.php | 6 +- .../DatabaseBackupUploadValidationTest.php | 135 +++++++++++++++++- .../DatabaseImportCommandBuilderTest.php | 6 +- 5 files changed, 177 insertions(+), 29 deletions(-) diff --git a/app/Support/DatabaseBackupFileValidator.php b/app/Support/DatabaseBackupFileValidator.php index b7052411aa..6e885123f3 100644 --- a/app/Support/DatabaseBackupFileValidator.php +++ b/app/Support/DatabaseBackupFileValidator.php @@ -100,13 +100,24 @@ class DatabaseBackupFileValidator return false; } - $withoutComments = self::stripSqlComments($sql); + // Token separator PostgreSQL treats as whitespace: real whitespace, a block comment + // (the lazy match can extend to the outer end of a nested comment), or a line + // comment. Comment forms also match inside string literals, which can only block + // more input, never less. + $sep = '(?:\s|/\*.*?\*/|--[^\r\n]*[\r\n])'; - if (preg_match('/^\s*\\\\(?:!|copy\b[^\r\n]*\bprogram\b|(?:o|g)\s*\|)/mi', $withoutComments) === 1) { + // psql meta-commands that can run a shell command, anywhere on a line. Same rules as + // the restore scan in DatabaseImportCommandBuilder::buildPostgresRestoreScanScript(). + // A regex error (for example the backtrack limit) returns false and blocks. + if (preg_match('~(?:^|[^\\\\])(?:\\\\\\\\)*\\\\(?:!|(?:o|out|gx?|w|write)[^\S\n][^\\\\\n]*\||copy[^\S\n].*program|(?:setenv|e|edit|ef|ev|i|ir|include|include_relative)(?:\s|$)|(?:[^nbv\s\\\\]|[nbv][^\s\\\\])[^\s\\\\]*[^\S\n][^\\\\\n]*`)~mi', $sql) !== 0) { return true; } - return preg_match('/(?:^|;)\s*copy\b[^;]{0,2000}\b(?:from|to)\s+program\b/i', $withoutComments) === 1; + // A TO/FROM PROGRAM token pair anywhere (dynamic SQL, DO blocks, function bodies), + // a server-side large-object import or export, or ALTER SYSTEM. Comments are no + // longer stripped first, because that cut inside string literals; a commented-out + // PROGRAM statement is now blocked too, which is the safer default. + return preg_match('~(?:from|to)'.$sep.'+program|lo_(?:import|export)'.$sep.'*\(|alter'.$sep.'+system'.$sep.'+(?:set|reset)~is', $sql) !== 0; } private static function extensionFor(string $name): ?string @@ -210,11 +221,4 @@ class DatabaseBackupFileValidator 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/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php index 61c4d2af9e..4535420e6f 100644 --- a/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php +++ b/app/Support/DatabaseImport/DatabaseImportCommandBuilder.php @@ -109,19 +109,36 @@ SH; $escapedPath = escapeshellarg($path); - // Token separator PostgreSQL treats as whitespace: real whitespace or a - // /* ... */ block comment (used to split keywords like FROM/**/PROGRAM). - $sep = '([[:space:]]|/\\*[^*]*\\*/)'; + // Token separator PostgreSQL treats as whitespace: real whitespace, a /* ... */ + // block comment (also nested or holding asterisks), or a -- comment through the + // line end. The joined scan maps line ends to '~' (after it deletes every real + // '~'), so a -- comment ends at the next '~'. Comment forms also match inside + // string literals, which can only block more input, never less. + $sep = '([[:space:]]|~|/\\*.*\\*/|--[^~]*~)'; - $sqlPattern = "(^|;){$sep}*copy{$sep}+[^;]*(from|to){$sep}+program"; - $psqlPattern = "^{$sep}*\\\\(!|copy{$sep}+[^[:space:]]+.*{$sep}+program|(o|g){$sep}*\\|)"; + // A TO/FROM PROGRAM token pair anywhere (so it is also found inside DO blocks, + // EXECUTE strings and function bodies), a server-side large-object import or + // export, or ALTER SYSTEM. The pair is matched without the former (^|;) anchor + // and [^;]* limit, which a ';' inside a string literal or dynamic SQL bypassed. + $sqlPattern = "(from|to){$sep}+program|lo_(import|export){$sep}*\\(|alter{$sep}+system{$sep}+(set|reset)"; + // psql meta-commands that can run a shell command, anywhere on a line (a meta-command + // may follow SQL without a ';'). An odd backslash run starts a meta-command; COPY data + // writes a literal backslash as "\\", so its rows do not match. Blocked: \!, output + // piped by \o/\out/\g/\gx/\w/\write, \copy ... program, \setenv and the editor commands, + // the include commands (they run a file that \o or \copy wrote), and backquotes in the + // arguments of a command. \n, \b, \v and \N are COPY escapes that psql does not know, + // and psql does not expand backquotes for unknown commands. + $psqlPattern = '(^|[^\\\\])(\\\\\\\\)*\\\\(!|(o|out|gx?|w|write)[[:space:]][^\\\\]*\||copy[[:space:]].*program|(setenv|e|edit|ef|ev|i|ir|include|include_relative)([[:space:]]|$)|([^nbv[:space:]\\\\]|[nbv][^[:space:]\\\\])[^[:space:]\\\\]*[[:space:]][^\\\\]*`)'; $escapedSqlPattern = escapeshellarg($sqlPattern); $escapedPsqlPattern = escapeshellarg($psqlPattern); $contents = "{ gunzip -cf {$escapedPath} 2>/dev/null || cat {$escapedPath}; }"; - $scan = static fn (string $source): string => "{$source} | sed 's/--.*//' | grep -Eiq {$escapedPsqlPattern} || {$source} | sed 's/--.*//' | tr '\\n\\r\\t' ' ' | grep -Eiq {$escapedSqlPattern}"; + // Comments are no longer stripped: deleting the rest of a line on "--" also cut + // inside string literals, which let "SELECT '--'; COPY ... PROGRAM" slip through. + // A commented-out PROGRAM statement is now blocked too, which is the safer default. + $scan = static fn (string $source): string => "{$source} | LC_ALL=C grep -Eiq {$escapedPsqlPattern} || {$source} | tr -d '~' | tr '\\n\\r' '~~' | LC_ALL=C grep -Eiq {$escapedSqlPattern}"; $customScan = $scan('pg_restore -f - "$inspect" 2>/dev/null'); $sqlScan = $scan($contents); - $blockedProgram = 'echo \'Blocked PostgreSQL restore: COPY ... PROGRAM and psql shell commands are not allowed.\'; exit 1'; + $blockedProgram = 'echo \'Blocked PostgreSQL restore: the backup contains a COPY ... PROGRAM, psql shell command, lo_import/lo_export, or ALTER SYSTEM pattern. If you trust this dump, restore it from the database container terminal.\'; exit 1'; $blockedInspect = 'echo \'Blocked PostgreSQL restore: unable to inspect custom archive.\'; exit 1'; return <<
@if ($resourceDbType === 'standalone-postgresql' && $dumpAll) - - The backup replaces PostgreSQL administrator role passwords, including the destination administrator password. - If the administrator password changes, update it in Coolify's database configuration after the restore. + + Restoring all databases needs a PostgreSQL superuser (it recreates databases and roles), so Coolify cannot restrict it. Only use a backup you trust. + The backup also replaces PostgreSQL administrator role passwords, including the destination administrator password. If the administrator password changes, update it in Coolify's database configuration after the restore. @endif ["COPY pwned FROM PROGRAM 'id';"], 'copy to program' => ["COPY pwned TO PROGRAM 'cat > /tmp/out';"], 'copy with block comment' => ["COPY pwned FROM/**/PROGRAM 'id';"], + 'copy with asterisks in block comment' => ["COPY pwned FROM/*a*b*/PROGRAM 'id';"], + 'copy with nested block comment' => ["COPY pwned FROM /* a /* b */ c */ PROGRAM 'id';"], + 'copy with line comment' => ["COPY pwned FROM--note\nPROGRAM 'id';"], + 'copy with mixed comments' => ["COPY pwned FROM /* -- */\n-- c\r\nPROGRAM 'id';"], 'psql shell command' => ["\\! id\n"], 'psql copy program' => ["\\copy pwned from program 'id'\n"], + 'psql shell command after statement' => ["SELECT 1; \\! id\n"], + 'psql write pipe' => ["\\w |id\n"], + 'program inside DO block' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;"], + 'program inside EXECUTE string' => ["DO \$\$ BEGIN EXECUTE 'COPY t TO PROGRAM ''id'''; END \$\$;"], + 'program inside function body' => ["CREATE FUNCTION f() RETURNS void AS 'COPY t TO PROGRAM ''id''' LANGUAGE sql;"], + 'copy with semicolon in string' => ["COPY (SELECT ';') TO PROGRAM 'id';"], + 'program after string holding comment' => ["SELECT '--'; COPY t TO PROGRAM 'id';"], + 'large object import' => ["SELECT lo_import('/etc/passwd');"], + 'alter system' => ["ALTER SYSTEM SET archive_command = 'id';"], ]); test('postgresql backup safety scanner allows ordinary sql dumps', function () { @@ -237,12 +251,36 @@ test('file scanner allows custom format archives without program execution', fun expect(DatabaseBackupFileValidator::fileContainsPostgresqlProgramExecution($archive))->toBeFalse(); }); -test('postgresql backup safety scanner allows copy words in table data', function () { - $dump = "COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n"; +test('postgresql backup safety scanner allows copy from stdin data', function () { + $dump = "COPY notes FROM stdin;\n1\tTaylor wrote a note\n\\.\n"; expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($dump))->toBeFalse(); }); +// Decided trade-off: an unanchored "TO/FROM PROGRAM" match cannot tell a real COPY +// directive from the same two words in a data row, and we keep it unanchored so it +// also catches dynamic SQL. A dump whose data literally contains "from program" or +// "to program" is therefore blocked, with an error that points to the container +// terminal for trusted dumps. This is rare in practice. +test('postgresql backup safety scanner blocks the program token pair even inside table data', function () { + $dump = "COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n"; + + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($dump))->toBeTrue(); +}); + +test('postgresql backup safety scanner blocks program execution hidden in dynamic sql', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeTrue(); +})->with([ + 'DO block' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;"], + 'EXECUTE string' => ["DO \$\$ BEGIN EXECUTE 'COPY t TO PROGRAM ''id'''; END \$\$;"], + 'function body' => ["CREATE FUNCTION f() RETURNS void AS 'COPY t TO PROGRAM ''id''' LANGUAGE sql;"], + 'semicolon in string' => ["COPY (SELECT ';') TO PROGRAM 'id';"], + 'string holding comment marker' => ["SELECT '--'; COPY t TO PROGRAM 'id';"], + 'lo_import' => ["SELECT lo_import('/etc/passwd');"], + 'alter system' => ["ALTER SYSTEM SET archive_command = 'id';"], + 'mid-statement psql shell' => ["SELECT 1; \\! id\n"], +]); + test('backup validator rejects plaintext .dump containing program execution', function () { $file = makeTemporaryUpload('evil.dump', "COPY x FROM PROGRAM 'id';\n"); @@ -258,13 +296,31 @@ test('remote postgresql scanner blocks bypass payloads', function (string $conte '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 with asterisks in block comment' => ["COPY x FROM/*a*b*/PROGRAM 'id';\n", false], + 'copy with nested block comment' => ["COPY x FROM /* a /* b */ c */ PROGRAM 'id';\n", false], + 'copy with line comment' => ["COPY x FROM--note\nPROGRAM 'id';\n", false], + 'copy with marker char in line comment' => ["COPY x FROM -- a~b\nPROGRAM 'id';\n", false], + 'copy with mixed comments' => ["COPY x FROM /* -- */\n-- c\r\nPROGRAM 'id';\n", false], + 'copy with block comment across lines' => ["COPY x FROM /* a\n-- */ PROGRAM 'id';\n", true], '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], 'psql query pipe redirect' => ["\\g | id\n", false], + 'psql write pipe redirect' => ["\\w |id\n", false], + 'psql shell after statement' => ["SELECT 1; \\! id\n", false], 'gzipped comment bypass' => ["COPY x FROM/**/PROGRAM 'id';\n", true], 'custom format archive' => ["PGDMP\0binary COPY records FROM PROGRAM payload", false], 'custom format gzip archive' => ["PGDMP\0binary COPY records FROM PROGRAM payload", true], + // Bypasses of the former (^|;) ... [^;]* COPY-only pattern. + 'program inside DO block' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;\n", false], + 'program inside EXECUTE string' => ["DO \$\$ BEGIN EXECUTE 'COPY t TO PROGRAM ''id'''; END \$\$;\n", false], + 'program inside function body' => ["CREATE FUNCTION f() RETURNS void AS 'COPY t TO PROGRAM ''id''' LANGUAGE sql;\n", false], + 'copy with semicolon in string' => ["COPY (SELECT ';') TO PROGRAM 'id';\n", false], + 'program after string holding comment' => ["SELECT '--'; COPY t TO PROGRAM 'id';\n", false], + 'large object import' => ["SELECT lo_import('/etc/passwd');\n", false], + 'alter system' => ["ALTER SYSTEM SET archive_command = 'id';\n", false], + 'commented out payload is now blocked' => ["-- COPY x FROM PROGRAM 'id'\nSELECT 1;\n", false], + 'program words in table data are blocked' => ["COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n", false], ]); test('remote postgresql scanner allows legitimate restores', function (string $content, bool $gzip) { @@ -273,11 +329,38 @@ test('remote postgresql scanner allows legitimate restores', function (string $c 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], - 'copy words in table data' => ["COPY notes FROM stdin;\n1\tcopy files from program storage\n\\.\n", false], + 'copy keyword without program' => ["COPY users (id, name) FROM stdin;\n1\tTaylor\n\\.\n", false], + 'program word alone in data' => ["COPY notes FROM stdin;\n1\tfinished the program today\n\\.\n", false], + 'comments between other words' => ["SELECT a FROM /* x */ t -- note\nWHERE b ~ 'program';\n", false], +]); + +test('the postgres safety command survives the non-root sudo parser', function (string $content, bool $blocked) { + $payload = writeScanPayload($content); + + $database = Mockery::mock(StandalonePostgresql::class); + $database->shouldReceive('getMorphClass')->andReturn(StandalonePostgresql::class); + + $server = Mockery::mock(Server::class)->makePartial(); + $server->user = 'ubuntu'; + + $command = (new DatabaseImportCommandBuilder)->buildPostgresSafetyCommand($database, 'scan-container', $payload); + $elevated = parseCommandsByLineForSudo(collect([$command]), $server)[0]; + + expect($elevated)->toStartWith('sudo bash -c ') + ->and($elevated)->not->toContain(' | sudo '); + + // Strip the host elevation and the container indirection so the embedded scan runs + // locally against the payload file; this proves the parser kept the script quoting intact. + $local = str_replace(['sudo bash -c', 'docker exec scan-container sh -c'], ['bash -c', 'sh -c'], $elevated); + + expect(Process::run(['sh', '-c', $local])->exitCode() === 1)->toBe($blocked); +})->with([ + 'blocks program payload' => ["COPY t TO PROGRAM 'id';\n", true], + 'blocks dynamic sql payload' => ["DO \$\$ BEGIN COPY t TO PROGRAM 'id'; END \$\$;\n", true], + 'allows a clean dump' => ["COPY users FROM stdin;\n1\tTaylor\n\\.\n", false], ]); test('remote postgresql scanner inspects custom archives instead of skipping them', function () { @@ -310,3 +393,47 @@ test('ALLOWED_EXTENSIONS does not include executable formats', function () { expect($constant)->not->toContain($bad); } }); + +// psql meta-commands can follow SQL on the same line, so they are matched anywhere after an +// odd backslash run. COPY data writes a literal backslash as "\\" and uses \n, \b, \v and \N, +// which psql does not know as commands, so ordinary dumps must still pass. +dataset('psql shell meta-commands', [ + 'shell escape after sql without semicolon' => ["SELECT 1 \\! id\n"], + 'output long name piped' => ["\\out |id\nSELECT 1;\n"], + 'output piped after sql' => ["SELECT 1 \\o |id\n"], + 'query piped with options' => ["SELECT 1 \\g (format=csv) |id\n"], + 'expanded query piped' => ["SELECT 1 \\gx |id\n"], + 'write long name piped' => ["\\write |id\n"], + 'copy program after sql' => ["SELECT 1 \\copy t from program 'id'\n"], + 'backquote in set' => ["\\set x `id`\n"], + 'backquote in short command after sql' => ["SELECT 1 \\t `id`\n"], + 'include file' => ["\\i /tmp/generated.sql\n"], + 'include relative file' => ["\\include_relative generated.sql\n"], + 'editor environment' => ["\\setenv PSQL_EDITOR id\n"], + 'editor' => ["\\e\n"], + 'shell escape after escaped backslash' => ["SELECT 1 \\\\\\! id\n"], +]); + +dataset('psql safe dump content', [ + 'escaped backslash before bang in copy data' => ["COPY notes FROM stdin;\n1\tC:\\\\! dir\n\\.\n"], + 'copy escapes before backquotes' => ["COPY notes FROM stdin;\n1\tline\\n `code` \\b `x` \\v `y`\t\\N\n\\.\n"], + 'copy hex and octal escapes' => ["COPY notes FROM stdin;\n1\t\\x41 \\101 |pipe\n\\.\n"], + 'pg_dumpall connect' => ["\\connect app\nSELECT 1;\n"], + 'pg_dump restrict keys' => ["\\restrict abc123\nSELECT 1;\n\\unrestrict abc123\n"], +]); + +test('postgresql backup safety scanner blocks psql shell meta-commands anywhere on a line', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeTrue(); +})->with('psql shell meta-commands'); + +test('postgresql backup safety scanner allows backslash sequences of ordinary dumps', function (string $payload) { + expect(DatabaseBackupFileValidator::containsPostgresqlProgramExecution($payload))->toBeFalse(); +})->with('psql safe dump content'); + +test('remote postgresql scanner blocks psql shell meta-commands anywhere on a line', function (string $payload) { + expect(scannerBlocks(postgresScanScript(writeScanPayload($payload))))->toBeTrue(); +})->with('psql shell meta-commands'); + +test('remote postgresql scanner allows backslash sequences of ordinary dumps', function (string $payload) { + expect(scannerBlocks(postgresScanScript(writeScanPayload($payload))))->toBeFalse(); +})->with('psql safe dump content'); diff --git a/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php b/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php index c6966c1706..c78c56d066 100644 --- a/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php +++ b/tests/Unit/DatabaseImport/DatabaseImportCommandBuilderTest.php @@ -75,7 +75,7 @@ test('builds dump-all commands and postgres safety scan', function () { ->toContain('unable to inspect custom archive') ->not->toContain('then exit 0') ->and($script) - ->toContain("tr '\\n\\r\\t'"); + ->toContain("tr '\\n\\r' '~~'"); }); test('restores dump-all PostgreSQL custom archives with pg_restore from the decompressed stream', function () { @@ -83,8 +83,8 @@ test('restores dump-all PostgreSQL custom archives with pg_restore from the deco // pg_restore cannot read gzip files, so it must receive the decompressed archive on stdin. expect($command) - ->toContain('stream | pg_restore -U "$pg_user" -d "$db"') - ->toContain('stream | psql -U "$pg_user" -d "$db"') + ->toContain('stream | pg_restore --exit-on-error -U "$pg_user" -d "$db"') + ->toContain('stream | psql -v ON_ERROR_STOP=1 -U "$pg_user" -d "$db"') ->not->toContain("-d \"\$db\" '/tmp/backup.dump.gz'"); });