ソースを参照

fix: rate-limit /login/local even when local admin is disabled (SEC_REVIEW F38)

`LocalLoginController::postLocal` returned 404 immediately when
`LOCAL_ADMIN_ENABLED=false`, before the LoginThrottle gate. An
attacker hammering this URL on an OIDC-only deployment burned worker
threads at line rate — a cheap DoS lever the rest of the throttle
machinery did nothing about, because the 404 short-circuit ran first.

Move the throttle gate above the disabled-path 404 and record a
failure on every hit. The bucket key is `('', source_ip)` — an empty
username sentinel — so a rotating-username spray from one IP folds
into one per-IP bucket and trips the existing 5/10/15 ladder
regardless of what `username` field the attacker submits. Once
locked, additional hits skip `recordFailure` so the throttle file
size is bounded by the lockout ladder, not by attacker request
volume. The 404 status is preserved on both branches so the response
doesn't leak the lockout state to a probing attacker.

Regression tests in `ui/tests/Integration/Auth/LocalLoginTest.php`:

  - `testDisabledLocalAdminRecordsThrottleFailure` posts five times
    with rotating username values from one IP and asserts the per-IP
    bucket trips after the fifth hit.
  - `testDisabledLocalAdminLockedHitDoesNotIncrementBucket` floods 50
    more hits after lockout and asserts the lockout window doesn't
    extend (the count freezes at the trigger threshold).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 日 前
コミット
d37890b68f

+ 12 - 1
ui/src/Auth/LocalLoginController.php

@@ -68,14 +68,25 @@ final class LocalLoginController
 
     public function postLocal(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface
     {
+        $sourceIp = self::extractSourceIp($request);
+
+        // SEC_REVIEW F38: rate-limit hits to /login/local even when the
+        // path is disabled, so an attacker can't burn worker threads
+        // hammering this URL. Normalising to an empty username folds all
+        // hits from one source IP into one per-IP bucket regardless of
+        // what username field the attacker happens to submit, so the
+        // existing 5/10/15 ladder applies to a rotating-username spray.
         if (!$this->localAdminEnabled) {
+            if (!$this->throttle->isLocked('', $sourceIp)) {
+                $this->throttle->recordFailure('', $sourceIp);
+            }
+
             return $this->responseFactory->createResponse(404);
         }
 
         $body = $request->getParsedBody();
         $username = is_array($body) && isset($body['username']) && is_string($body['username']) ? $body['username'] : '';
         $password = is_array($body) && isset($body['password']) && is_string($body['password']) ? $body['password'] : '';
-        $sourceIp = self::extractSourceIp($request);
 
         // Throttle gate is keyed by (username, source_ip). An empty
         // username still gets a bucket so blank-form spamming is rate-limited.

+ 96 - 0
ui/tests/Integration/Auth/LocalLoginTest.php

@@ -147,6 +147,102 @@ final class LocalLoginTest extends AppTestCase
         );
     }
 
+    public function testDisabledLocalAdminRecordsThrottleFailure(): void
+    {
+        // SEC_REVIEW F38: hitting POST /login/local when local-admin is
+        // disabled returned 404 with no rate-limit. An attacker could
+        // burn worker threads on this URL. The throttle must accumulate
+        // a per-IP failure for each disabled-path hit so the existing
+        // 5/10/15 ladder applies even on the disabled URL.
+        $this->bootApp(['local_admin_enabled' => false]);
+        $this->request('GET', '/login');
+        $token = (string) ($_SESSION[CsrfMiddleware::SESSION_KEY] ?? '');
+
+        $body = http_build_query([
+            'csrf_token' => $token,
+            'username' => 'whatever',
+            'password' => 'spam',
+        ]);
+        $response = $this->request(
+            'POST',
+            '/login/local',
+            [],
+            $body,
+            'application/x-www-form-urlencoded',
+            ['REMOTE_ADDR' => '198.51.100.50'],
+        );
+
+        self::assertSame(404, $response->getStatusCode());
+        // The per-IP bucket key is `('', source_ip)` — independent of the
+        // attacker's submitted username so a rotating-username spray from
+        // one IP all lands in the same bucket.
+        /** @var LoginThrottle $throttle */
+        $throttle = $this->container->get(LoginThrottle::class);
+        self::assertFalse($throttle->isLocked('', '198.51.100.50'));
+        // Five hits should trip the lockout regardless of submitted username.
+        for ($i = 0; $i < 4; ++$i) {
+            $varied = http_build_query([
+                'csrf_token' => $token,
+                'username' => 'rotating-' . $i,
+                'password' => 'spam',
+            ]);
+            $this->request(
+                'POST',
+                '/login/local',
+                [],
+                $varied,
+                'application/x-www-form-urlencoded',
+                ['REMOTE_ADDR' => '198.51.100.50'],
+            );
+        }
+        self::assertTrue($throttle->isLocked('', '198.51.100.50'));
+    }
+
+    public function testDisabledLocalAdminLockedHitDoesNotIncrementBucket(): void
+    {
+        // After lockout, additional hammering must not grow the bucket
+        // unbounded — we want the throttle file size + decode cost to be
+        // bounded by the lockout ladder, not by attacker request volume.
+        $this->bootApp(['local_admin_enabled' => false]);
+        $this->request('GET', '/login');
+        $token = (string) ($_SESSION[CsrfMiddleware::SESSION_KEY] ?? '');
+
+        $body = http_build_query([
+            'csrf_token' => $token,
+            'username' => 'x',
+            'password' => 'x',
+        ]);
+        for ($i = 0; $i < 5; ++$i) {
+            $this->request(
+                'POST',
+                '/login/local',
+                [],
+                $body,
+                'application/x-www-form-urlencoded',
+                ['REMOTE_ADDR' => '198.51.100.51'],
+            );
+        }
+        /** @var LoginThrottle $throttle */
+        $throttle = $this->container->get(LoginThrottle::class);
+        self::assertTrue($throttle->isLocked('', '198.51.100.51'));
+        $remainingBefore = $throttle->lockoutSecondsRemaining('', '198.51.100.51');
+
+        // 50 more hits while locked — must not extend or escalate the lockout.
+        for ($i = 0; $i < 50; ++$i) {
+            $this->request(
+                'POST',
+                '/login/local',
+                [],
+                $body,
+                'application/x-www-form-urlencoded',
+                ['REMOTE_ADDR' => '198.51.100.51'],
+            );
+        }
+        $remainingAfter = $throttle->lockoutSecondsRemaining('', '198.51.100.51');
+        // Allow ±1s for clock drift across the calls.
+        self::assertLessThanOrEqual($remainingBefore + 1, $remainingAfter);
+    }
+
     public function testCsrfMissingIs403(): void
     {
         $this->request('GET', '/login');