소스 검색

fix: rotate session id at /login/oidc before OIDC state is stashed (SEC_REVIEW F9)

Pre-handshake rotation: OidcController::initiate now calls
SessionManager::regenerateId() at the top, BEFORE delegating to the
authenticator. The jumbojett library stores state, nonce and the PKCE
code_verifier in $_SESSION as part of authenticate(); rotating first
ensures those security-critical values are bound only to a freshly
minted session id, so an attacker who pre-fixated the victim's
irdb_session cookie loses contact at this moment.

A second rotation (post-callback) is unchanged, defeating anything
carrying over.

Per F8, regenerateId() now fail-closes if response headers are already
sent, so this rotation cannot silently no-op in HTTP.

Regression test in OidcFlowTest::testInitiateRotatesSessionIdBeforeAuthenticate
captures session_id() inside a fake authenticator and asserts it
differs from the pre-request id.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 5 일 전
부모
커밋
2a5758925c
3개의 변경된 파일58개의 추가작업 그리고 2개의 파일을 삭제
  1. 4 2
      ui/src/Auth/JumbojettOidcAuthenticator.php
  2. 14 0
      ui/src/Auth/OidcController.php
  3. 40 0
      ui/tests/Integration/Auth/OidcFlowTest.php

+ 4 - 2
ui/src/Auth/JumbojettOidcAuthenticator.php

@@ -12,8 +12,10 @@ use Jumbojett\OpenIDConnectClientException;
  *
  * The library handles state/nonce/PKCE and stashes them in the PHP
  * session itself (`$_SESSION['openid_connect_*']`), so we don't manage
- * those — but we DO regenerate the session id on success in the
- * controller after `authenticate()` returns.
+ * those — but the controller regenerates the session id BEFORE
+ * `authenticate()` runs on `/login/oidc` (SEC_REVIEW F9, defeats
+ * pre-handshake fixation) and AGAIN after a successful callback
+ * (defeats anything carrying over).
  *
  * SPEC §M08.4 scopes: `openid profile email`. Entra emits the `groups`
  * claim once configured in the app manifest (see `doc/oidc.md`); we

+ 14 - 0
ui/src/Auth/OidcController.php

@@ -24,6 +24,16 @@ use Psr\Log\LoggerInterface;
  * `OIDC_DEFAULT_ROLE=none` case surfaces as `role = "none"` in the
  * upsert response), redirect to `/no-access` — they're authenticated
  * but unauthorized.
+ *
+ * SEC_REVIEW F9: the session id is rotated at the top of `initiate()`,
+ * BEFORE the jumbojett library stashes `state`, `nonce`, and the PKCE
+ * `code_verifier` in `$_SESSION`. An attacker who pre-fixated the
+ * victim's `irdb_session` cookie loses contact at this moment — the
+ * OIDC handshake state is bound only to the freshly rotated id, and
+ * (per F8) the rotation itself fail-closes if headers are already
+ * sent. A second rotation runs after the callback succeeds (line
+ * `regenerateId()` below) to defeat anything carrying over from the
+ * pre-callback request.
  */
 final class OidcController
 {
@@ -41,6 +51,10 @@ final class OidcController
         if (!$this->enabled) {
             return $response->withStatus(404);
         }
+        // SEC_REVIEW F9: rotate the session id BEFORE the underlying
+        // library stashes OIDC state/nonce/code_verifier in $_SESSION. Any
+        // pre-fixated cookie is invalidated at this point.
+        $this->sessions->regenerateId();
         // authenticate() will redirect-and-exit on the initiate path; only
         // on the callback path does it return normally. We delegate.
         return $this->finishOrFail($request, $response);

+ 40 - 0
ui/tests/Integration/Auth/OidcFlowTest.php

@@ -84,6 +84,46 @@ final class OidcFlowTest extends AppTestCase
         self::assertSame('error', $flash[0]['type']);
     }
 
+    public function testInitiateRotatesSessionIdBeforeAuthenticate(): void
+    {
+        // SEC_REVIEW F9: the session id must be rotated at the top of
+        // /login/oidc, BEFORE the authenticator stashes state/nonce/PKCE
+        // in $_SESSION. An attacker who pre-fixated the victim's cookie
+        // loses contact at this moment.
+        //
+        // Drive an initial request to establish a session id; capture it
+        // and the id observed inside authenticate(); assert they differ.
+        $this->request('GET', '/login');
+        $preId = session_id();
+        self::assertNotSame('', $preId);
+
+        $capture = new \stdClass();
+        $capture->idAtAuthenticate = null;
+        $this->bindOidcAuthenticator(new class ($capture) implements OidcAuthenticator {
+            public function __construct(private readonly \stdClass $capture)
+            {
+            }
+
+            public function authenticate(): OidcClaims
+            {
+                $this->capture->idAtAuthenticate = session_id();
+                // Short-circuit: we only care about the pre-handshake
+                // rotation, not the rest of the OIDC flow.
+                throw new OidcException('captured for test');
+            }
+        });
+
+        $this->request('GET', '/login/oidc');
+
+        self::assertIsString($capture->idAtAuthenticate);
+        self::assertNotSame('', $capture->idAtAuthenticate);
+        self::assertNotSame(
+            $preId,
+            $capture->idAtAuthenticate,
+            'session id was not rotated before OIDC handshake (F9 regression)',
+        );
+    }
+
     public function testApiUnreachableDuringUpsertFlashesAndRedirects(): void
     {
         $this->bindOidcAuthenticator(new class () implements OidcAuthenticator {