From a05d4e3a4b024719cda512244549fb5c949180c3 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 20 Apr 2026 21:26:34 +0200 Subject: [PATCH] fix(database): tighten Postgres init script filename handling Validate new init-script filenames against path traversal and shell metacharacters via a new validateFilenameSafe() helper, and harden the write/delete paths with basename() + escapeshellarg() so legacy rows still deploy and can be cleaned up without regressions. Co-Authored-By: Claude Opus 4.7 --- app/Actions/Database/StartPostgresql.php | 13 +- .../Project/Database/Postgresql/General.php | 23 ++- bootstrap/helpers/shared.php | 67 +++++++++ .../Unit/PostgresqlInitScriptSecurityTest.php | 66 +++++++++ tests/Unit/ValidateFilenameSafeTest.php | 138 ++++++++++++++++++ 5 files changed, 297 insertions(+), 10 deletions(-) create mode 100644 tests/Unit/ValidateFilenameSafeTest.php diff --git a/app/Actions/Database/StartPostgresql.php b/app/Actions/Database/StartPostgresql.php index 81cffeb94..52ff64ebf 100644 --- a/app/Actions/Database/StartPostgresql.php +++ b/app/Actions/Database/StartPostgresql.php @@ -301,9 +301,18 @@ private function generate_init_scripts() foreach ($this->database->init_scripts as $init_script) { $filename = data_get($init_script, 'filename'); $content = data_get($init_script, 'content'); + + // Normalise filename without rejecting legacy values so previously created + // init scripts keep deploying. basename() strips any directory components + // (path traversal) and escapeshellarg() contains every shell metacharacter + // in the tee target. Livewire / API validate new filenames up front. + $filename = basename((string) $filename); + + $target_path = "$this->configuration_dir/docker-entrypoint-initdb.d/{$filename}"; + $escaped_target = escapeshellarg($target_path); $content_base64 = base64_encode($content); - $this->commands[] = "echo '{$content_base64}' | base64 -d | tee $this->configuration_dir/docker-entrypoint-initdb.d/{$filename} > /dev/null"; - $this->init_scripts[] = "$this->configuration_dir/docker-entrypoint-initdb.d/{$filename}"; + $this->commands[] = "echo '{$content_base64}' | base64 -d | tee {$escaped_target} > /dev/null"; + $this->init_scripts[] = $target_path; } } diff --git a/app/Livewire/Project/Database/Postgresql/General.php b/app/Livewire/Project/Database/Postgresql/General.php index a9a4115fd..b5fb85483 100644 --- a/app/Livewire/Project/Database/Postgresql/General.php +++ b/app/Livewire/Project/Database/Postgresql/General.php @@ -358,9 +358,14 @@ public function save_init_script($script) if ($oldScript && $oldScript['filename'] !== $script['filename']) { try { - // Validate and escape filename to prevent command injection - validateShellSafePath($oldScript['filename'], 'init script filename'); - $old_file_path = "$configuration_dir/docker-entrypoint-initdb.d/{$oldScript['filename']}"; + // New filename is user-supplied — must be safe before accepting the rename. + validateFilenameSafe($script['filename'], 'init script filename'); + + // Old filename may be a legacy value written before this validation existed. + // basename() scopes the rm to the initdb.d directory; escapeshellarg() contains + // any remaining shell-metachars. No validator — don't block cleanup of legacy rows. + $old_filename = basename($oldScript['filename']); + $old_file_path = "$configuration_dir/docker-entrypoint-initdb.d/{$old_filename}"; $escapedOldPath = escapeshellarg($old_file_path); $delete_command = "rm -f {$escapedOldPath}"; instant_remote_process([$delete_command], $this->server); @@ -404,9 +409,11 @@ public function delete_init_script($script) $configuration_dir = database_configuration_dir().'/'.$container_name; try { - // Validate and escape filename to prevent command injection - validateShellSafePath($script['filename'], 'init script filename'); - $file_path = "$configuration_dir/docker-entrypoint-initdb.d/{$script['filename']}"; + // Allow deletion of legacy rows with unsafe filenames so operators can clean up. + // basename() scopes the rm to the initdb.d directory; escapeshellarg() keeps the + // shell invocation safe regardless of the stored value. + $safe_filename = basename($script['filename']); + $file_path = "$configuration_dir/docker-entrypoint-initdb.d/{$safe_filename}"; $escapedPath = escapeshellarg($file_path); $command = "rm -f {$escapedPath}"; @@ -443,8 +450,8 @@ public function save_new_init_script() ]); try { - // Validate filename to prevent command injection - validateShellSafePath($this->new_filename, 'init script filename'); + // Validate filename to prevent path traversal and command injection + validateFilenameSafe($this->new_filename, 'init script filename'); } catch (Exception $e) { $this->dispatch('error', $e->getMessage()); diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 77ca64fbd..881211513 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -157,6 +157,73 @@ function validateShellSafePath(string $input, string $context = 'path'): string return $input; } +/** + * Validate that a filename is safe for use as a plain file name (no path components). + * + * Prevents path traversal attacks by rejecting directory separators, traversal + * sequences, and null bytes, in addition to all shell metacharacters blocked by + * validateShellSafePath(). Intended for user-supplied filenames such as PostgreSQL + * init script names that are later written to a specific directory on the host. + * + * @param string $input The filename to validate + * @param string $context Descriptive name for error messages (e.g., 'init script filename') + * @return string The validated input (unchanged if valid) + * + * @throws Exception If dangerous characters or path traversal sequences are detected + */ +function validateFilenameSafe(string $input, string $context = 'filename'): string +{ + // First apply shell-metachar checks + validateShellSafePath($input, $context); + + // Reject NUL bytes (can be used to truncate path strings in some contexts) + if (str_contains($input, "\0")) { + throw new Exception( + "Invalid {$context}: contains null byte. ". + 'Null bytes are not allowed in filenames for security reasons.' + ); + } + + // Reject directory separators — filename must be a single path component + if (str_contains($input, '/') || str_contains($input, '\\')) { + throw new Exception( + "Invalid {$context}: directory separators ('/' or '\\') are not allowed. ". + 'Provide a plain filename without path components.' + ); + } + + // Reject path traversal sequences (catches encoded or unusual forms) + if (str_contains($input, '..')) { + throw new Exception( + "Invalid {$context}: path traversal sequence ('..') is not allowed." + ); + } + + // Reject shell globbing / expansion metacharacters and whitespace that would + // split the filename into additional shell arguments if ever interpolated + // unquoted (defence in depth on top of escapeshellarg() at call sites). + $shellExpansionChars = [ + ' ' => 'whitespace', + '*' => 'glob wildcard', + '?' => 'glob wildcard', + '[' => 'glob character class', + ']' => 'glob character class', + '~' => 'tilde expansion', + '"' => 'double quote', + "'" => 'single quote', + ]; + + foreach ($shellExpansionChars as $char => $description) { + if (str_contains($input, $char)) { + throw new Exception( + "Invalid {$context}: contains forbidden character '{$char}' ({$description})." + ); + } + } + + return $input; +} + /** * Validate that a databases_to_backup input string is safe from command injection. * diff --git a/tests/Unit/PostgresqlInitScriptSecurityTest.php b/tests/Unit/PostgresqlInitScriptSecurityTest.php index 4f74b13a4..2f85d1156 100644 --- a/tests/Unit/PostgresqlInitScriptSecurityTest.php +++ b/tests/Unit/PostgresqlInitScriptSecurityTest.php @@ -74,3 +74,69 @@ expect(fn () => validateShellSafePath('setup_db.sql', 'init script filename')) ->not->toThrow(Exception::class); }); + +// Path traversal — GHSA-mv4c-9x67-rrmv regression tests +test('postgresql init script rejects path traversal with ../ sequence', function () { + expect(fn () => validateFilenameSafe('../../../etc/cron.d/pwn', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script rejects path traversal targeting /etc/cron.d', function () { + expect(fn () => validateFilenameSafe('../../../../../etc/cron.d/k4zrce', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script rejects absolute path', function () { + expect(fn () => validateFilenameSafe('/etc/passwd', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script rejects filename with forward slash', function () { + expect(fn () => validateFilenameSafe('subdir/evil.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script rejects filename with backslash', function () { + expect(fn () => validateFilenameSafe('subdir\\evil.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script rejects double-dot without slashes', function () { + expect(fn () => validateFilenameSafe('..', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script rejects null byte injection', function () { + expect(fn () => validateFilenameSafe("init.sql\0../../etc/passwd", 'init script filename')) + ->toThrow(Exception::class); +}); + +test('postgresql init script accepts legitimate filenames via validateFilenameSafe', function () { + expect(fn () => validateFilenameSafe('init.sql', 'init script filename')) + ->not->toThrow(Exception::class); + + expect(fn () => validateFilenameSafe('01_schema.sql', 'init script filename')) + ->not->toThrow(Exception::class); + + expect(fn () => validateFilenameSafe('init-script.sh', 'init script filename')) + ->not->toThrow(Exception::class); +}); + +// Write-site defence — basename() + escapeshellarg() keep legacy/bad rows safe +test('basename() strips path traversal from legacy filenames at write site', function () { + expect(basename('../../../etc/cron.d/pwn'))->toBe('pwn'); + expect(basename('/etc/passwd'))->toBe('passwd'); + expect(basename('subdir/evil.sql'))->toBe('evil.sql'); +}); + +test('escapeshellarg() neutralises shell metacharacters in tee target', function () { + // Simulates how StartPostgresql::generate_init_scripts() builds the tee argument + $configuration_dir = '/data/coolify/databases/abc123'; + $legacy_filename = basename('foo bar*.sql;rm -rf /'); + $target = "$configuration_dir/docker-entrypoint-initdb.d/{$legacy_filename}"; + $escaped = escapeshellarg($target); + + // Single-quoted in POSIX sh means no expansion / no extra args regardless of contents. + expect($escaped)->toStartWith("'")->toEndWith("'"); + expect($escaped)->toContain('foo bar*.sql;rm -rf'); +}); diff --git a/tests/Unit/ValidateFilenameSafeTest.php b/tests/Unit/ValidateFilenameSafeTest.php new file mode 100644 index 000000000..012059e05 --- /dev/null +++ b/tests/Unit/ValidateFilenameSafeTest.php @@ -0,0 +1,138 @@ + validateFilenameSafe($name, 'init script filename')) + ->not->toThrow(Exception::class, "Expected '{$name}' to pass"); + } +}); + +test('rejects path traversal with ../', function () { + expect(fn () => validateFilenameSafe('../../../etc/cron.d/pwn', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects path traversal with .. alone', function () { + expect(fn () => validateFilenameSafe('..', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects path traversal embedded in filename', function () { + expect(fn () => validateFilenameSafe('foo..bar', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects forward slash directory separator', function () { + expect(fn () => validateFilenameSafe('foo/bar.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects backslash directory separator', function () { + expect(fn () => validateFilenameSafe('foo\\bar.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects absolute path starting with slash', function () { + expect(fn () => validateFilenameSafe('/etc/passwd', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects absolute Windows-style path', function () { + expect(fn () => validateFilenameSafe('C:\\Windows\\System32\\cmd.exe', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects null byte injection', function () { + expect(fn () => validateFilenameSafe("init.sql\0../../etc/passwd", 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects shell command substitution (inherits from validateShellSafePath)', function () { + expect(fn () => validateFilenameSafe('$(whoami).sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects backtick command substitution', function () { + expect(fn () => validateFilenameSafe('`id`.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects semicolon command separator', function () { + expect(fn () => validateFilenameSafe('init.sql;rm -rf /', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects pipe operator', function () { + expect(fn () => validateFilenameSafe('init.sql|whoami', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects redirect operators', function () { + expect(fn () => validateFilenameSafe('init.sql>/etc/passwd', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects mixed traversal and shell injection', function () { + expect(fn () => validateFilenameSafe('../etc/cron.d/$(id)', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('error message contains context string', function () { + try { + validateFilenameSafe('../evil', 'init script filename'); + expect(false)->toBeTrue('Should have thrown'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('init script filename'); + } +}); + +test('handles empty string without throwing', function () { + expect(fn () => validateFilenameSafe('', 'init script filename')) + ->not->toThrow(Exception::class); +}); + +test('rejects whitespace inside filename (would split into extra tee arg)', function () { + expect(fn () => validateFilenameSafe('foo bar.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects glob wildcards', function () { + expect(fn () => validateFilenameSafe('init*.sql', 'init script filename')) + ->toThrow(Exception::class); + + expect(fn () => validateFilenameSafe('init?.sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects glob character class brackets', function () { + expect(fn () => validateFilenameSafe('init[abc].sql', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects tilde expansion', function () { + expect(fn () => validateFilenameSafe('~/evil.sql', 'init script filename')) + ->toThrow(Exception::class); + + expect(fn () => validateFilenameSafe('~root', 'init script filename')) + ->toThrow(Exception::class); +}); + +test('rejects single and double quotes', function () { + expect(fn () => validateFilenameSafe("foo'bar.sql", 'init script filename')) + ->toThrow(Exception::class); + + expect(fn () => validateFilenameSafe('foo"bar.sql', 'init script filename')) + ->toThrow(Exception::class); +});