Quellcode durchsuchen

fix: reject weak local-admin password hash at UI boot (SEC_REVIEW F37)

`LocalLoginController` accepted whatever hash was in the env without
checking the algorithm. An operator who set
`LOCAL_ADMIN_PASSWORD_HASH='$2y$04$…'` (bcrypt cost 4) or a legacy
`$1$…` md5-crypt string would have those accepted at runtime via
`password_verify` — but a four-round bcrypt hash falls to a commodity
GPU in seconds and md5-crypt is broken outright. The local-admin
account is the break-glass admin path; if its hash is exfiltrated
through any side channel (env dump, container snapshot, backup
inspection) it should at least be expensive to crack.

Extend `App\App\Config::validateOrExit` with a `password_get_info()`
check on the hash. The api refuses to boot unless `LOCAL_ADMIN_ENABLED`
is false OR the hash is Argon2id (preferred) or bcrypt with cost ≥ 12
(new `Config::BCRYPT_MIN_COST` constant). Anything else — argon2i,
bcrypt cost < 12, md5-crypt, sha256-crypt, plain text, base64 noise —
collapses into a rejection with a human-readable message pointing the
operator at `password_hash('…', PASSWORD_ARGON2ID)`. The `collectErrors`
extraction (mirroring the api side from F35) keeps the validator
unit-testable without spawning a process.

Tests in `ui/tests/Unit/App/ConfigTest.php` cover Argon2id-accept,
bcrypt-cost-12-accept, bcrypt-cost-4-reject, argon2i-reject,
md5-crypt-reject, plain-string-reject, empty-string-when-local-enabled-
reject, and the "local-admin disabled → hash not checked" path so
OIDC-only deployments don't have to invent a strong dummy hash. Test
fixtures (`AppTestCase::bootApp`) already use `PASSWORD_ARGON2ID` and
the test path goes through `Bootstrap::container()` which skips the
validator, so this change doesn't perturb the integration suite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa vor 4 Tagen
Ursprung
Commit
f2a81ad611
2 geänderte Dateien mit 185 neuen und 7 gelöschten Zeilen
  1. 76 7
      ui/src/App/Config.php
  2. 109 0
      ui/tests/Unit/App/ConfigTest.php

+ 76 - 7
ui/src/App/Config.php

@@ -14,10 +14,40 @@ namespace App\App;
  */
 final class Config
 {
+    /**
+     * Minimum bcrypt cost accepted for `LOCAL_ADMIN_PASSWORD_HASH` (SEC_REVIEW
+     * F37). Below this, an attacker who exfiltrates the hash can crack it on
+     * commodity hardware in tractable time. Argon2id is preferred over
+     * bcrypt at any cost; this floor only matters for operators who insist
+     * on bcrypt for legacy reasons.
+     */
+    public const BCRYPT_MIN_COST = 12;
+
     /**
      * @param array<string, mixed> $settings
      */
     public static function validateOrExit(array $settings): void
+    {
+        $errors = self::collectErrors($settings);
+        if ($errors === []) {
+            return;
+        }
+
+        fwrite(STDERR, "[ui] startup configuration error(s):\n");
+        foreach ($errors as $err) {
+            fwrite(STDERR, "  - {$err}\n");
+        }
+        exit(1);
+    }
+
+    /**
+     * Pure validation — returns a list of human-readable error messages.
+     * Public so unit tests can exercise it without spawning a process.
+     *
+     * @param array<string, mixed> $settings
+     * @return list<string>
+     */
+    public static function collectErrors(array $settings): array
     {
         $errors = [];
 
@@ -38,8 +68,14 @@ final class Config
             if (($settings['local_admin_username'] ?? '') === '') {
                 $errors[] = 'LOCAL_ADMIN_USERNAME is empty but LOCAL_ADMIN_ENABLED=true';
             }
-            if (($settings['local_admin_password_hash'] ?? '') === '') {
+            $hash = (string) ($settings['local_admin_password_hash'] ?? '');
+            if ($hash === '') {
                 $errors[] = 'LOCAL_ADMIN_PASSWORD_HASH is empty but LOCAL_ADMIN_ENABLED=true';
+            } else {
+                $algoError = self::validatePasswordHashAlgorithm($hash);
+                if ($algoError !== null) {
+                    $errors[] = $algoError;
+                }
             }
         }
         if ($oidcEnabled) {
@@ -50,14 +86,47 @@ final class Config
             }
         }
 
-        if ($errors === []) {
-            return;
+        return $errors;
+    }
+
+    /**
+     * SEC_REVIEW F37: refuse to boot if the local-admin password hash
+     * isn't Argon2id (preferred) or bcrypt with cost ≥ 12. Returns null
+     * when the hash is acceptable, otherwise a human-readable error.
+     *
+     * `password_get_info()` reports `algoName = 'unknown'` for anything
+     * the password_hash family doesn't recognise (md5-crypt, sha256-crypt,
+     * plain strings, base64 noise) — those all collapse into the
+     * not-allowed branch with the same message.
+     */
+    private static function validatePasswordHashAlgorithm(string $hash): ?string
+    {
+        $info = password_get_info($hash);
+        $algoName = isset($info['algoName']) && is_string($info['algoName'])
+            ? $info['algoName']
+            : 'unknown';
+
+        if ($algoName === 'argon2id') {
+            return null;
         }
+        if ($algoName === 'bcrypt') {
+            $options = is_array($info['options'] ?? null) ? $info['options'] : [];
+            $cost = isset($options['cost']) && is_int($options['cost']) ? $options['cost'] : 0;
+            if ($cost >= self::BCRYPT_MIN_COST) {
+                return null;
+            }
 
-        fwrite(STDERR, "[ui] startup configuration error(s):\n");
-        foreach ($errors as $err) {
-            fwrite(STDERR, "  - {$err}\n");
+            return sprintf(
+                'LOCAL_ADMIN_PASSWORD_HASH uses bcrypt with cost=%d; must be ≥ %d (regenerate with: php -r "echo password_hash(\'…\', PASSWORD_ARGON2ID);")',
+                $cost,
+                self::BCRYPT_MIN_COST,
+            );
         }
-        exit(1);
+
+        return sprintf(
+            'LOCAL_ADMIN_PASSWORD_HASH algorithm "%s" is not allowed; use Argon2id (preferred) or bcrypt cost ≥ %d',
+            $algoName,
+            self::BCRYPT_MIN_COST,
+        );
     }
 }

+ 109 - 0
ui/tests/Unit/App/ConfigTest.php

@@ -0,0 +1,109 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Unit\App;
+
+use App\App\Config;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Boot-time configuration validation.
+ *
+ * SEC_REVIEW F37: `LOCAL_ADMIN_PASSWORD_HASH` must be Argon2id or bcrypt
+ * cost ≥ 12. Anything weaker — bcrypt cost 4, plain text, md5-crypt,
+ * argon2i — is rejected at boot so a misconfigured deployment crashes
+ * on `docker compose up` instead of accepting hashed-but-cracked
+ * passwords on the local-admin path.
+ */
+final class ConfigTest extends TestCase
+{
+    public function testValidArgon2idIsAccepted(): void
+    {
+        $hash = password_hash('test', PASSWORD_ARGON2ID);
+        self::assertSame([], Config::collectErrors($this->localBaseSettings($hash)));
+    }
+
+    public function testValidBcryptAtMinimumCostIsAccepted(): void
+    {
+        $hash = password_hash('test', PASSWORD_BCRYPT, ['cost' => Config::BCRYPT_MIN_COST]);
+        self::assertSame([], Config::collectErrors($this->localBaseSettings($hash)));
+    }
+
+    public function testBcryptBelowMinimumCostIsRejected(): void
+    {
+        $hash = password_hash('test', PASSWORD_BCRYPT, ['cost' => 4]);
+        $errors = Config::collectErrors($this->localBaseSettings($hash));
+        self::assertNotEmpty($errors);
+        $joined = implode("\n", $errors);
+        self::assertStringContainsString('cost=4', $joined);
+        self::assertStringContainsString('LOCAL_ADMIN_PASSWORD_HASH', $joined);
+    }
+
+    public function testArgon2iIsRejected(): void
+    {
+        if (!defined('PASSWORD_ARGON2I')) {
+            self::markTestSkipped('argon2i not built into this PHP');
+        }
+        $hash = password_hash('test', PASSWORD_ARGON2I);
+        $errors = Config::collectErrors($this->localBaseSettings($hash));
+        self::assertNotEmpty($errors);
+        self::assertStringContainsString('argon2i', implode("\n", $errors));
+    }
+
+    public function testMd5CryptIsRejected(): void
+    {
+        // crypt() with $1$ prefix produces md5-crypt, which password_get_info
+        // reports as 'unknown'. The literal string SEC_REVIEW called out.
+        $hash = '$1$salt1234$KdyIvFMZJKR1qDJ5qE5W31';
+        $errors = Config::collectErrors($this->localBaseSettings($hash));
+        self::assertNotEmpty($errors);
+        self::assertStringContainsString('"unknown"', implode("\n", $errors));
+    }
+
+    public function testPlainStringIsRejected(): void
+    {
+        $errors = Config::collectErrors($this->localBaseSettings('not-a-hash-at-all'));
+        self::assertNotEmpty($errors);
+    }
+
+    public function testEmptyHashIsStillRejectedWhenLocalEnabled(): void
+    {
+        $errors = Config::collectErrors($this->localBaseSettings(''));
+        self::assertNotEmpty($errors);
+        self::assertStringContainsString('empty', implode("\n", $errors));
+    }
+
+    public function testHashIsNotValidatedWhenLocalAdminDisabled(): void
+    {
+        // Operators who only run OIDC don't need a strong dummy hash.
+        $settings = [
+            'ui_service_token' => 'irdb_svc_AAAA',
+            'api_base_url' => 'http://api:8081',
+            'oidc_enabled' => true,
+            'oidc_issuer' => 'https://issuer',
+            'oidc_client_id' => 'cid',
+            'oidc_client_secret' => 'csec',
+            'oidc_redirect_uri' => 'https://r/cb',
+            'local_admin_enabled' => false,
+            'local_admin_username' => '',
+            'local_admin_password_hash' => 'this-would-fail-if-checked',
+        ];
+        self::assertSame([], Config::collectErrors($settings));
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    private function localBaseSettings(string $hash): array
+    {
+        return [
+            'ui_service_token' => 'irdb_svc_AAAA',
+            'api_base_url' => 'http://api:8081',
+            'oidc_enabled' => false,
+            'local_admin_enabled' => true,
+            'local_admin_username' => 'admin',
+            'local_admin_password_hash' => $hash,
+        ];
+    }
+}