diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index 723c6d4a5..54d5714a6 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -37,7 +37,7 @@ public static function ensureMultiplexedConnection(Server $server): bool if (data_get($server, 'settings.is_cloudflare_tunnel')) { $checkCommand .= '-o ProxyCommand="cloudflared access ssh --hostname %h" '; } - $checkCommand .= "{$server->user}@{$server->ip}"; + $checkCommand .= self::escapedUserAtHost($server); $process = Process::run($checkCommand); if ($process->exitCode() !== 0) { @@ -80,7 +80,7 @@ public static function establishNewMultiplexedConnection(Server $server): bool $establishCommand .= ' -o ProxyCommand="cloudflared access ssh --hostname %h" '; } $establishCommand .= self::getCommonSshOptions($server, $sshKeyLocation, $connectionTimeout, $serverInterval); - $establishCommand .= "{$server->user}@{$server->ip}"; + $establishCommand .= self::escapedUserAtHost($server); $establishProcess = Process::run($establishCommand); if ($establishProcess->exitCode() !== 0) { return false; @@ -101,7 +101,7 @@ public static function removeMuxFile(Server $server) if (data_get($server, 'settings.is_cloudflare_tunnel')) { $closeCommand .= '-o ProxyCommand="cloudflared access ssh --hostname %h" '; } - $closeCommand .= "{$server->user}@{$server->ip}"; + $closeCommand .= self::escapedUserAtHost($server); Process::run($closeCommand); // Clear connection metadata from cache @@ -141,9 +141,9 @@ public static function generateScpCommand(Server $server, string $source, string $scp_command .= self::getCommonSshOptions($server, $sshKeyLocation, config('constants.ssh.connection_timeout'), config('constants.ssh.server_interval'), isScp: true); if ($server->isIpv6()) { - $scp_command .= "{$source} {$server->user}@[{$server->ip}]:{$dest}"; + $scp_command .= "{$source} ".escapeshellarg($server->user).'@['.escapeshellarg($server->ip)."]:{$dest}"; } else { - $scp_command .= "{$source} {$server->user}@{$server->ip}:{$dest}"; + $scp_command .= "{$source} ".self::escapedUserAtHost($server).":{$dest}"; } return $scp_command; @@ -189,13 +189,18 @@ public static function generateSshCommand(Server $server, string $command, bool $delimiter = base64_encode($delimiter); $command = str_replace($delimiter, '', $command); - $ssh_command .= "{$server->user}@{$server->ip} 'bash -se' << \\$delimiter".PHP_EOL + $ssh_command .= self::escapedUserAtHost($server)." 'bash -se' << \\$delimiter".PHP_EOL .$command.PHP_EOL .$delimiter; return $ssh_command; } + private static function escapedUserAtHost(Server $server): string + { + return escapeshellarg($server->user).'@'.escapeshellarg($server->ip); + } + private static function isMultiplexingEnabled(): bool { return config('constants.ssh.mux_enabled') && ! config('constants.coolify.is_windows_docker_desktop'); @@ -224,9 +229,9 @@ private static function getCommonSshOptions(Server $server, string $sshKeyLocati // Bruh if ($isScp) { - $options .= "-P {$server->port} "; + $options .= '-P '.escapeshellarg((string) $server->port).' '; } else { - $options .= "-p {$server->port} "; + $options .= '-p '.escapeshellarg((string) $server->port).' '; } return $options; @@ -245,7 +250,7 @@ public static function isConnectionHealthy(Server $server): bool if (data_get($server, 'settings.is_cloudflare_tunnel')) { $healthCommand .= '-o ProxyCommand="cloudflared access ssh --hostname %h" '; } - $healthCommand .= "{$server->user}@{$server->ip} 'echo \"health_check_ok\"'"; + $healthCommand .= self::escapedUserAtHost($server)." 'echo \"health_check_ok\"'"; $process = Process::run($healthCommand); $isHealthy = $process->exitCode() === 0 && str_contains($process->output(), 'health_check_ok'); diff --git a/app/Http/Controllers/Api/ServersController.php b/app/Http/Controllers/Api/ServersController.php index 29c6b854a..892457925 100644 --- a/app/Http/Controllers/Api/ServersController.php +++ b/app/Http/Controllers/Api/ServersController.php @@ -11,6 +11,7 @@ use App\Models\PrivateKey; use App\Models\Project; use App\Models\Server as ModelsServer; +use App\Rules\ValidServerIp; use Illuminate\Http\Request; use OpenApi\Attributes as OA; use Stringable; @@ -472,10 +473,10 @@ public function create_server(Request $request) $validator = customApiValidator($request->all(), [ 'name' => 'string|max:255', 'description' => 'string|nullable', - 'ip' => 'string|required', - 'port' => 'integer|nullable', + 'ip' => ['string', 'required', new ValidServerIp], + 'port' => 'integer|nullable|between:1,65535', 'private_key_uuid' => 'string|required', - 'user' => 'string|nullable', + 'user' => ['string', 'nullable', 'regex:/^[a-zA-Z0-9_-]+$/'], 'is_build_server' => 'boolean|nullable', 'instant_validate' => 'boolean|nullable', 'proxy_type' => 'string|nullable', @@ -637,10 +638,10 @@ public function update_server(Request $request) $validator = customApiValidator($request->all(), [ 'name' => 'string|max:255|nullable', 'description' => 'string|nullable', - 'ip' => 'string|nullable', - 'port' => 'integer|nullable', + 'ip' => ['string', 'nullable', new ValidServerIp], + 'port' => 'integer|nullable|between:1,65535', 'private_key_uuid' => 'string|nullable', - 'user' => 'string|nullable', + 'user' => ['string', 'nullable', 'regex:/^[a-zA-Z0-9_-]+$/'], 'is_build_server' => 'boolean|nullable', 'instant_validate' => 'boolean|nullable', 'proxy_type' => 'string|nullable', diff --git a/app/Livewire/Server/New/ByIp.php b/app/Livewire/Server/New/ByIp.php index eecdfb4d0..51c6a06ee 100644 --- a/app/Livewire/Server/New/ByIp.php +++ b/app/Livewire/Server/New/ByIp.php @@ -5,6 +5,7 @@ use App\Enums\ProxyTypes; use App\Models\Server; use App\Models\Team; +use App\Rules\ValidServerIp; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Livewire\Attributes\Locked; @@ -55,8 +56,8 @@ protected function rules(): array 'new_private_key_value' => 'nullable|string', 'name' => ValidationPatterns::nameRules(), 'description' => ValidationPatterns::descriptionRules(), - 'ip' => 'required|string', - 'user' => 'required|string', + 'ip' => ['required', 'string', new ValidServerIp], + 'user' => ['required', 'string', 'regex:/^[a-zA-Z0-9_-]+$/'], 'port' => 'required|integer|between:1,65535', 'is_build_server' => 'required|boolean', ]; diff --git a/app/Livewire/Server/Show.php b/app/Livewire/Server/Show.php index 83c63a81c..edc17004c 100644 --- a/app/Livewire/Server/Show.php +++ b/app/Livewire/Server/Show.php @@ -7,6 +7,7 @@ use App\Events\ServerReachabilityChanged; use App\Models\CloudProviderToken; use App\Models\Server; +use App\Rules\ValidServerIp; use App\Services\HetznerService; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; @@ -106,9 +107,9 @@ protected function rules(): array return [ 'name' => ValidationPatterns::nameRules(), 'description' => ValidationPatterns::descriptionRules(), - 'ip' => 'required', - 'user' => 'required', - 'port' => 'required', + 'ip' => ['required', new ValidServerIp], + 'user' => ['required', 'regex:/^[a-zA-Z0-9_-]+$/'], + 'port' => 'required|integer|between:1,65535', 'validationLogs' => 'nullable', 'wildcardDomain' => 'nullable|url', 'isReachable' => 'required', diff --git a/app/Models/Server.php b/app/Models/Server.php index 49d9c3289..5099a9fec 100644 --- a/app/Models/Server.php +++ b/app/Models/Server.php @@ -913,6 +913,9 @@ public function port(): Attribute return Attribute::make( get: function ($value) { return (int) preg_replace('/[^0-9]/', '', $value); + }, + set: function ($value) { + return (int) preg_replace('/[^0-9]/', '', (string) $value); } ); } @@ -922,6 +925,9 @@ public function user(): Attribute return Attribute::make( get: function ($value) { return preg_replace('/[^A-Za-z0-9\-_]/', '', $value); + }, + set: function ($value) { + return preg_replace('/[^A-Za-z0-9\-_]/', '', $value); } ); } @@ -931,6 +937,9 @@ public function ip(): Attribute return Attribute::make( get: function ($value) { return preg_replace('/[^0-9a-zA-Z.:%-]/', '', $value); + }, + set: function ($value) { + return preg_replace('/[^0-9a-zA-Z.:%-]/', '', $value); } ); } diff --git a/app/Rules/ValidServerIp.php b/app/Rules/ValidServerIp.php new file mode 100644 index 000000000..270ff1c34 --- /dev/null +++ b/app/Rules/ValidServerIp.php @@ -0,0 +1,40 @@ +validate($attribute, $trimmed, function () use (&$failed) { + $failed = true; + }); + + if ($failed) { + $fail('The :attribute must be a valid IPv4 address, IPv6 address, or hostname.'); + } + } +} diff --git a/tests/Unit/SshCommandInjectionTest.php b/tests/Unit/SshCommandInjectionTest.php new file mode 100644 index 000000000..e7eeff62d --- /dev/null +++ b/tests/Unit/SshCommandInjectionTest.php @@ -0,0 +1,125 @@ +validate('ip', '192.168.1.1', function () use (&$failed) { + $failed = true; + }); + expect($failed)->toBeFalse(); +}); + +it('accepts a valid IPv6 address', function () { + $rule = new ValidServerIp; + $failed = false; + $rule->validate('ip', '2001:db8::1', function () use (&$failed) { + $failed = true; + }); + expect($failed)->toBeFalse(); +}); + +it('accepts a valid hostname', function () { + $rule = new ValidServerIp; + $failed = false; + $rule->validate('ip', 'my-server.example.com', function () use (&$failed) { + $failed = true; + }); + expect($failed)->toBeFalse(); +}); + +it('rejects injection payloads in server ip', function (string $payload) { + $rule = new ValidServerIp; + $failed = false; + $rule->validate('ip', $payload, function () use (&$failed) { + $failed = true; + }); + expect($failed)->toBeTrue("ValidServerIp should reject: $payload"); +})->with([ + 'semicolon' => ['192.168.1.1; rm -rf /'], + 'pipe' => ['192.168.1.1 | cat /etc/passwd'], + 'backtick' => ['192.168.1.1`id`'], + 'dollar subshell' => ['192.168.1.1$(id)'], + 'ampersand' => ['192.168.1.1 & id'], + 'newline' => ["192.168.1.1\nid"], + 'null byte' => ["192.168.1.1\0id"], +]); + +// ------------------------------------------------------------------------- +// Server model setter casts +// ------------------------------------------------------------------------- + +it('strips dangerous characters from server ip on write', function () { + $server = new App\Models\Server; + $server->ip = '192.168.1.1;rm -rf /'; + // Regex [^0-9a-zA-Z.:%-] removes ; space and /; hyphen is allowed + expect($server->ip)->toBe('192.168.1.1rm-rf'); +}); + +it('strips dangerous characters from server user on write', function () { + $server = new App\Models\Server; + $server->user = 'root$(id)'; + expect($server->user)->toBe('rootid'); +}); + +it('strips non-numeric characters from server port on write', function () { + $server = new App\Models\Server; + $server->port = '22; evil'; + expect($server->port)->toBe(22); +}); + +// ------------------------------------------------------------------------- +// escapeshellarg() in generated SSH commands (source-level verification) +// ------------------------------------------------------------------------- + +it('has escapedUserAtHost private static helper in SshMultiplexingHelper', function () { + $reflection = new ReflectionClass(SshMultiplexingHelper::class); + expect($reflection->hasMethod('escapedUserAtHost'))->toBeTrue(); + + $method = $reflection->getMethod('escapedUserAtHost'); + expect($method->isPrivate())->toBeTrue(); + expect($method->isStatic())->toBeTrue(); +}); + +it('wraps port with escapeshellarg in getCommonSshOptions', function () { + $reflection = new ReflectionClass(SshMultiplexingHelper::class); + $source = file_get_contents($reflection->getFileName()); + + expect($source)->toContain('escapeshellarg((string) $server->port)'); +}); + +it('has no raw user@ip string interpolation in SshMultiplexingHelper', function () { + $reflection = new ReflectionClass(SshMultiplexingHelper::class); + $source = file_get_contents($reflection->getFileName()); + + expect($source)->not->toContain('{$server->user}@{$server->ip}'); +}); + +// ------------------------------------------------------------------------- +// ValidHostname rejects shell metacharacters +// ------------------------------------------------------------------------- + +it('rejects semicolon in hostname', function () { + $rule = new ValidHostname; + $failed = false; + $rule->validate('hostname', 'example.com;id', function () use (&$failed) { + $failed = true; + }); + expect($failed)->toBeTrue(); +}); + +it('rejects backtick in hostname', function () { + $rule = new ValidHostname; + $failed = false; + $rule->validate('hostname', 'example.com`id`', function () use (&$failed) { + $failed = true; + }); + expect($failed)->toBeTrue(); +});