From 54b2e620ff2d9d518fed22ad03819fef10f28492 Mon Sep 17 00:00:00 2001 From: Marek Miklewicz Date: Tue, 30 Jun 2026 11:41:40 +0200 Subject: [PATCH] fix: address security review findings --- exec/handlers/login.php | 6 ++- exec/lib/DirectAdminContext.php | 21 ++++++++++- exec/lib/RateLimiter.php | 30 +++++++++++---- exec/lib/RemoteLoginUrlClient.php | 4 +- exec/lib/Settings.php | 32 +++++++++++++++- mx-helper/cleanup-expired-login-urls.php | 15 +++++++- mx-helper/mail-login-authorized-command.sh | 13 ++++--- tests/cleanup_test.php | 13 +++++++ tests/context_test.php | 43 +++++++++++++++++++++- tests/mx_helper_test.php | 11 +++--- tests/rate_limiter_test.php | 19 ++++++++++ tests/remote_login_url_client_test.php | 12 +++--- 12 files changed, 188 insertions(+), 31 deletions(-) create mode 100644 tests/rate_limiter_test.php diff --git a/exec/handlers/login.php b/exec/handlers/login.php index af385d0..c5081ef 100644 --- a/exec/handlers/login.php +++ b/exec/handlers/login.php @@ -28,7 +28,8 @@ return static function (): void { $clientIp = ClientIp::resolve($_SERVER, $settings); $auditBase['client_ip'] = $clientIp; - $correlation = KeyName::build($ctx->serverName(), $ctx->username(), time()); + $createdAt = time(); + $correlation = KeyName::build($ctx->serverName(), $ctx->username(), $createdAt); $auditBase['correlation'] = $correlation; $limiter = new RateLimiter(Settings::STATE_DIR . '/rate-limit'); @@ -43,7 +44,8 @@ return static function (): void { $settings->loginKeyTtl(), $settings->userIpBound(), $clientIp, - $redirectPath + $redirectPath, + $createdAt ); try { diff --git a/exec/lib/DirectAdminContext.php b/exec/lib/DirectAdminContext.php index de8ae69..8c3baf0 100644 --- a/exec/lib/DirectAdminContext.php +++ b/exec/lib/DirectAdminContext.php @@ -16,11 +16,11 @@ final class DirectAdminContext */ public static function fromServer(array $server, Settings $settings): self { - $username = trim((string)($server['USERNAME'] ?? $server['USER'] ?? '')); + $username = trim((string)($server['USERNAME'] ?? '')); if (!preg_match('/^[A-Za-z0-9][A-Za-z0-9._-]{0,31}$/', $username)) { throw new InvalidArgumentException('Invalid DirectAdmin username'); } - $host = Settings::normalizeHost((string)($server['HTTP_HOST'] ?? $server['SERVER_NAME'] ?? '')); + $host = self::trustedSourceHost($server, $settings); if ($host === '') { throw new InvalidArgumentException('Missing source host'); } @@ -34,6 +34,23 @@ final class DirectAdminContext return new self($username, $host, $serverName, strtoupper((string)($server['REQUEST_METHOD'] ?? 'GET'))); } + /** + * @param array $server + */ + private static function trustedSourceHost(array $server, Settings $settings): string + { + $configuredName = $settings->sourceServerName(); + if ($configuredName !== '') { + foreach ($settings->allowedSourceHosts() as $allowedHost) { + $label = $settings->sanitizeServerName(explode('.', $allowedHost)[0] ?? ''); + if ($label === $configuredName) { + return $allowedHost; + } + } + } + return Settings::normalizeHost((string)($server['SERVER_NAME'] ?? '')); + } + public function username(): string { return $this->username; diff --git a/exec/lib/RateLimiter.php b/exec/lib/RateLimiter.php index fee7376..5d9a65c 100644 --- a/exec/lib/RateLimiter.php +++ b/exec/lib/RateLimiter.php @@ -16,23 +16,37 @@ final class RateLimiter mkdir($this->dir, 0700, true); } $file = $this->dir . '/' . preg_replace('/[^A-Za-z0-9._-]/', '_', $bucket) . '.json'; + $handle = fopen($file, 'c+'); + if ($handle === false) { + throw new RuntimeException('Cannot open rate limit bucket'); + } + if (!flock($handle, LOCK_EX)) { + fclose($handle); + throw new RuntimeException('Cannot lock rate limit bucket'); + } $now = time(); $events = []; - if (is_file($file)) { - $decoded = json_decode((string)file_get_contents($file), true); - if (is_array($decoded)) { - foreach ($decoded as $event) { - if (is_int($event) && $event > $now - $windowSeconds) { - $events[] = $event; - } + $contents = stream_get_contents($handle); + $decoded = json_decode(is_string($contents) ? $contents : '', true); + if (is_array($decoded)) { + foreach ($decoded as $event) { + if (is_int($event) && $event > $now - $windowSeconds) { + $events[] = $event; } } } if (count($events) >= $limit) { + flock($handle, LOCK_UN); + fclose($handle); throw new RuntimeException('Rate limit exceeded'); } $events[] = $now; - file_put_contents($file, json_encode($events), LOCK_EX); + ftruncate($handle, 0); + rewind($handle); + fwrite($handle, json_encode($events) ?: '[]'); + fflush($handle); + flock($handle, LOCK_UN); + fclose($handle); @chmod($file, 0600); } } diff --git a/exec/lib/RemoteLoginUrlClient.php b/exec/lib/RemoteLoginUrlClient.php index 5137ff8..6811853 100644 --- a/exec/lib/RemoteLoginUrlClient.php +++ b/exec/lib/RemoteLoginUrlClient.php @@ -10,7 +10,8 @@ final class LoginUrlRequest public readonly int $ttl, public readonly bool $userIpBound, public readonly string $clientIp, - public readonly string $redirectPath + public readonly string $redirectPath, + public readonly int $createdAt ) { } } @@ -66,6 +67,7 @@ final class RemoteLoginUrlClient $request->userIpBound ? '1' : '0', $request->clientIp, $request->redirectPath, + (string)$request->createdAt, ]; } diff --git a/exec/lib/Settings.php b/exec/lib/Settings.php index 4044671..51d4eb3 100644 --- a/exec/lib/Settings.php +++ b/exec/lib/Settings.php @@ -153,7 +153,7 @@ final class Settings */ public function trustedProxies(): array { - return $this->csvHosts('MAIL_LOGIN_TRUSTED_PROXIES'); + return $this->csvNetworks('MAIL_LOGIN_TRUSTED_PROXIES'); } public function sshHost(): string @@ -226,6 +226,36 @@ final class Settings return array_values(array_unique($out)); } + /** + * @return list + */ + private function csvNetworks(string $key): array + { + $raw = $this->getString($key, ''); + if ($raw === '') { + return []; + } + $out = []; + foreach (explode(',', $raw) as $item) { + $item = trim($item); + if ($item === '') { + continue; + } + if (str_contains($item, '/')) { + [$ip, $prefix] = explode('/', $item, 2); + if (filter_var($ip, FILTER_VALIDATE_IP) && preg_match('/^[0-9]+$/', $prefix)) { + $out[] = $ip . '/' . $prefix; + } + } else { + $host = self::normalizeHost($item); + if ($host !== '') { + $out[] = $host; + } + } + } + return array_values(array_unique($out)); + } + private function validate(): void { $ttlRaw = $this->getString('LOGIN_KEY_TTL', ''); diff --git a/mx-helper/cleanup-expired-login-urls.php b/mx-helper/cleanup-expired-login-urls.php index 8f25740..e089910 100755 --- a/mx-helper/cleanup-expired-login-urls.php +++ b/mx-helper/cleanup-expired-login-urls.php @@ -21,7 +21,7 @@ function mailLoginCleanupMain(): void exit(2); } - $base = rtrim($config['base_url'], '/'); + $base = mailLoginCleanupValidateBaseUrl($config['base_url']); $now = time(); $states = mailLoginCleanupLoadStates($config['state_dir']); $entries = mailLoginCleanupApiRequest('GET', $base . '/api/login-keys/urls', $config); @@ -76,6 +76,9 @@ function mailLoginCleanupShouldDelete(array $entry, array $states, int $now): bo return false; } foreach ($states as $state) { + if ((string)($state['id'] ?? '') === '' || (string)$entry['id'] !== (string)$state['id']) { + continue; + } if (($state['status'] ?? '') !== 'created') { continue; } @@ -100,6 +103,16 @@ function mailLoginCleanupShouldDelete(array $entry, array $states, int $now): bo return false; } +function mailLoginCleanupValidateBaseUrl(string $baseUrl): string +{ + $baseUrl = rtrim($baseUrl, '/'); + $parts = parse_url($baseUrl); + if (!is_array($parts) || strtolower((string)($parts['scheme'] ?? '')) !== 'https' || empty($parts['host'])) { + throw new InvalidArgumentException('DA_BASE_URL must be an https URL'); + } + return $baseUrl; +} + /** * @param array $config * @return mixed diff --git a/mx-helper/mail-login-authorized-command.sh b/mx-helper/mail-login-authorized-command.sh index f7d6fd7..b594138 100755 --- a/mx-helper/mail-login-authorized-command.sh +++ b/mx-helper/mail-login-authorized-command.sh @@ -16,8 +16,8 @@ audit() { dir=$(dirname "$AUDIT_LOG") mkdir -p "$dir" chmod 700 "$dir" - printf '{"timestamp":"%s","event":"%s","result":"%s","correlation":"%s","username":"%s","source_host":"%s","client_ip":"%s"}\n' \ - "$(date -u '+%Y-%m-%dT%H:%M:%SZ')" "$1" "$2" "$3" "$4" "$5" "$6" >> "$AUDIT_LOG" + php -r '$fields=["timestamp"=>gmdate("c"),"event"=>$argv[1],"result"=>$argv[2],"correlation"=>$argv[3],"username"=>$argv[4],"source_host"=>$argv[5],"client_ip"=>$argv[6]]; echo json_encode($fields, JSON_UNESCAPED_SLASHES)."\n";' \ + "$1" "$2" "$3" "$4" "$5" "$6" >> "$AUDIT_LOG" chmod 600 "$AUDIT_LOG" } @@ -66,6 +66,7 @@ TTL="${5:-}" USER_IP_BOUND="${6:-}" CLIENT_IP="${7:-}" REDIRECT_PATH="${8:-}" +TIMESTAMP_ARG="${9:-}" [ "$COMMAND" = "mail-login-create-url" ] || fail printf '%s' "$USERNAME_ARG" | grep -Eq '^[A-Za-z0-9][A-Za-z0-9._-]{0,31}$' || fail @@ -87,10 +88,11 @@ case "$REDIRECT_PATH" in //*|*\\*) fail ;; esac printf '%s' "$REDIRECT_PATH" | grep -Eq '^/[A-Za-z0-9._~%/?=+-]*$' || fail +printf '%s' "$TIMESTAMP_ARG" | grep -Eq '^[0-9]+$' || fail SERVER_CLEAN=$(clean_label "$SERVER_NAME") USER_CLEAN=$(clean_label "$USERNAME_ARG") -TIMESTAMP=$(date '+%s') +TIMESTAMP="$TIMESTAMP_ARG" CORRELATION="autologin-$SERVER_CLEAN-$USER_CLEAN-$TIMESTAMP" if [ ${#CORRELATION} -gt 96 ]; then @@ -116,9 +118,10 @@ case "$URL" in https://*/api/login/url?key=*) ;; *) fail ;; esac +LOGIN_URL_ID=$(printf '%s\n' "$OUTPUT" | awk 'match($0,/HASHURL[A-Z0-9]+/) {print substr($0,RSTART,RLENGTH); exit}') -printf '{"timestamp":%s,"expires":%s,"correlation":"%s","username":"%s","source_host":"%s","client_ip":"%s","ttl":%s,"ip_bound":%s,"redirect_path":"%s","status":"created"}\n' \ - "$TIMESTAMP" "$((TIMESTAMP + TTL))" "$CORRELATION" "$USERNAME_ARG" "$SOURCE_HOST" "$CLIENT_IP" "$TTL" "$USER_IP_BOUND" "$REDIRECT_PATH" > "$STATE_DIR/$CORRELATION.json" +php -r '$fields=["timestamp"=>(int)$argv[1],"expires"=>(int)$argv[2],"correlation"=>$argv[3],"id"=>$argv[4],"username"=>$argv[5],"source_host"=>$argv[6],"client_ip"=>$argv[7],"ttl"=>(int)$argv[8],"ip_bound"=>(int)$argv[9],"redirect_path"=>$argv[10],"status"=>"created"]; echo json_encode($fields, JSON_UNESCAPED_SLASHES)."\n";' \ + "$TIMESTAMP" "$((TIMESTAMP + TTL))" "$CORRELATION" "$LOGIN_URL_ID" "$USERNAME_ARG" "$SOURCE_HOST" "$CLIENT_IP" "$TTL" "$USER_IP_BOUND" "$REDIRECT_PATH" > "$STATE_DIR/$CORRELATION.json" chmod 600 "$STATE_DIR/$CORRELATION.json" audit "create_login_url" "success" "$CORRELATION" "$USERNAME_ARG" "$SOURCE_HOST" "$CLIENT_IP" printf 'URL: %s\n' "$URL" diff --git a/tests/cleanup_test.php b/tests/cleanup_test.php index 9bbb04a..86a8b35 100644 --- a/tests/cleanup_test.php +++ b/tests/cleanup_test.php @@ -8,6 +8,7 @@ test('cleanup deletes only expired login urls matching local mail-login state', 'timestamp' => 1782810000, 'expires' => 1782810030, 'correlation' => 'autologin-h4-mxtest-1782810000', + 'id' => 'HASHURLMATCH', 'redirect_path' => '/', 'client_ip' => '198.51.100.10', 'ip_bound' => 1, @@ -31,5 +32,17 @@ test('cleanup deletes only expired login urls matching local mail-login state', assert_true(mailLoginCleanupShouldDelete($matching, [$state], strtotime('2026-06-30T10:01:00Z'))); assert_false(mailLoginCleanupShouldDelete($wrongRedirect, [$state], strtotime('2026-06-30T10:01:00Z'))); assert_false(mailLoginCleanupShouldDelete($notExpired, [$state], strtotime('2026-06-30T10:01:00Z'))); + + $stateWithoutId = $state; + unset($stateWithoutId['id']); + assert_false(mailLoginCleanupShouldDelete($matching, [$stateWithoutId], strtotime('2026-06-30T10:01:00Z'))); }); +test('cleanup rejects non-https directadmin api base url', function (): void { + try { + mailLoginCleanupValidateBaseUrl('http://127.0.0.1:2222'); + throw new RuntimeException('Expected http base URL to fail'); + } catch (InvalidArgumentException $e) { + assert_contains('https', strtolower($e->getMessage())); + } +}); diff --git a/tests/context_test.php b/tests/context_test.php index b53ea40..a2cb07a 100644 --- a/tests/context_test.php +++ b/tests/context_test.php @@ -12,6 +12,7 @@ test('directadmin context derives server name from first host label', function ( $ctx = DirectAdminContext::fromServer([ 'USERNAME' => 'mxtest', 'HTTP_HOST' => 'https://h4.domena.pl:2222', + 'SERVER_NAME' => 'h4.domena.pl', 'REMOTE_ADDR' => '198.51.100.10', 'REQUEST_METHOD' => 'GET', ], $settings); @@ -21,6 +22,34 @@ test('directadmin context derives server name from first host label', function ( assert_same('h4', $ctx->serverName()); }); +test('directadmin context prefers trusted configured source server name over host header', function (): void { + $settings = Settings::fromArray(['MAIL_LOGIN_SOURCE_SERVER_NAME' => 'h4']); + $ctx = DirectAdminContext::fromServer([ + 'USERNAME' => 'mxtest', + 'HTTP_HOST' => 'evil.example.net:2222', + 'SERVER_NAME' => 'h4.domena.pl', + 'REMOTE_ADDR' => '198.51.100.10', + 'REQUEST_METHOD' => 'GET', + ], $settings); + + assert_same('h4.domena.pl', $ctx->sourceHost()); + assert_same('h4', $ctx->serverName()); +}); + +test('directadmin context fails closed without directadmin username', function (): void { + try { + DirectAdminContext::fromServer([ + 'USER' => 'root', + 'HTTP_HOST' => 'h4.domena.pl:2222', + 'REMOTE_ADDR' => '198.51.100.10', + 'REQUEST_METHOD' => 'GET', + ], Settings::fromArray([])); + throw new RuntimeException('Expected missing USERNAME to fail'); + } catch (InvalidArgumentException $e) { + assert_contains('username', strtolower($e->getMessage())); + } +}); + test('directadmin context rejects invalid usernames', function (): void { try { DirectAdminContext::fromServer([ @@ -58,6 +87,19 @@ test('client ip honors forwarded header only from trusted proxy', function (): v assert_same('203.0.113.20', $ip); }); +test('client ip supports trusted proxy cidr ranges', function (): void { + $settings = Settings::fromArray([ + 'MAIL_LOGIN_CLIENT_IP_MODE' => 'trusted_proxy', + 'MAIL_LOGIN_TRUSTED_PROXIES' => '10.0.0.0/24', + ]); + $ip = ClientIp::resolve([ + 'REMOTE_ADDR' => '10.0.0.25', + 'HTTP_X_FORWARDED_FOR' => '203.0.113.20', + ], $settings); + + assert_same('203.0.113.20', $ip); +}); + test('source authorizer rejects hosts outside allowlist', function (): void { $settings = Settings::fromArray(['MAIL_LOGIN_ALLOWED_SOURCE_HOSTS' => 'h4.domena.pl']); @@ -79,4 +121,3 @@ test('key name uses requested format and truncates long values deterministically assert_true(str_starts_with($long, 'autologin-')); assert_true((bool)preg_match('/-[a-f0-9]{10}-1782810000$/', $long), 'Long key should contain hash suffix'); }); - diff --git a/tests/mx_helper_test.php b/tests/mx_helper_test.php index c89a381..506e6df 100644 --- a/tests/mx_helper_test.php +++ b/tests/mx_helper_test.php @@ -18,7 +18,7 @@ test('mx helper conditionally passes user ip binding to da login-url', function escapeshellarg($state), escapeshellarg('h4.domena.pl'), escapeshellarg($script), - 'mail-login-create-url mxtest h4.domena.pl h4 30 1 198.51.100.10 /' + 'mail-login-create-url mxtest h4.domena.pl h4 30 1 198.51.100.10 / 1782810000' ); exec($cmd, $out, $code); assert_same(0, $code, implode("\n", $out)); @@ -33,7 +33,7 @@ test('mx helper conditionally passes user ip binding to da login-url', function escapeshellarg($state), escapeshellarg('h4.domena.pl'), escapeshellarg($script), - 'mail-login-create-url mxtest h4.domena.pl h4 30 0 198.51.100.10 /' + 'mail-login-create-url mxtest h4.domena.pl h4 30 0 198.51.100.10 / 1782810001' ); exec($cmdNoIp, $outNoIp, $codeNoIp); assert_same(0, $codeNoIp, implode("\n", $outNoIp)); @@ -57,7 +57,7 @@ test('mx helper accepts forced command arguments from ssh original command', fun escapeshellarg($audit), escapeshellarg($state), escapeshellarg('h4.domena.pl'), - escapeshellarg('mail-login-create-url mxtest h4.domena.pl evil 30 1 198.51.100.10 /'), + escapeshellarg('mail-login-create-url mxtest h4.domena.pl evil 30 1 198.51.100.10 / 1782810002'), escapeshellarg($script) ); exec($cmd, $out, $code); @@ -83,7 +83,7 @@ test('mx helper rejects disallowed source hosts and invalid client ips', functio escapeshellarg($state), escapeshellarg('h4.domena.pl'), escapeshellarg($script), - 'mail-login-create-url mxtest evil.domena.pl evil 30 1 198.51.100.10 /' + 'mail-login-create-url mxtest evil.domena.pl evil 30 1 198.51.100.10 / 1782810003' ); exec($badHost, $outHost, $codeHost); assert_same(1, $codeHost); @@ -95,8 +95,9 @@ test('mx helper rejects disallowed source hosts and invalid client ips', functio escapeshellarg($state), escapeshellarg('h4.domena.pl'), escapeshellarg($script), - 'mail-login-create-url mxtest h4.domena.pl h4 30 1 999.999.999.999 /' + 'mail-login-create-url mxtest h4.domena.pl h4 30 1 999.999.999.999 / 1782810004' ); exec($badIp, $outIp, $codeIp); assert_same(1, $codeIp); + assert_not_contains("\nBROKEN", file_get_contents($audit) ?: ''); }); diff --git a/tests/rate_limiter_test.php b/tests/rate_limiter_test.php new file mode 100644 index 0000000..0b8cf27 --- /dev/null +++ b/tests/rate_limiter_test.php @@ -0,0 +1,19 @@ +assertAllowed('user-mxtest', 2, 60); + $limiter->assertAllowed('user-mxtest', 2, 60); + + try { + $limiter->assertAllowed('user-mxtest', 2, 60); + throw new RuntimeException('Expected third request to be rate limited'); + } catch (RuntimeException $e) { + assert_contains('rate limit', strtolower($e->getMessage())); + } +}); + diff --git a/tests/remote_login_url_client_test.php b/tests/remote_login_url_client_test.php index a3164f0..6540c97 100644 --- a/tests/remote_login_url_client_test.php +++ b/tests/remote_login_url_client_test.php @@ -15,20 +15,22 @@ test('remote login url client builds fixed ssh command with positional arguments 'LOGIN_KEY_TTL' => '30', 'USER_IP_BOUND' => '1', ]); - $request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/'); + $request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/', 1782810000); $command = RemoteLoginUrlClient::buildCommand($settings, $request); assert_same('ssh', $command[0]); assert_contains('UserKnownHostsFile=/secure/known_hosts', implode(' ', $command)); assert_contains('root@mx1.domena.pl', implode(' ', $command)); - assert_same('mail-login-create-url', $command[count($command) - 8]); - assert_same('mxtest', $command[count($command) - 7]); - assert_same('1', $command[count($command) - 3]); + $pos = array_search('mail-login-create-url', $command, true); + assert_true(is_int($pos), 'Command marker missing'); + assert_same('mxtest', $command[$pos + 1]); + assert_same('1', $command[$pos + 5]); + assert_same('1782810000', $command[$pos + 8]); }); test('remote login url client validates helper output before returning it', function (): void { $settings = Settings::fromArray(['MAIL_LOGIN_TARGET_BASE_URL' => 'https://mx1.domena.pl:2222']); - $request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/'); + $request = new LoginUrlRequest('mxtest', 'h4.domena.pl', 'h4', 30, true, '198.51.100.10', '/', 1782810000); $client = new RemoteLoginUrlClient(function (array $command, int $timeout): ProcessResult { return new ProcessResult(0, "URL: https://mx1.domena.pl:2222/api/login/url?key=SECRET\n", '');