fix(federation/auth-guard): remediate CRIT-1/CRIT-2 + HIGH-1..4 review findings

- CRIT-1: Validate cert subjectUserId against grant.subjectUserId from DB; use authoritative DB value in FederationContext - CRIT-2: Add @Inject(GrantsService) decorator (tsx/esbuild requirement) - HIGH-1: Validate UTF8String TLV tag, length, and bounds in OID parser - HIGH-2: Collapse all 403 wire messages to a generic string to prevent grant enumeration; keep internal logger detail - HIGH-3: Assert federation wire envelope shape in all guard tests - HIGH-4: Regression test for subjectUserId cert/DB mismatch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-23 22:45:35 -05:00
parent 53ee36239b
commit 71c7b85026
3 changed files with 146 additions and 32 deletions
--- a/apps/gateway/src/federation/oid.util.ts
+++ b/apps/gateway/src/federation/oid.util.ts
@@ -50,10 +50,40 @@ const decoder = new TextDecoder();
 /**
 * Decode an extension value encoded as ASN.1 UTF8String TLV
 * (tag 0x0C + 1-byte length + UTF-8 bytes).
- * Slices off the 2-byte TLV header and decodes the remainder as UTF-8.
+ * Validates tag, length byte, and buffer bounds before decoding.
+ * Throws a descriptive Error on malformed input; caller wraps in try/catch.
 */
 function decodeUtf8StringTlv(value: ArrayBuffer): string {
  const bytes = new Uint8Array(value);
+
+  // Need at least tag + length bytes
+  if (bytes.length < 2) {
+    throw new Error(`UTF8String TLV too short: expected at least 2 bytes, got ${bytes.length}`);
+  }
+
+  // Tag byte must be 0x0C (ASN.1 UTF8String)
+  if (bytes[0] !== 0x0c) {
+    throw new Error(
+      `UTF8String TLV tag mismatch: expected 0x0C, got 0x${bytes[0]!.toString(16).toUpperCase()}`,
+    );
+  }
+
+  // Only single-byte length form is supported (values 0–127); long form not needed
+  // for OID strings of this length.
+  const declaredLength = bytes[1]!;
+  if (declaredLength > 127) {
+    throw new Error(
+      `UTF8String TLV uses long-form length (0x${declaredLength.toString(16).toUpperCase()}), which is not supported`,
+    );
+  }
+
+  // Declared length must match actual remaining bytes
+  if (declaredLength !== bytes.length - 2) {
+    throw new Error(
+      `UTF8String TLV length mismatch: declared ${declaredLength}, actual ${bytes.length - 2}`,
+    );
+  }
+
  // Skip: tag (1 byte) + length (1 byte)
  return decoder.decode(bytes.slice(2));
 }
--- a/apps/gateway/src/federation/server/tests/federation-auth.guard.spec.ts
+++ b/apps/gateway/src/federation/server/tests/federation-auth.guard.spec.ts
@@ -162,7 +162,7 @@ describe('FederationAuthGuard', () => {
  // ── 401: No TLS socket ────────────────────────────────────────────────────

  it('returns 401 when there is no TLS socket (plain HTTP connection)', async () => {
-    const { ctx, statusMock } = makeContext({
+    const { ctx, statusMock, sendMock } = makeContext({
      certPem: certPem,
      hasTlsSocket: false,
    });
@@ -172,18 +172,28 @@ describe('FederationAuthGuard', () => {

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(401);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }),
+      }),
+    );
  });

  // ── 401: Cert not presented ───────────────────────────────────────────────

  it('returns 401 when the peer did not present a certificate', async () => {
-    const { ctx, statusMock } = makeContext({ certPem: null });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem: null });

    const guard = new FederationAuthGuard(makeGrantsService());
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(401);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }),
+      }),
+    );
  });

  // ── 401: Cert parse failure ───────────────────────────────────────────────
@@ -191,13 +201,18 @@ describe('FederationAuthGuard', () => {
  it('returns 401 when the certificate DER bytes are corrupt', async () => {
    // Build context with a cert that has garbage DER bytes
    const corruptPem = '-----BEGIN CERTIFICATE-----\naW52YWxpZA==\n-----END CERTIFICATE-----';
-    const { ctx, statusMock } = makeContext({ certPem: corruptPem });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem: corruptPem });

    const guard = new FederationAuthGuard(makeGrantsService());
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(401);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }),
+      }),
+    );
  });

  // ── 401: Missing grantId OID ─────────────────────────────────────────────
@@ -206,13 +221,18 @@ describe('FederationAuthGuard', () => {
    // makeSelfSignedCert produces a cert without any Mosaic OIDs
    const { makeSelfSignedCert } = await import('../../__tests__/helpers/test-cert.js');
    const plainCert = await makeSelfSignedCert();
-    const { ctx, statusMock } = makeContext({ certPem: plainCert });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem: plainCert });

    const guard = new FederationAuthGuard(makeGrantsService());
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(401);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }),
+      }),
+    );
  });

  // ── 401: Missing subjectUserId OID ───────────────────────────────────────
@@ -257,13 +277,18 @@ describe('FederationAuthGuard', () => {
      ],
    });

-    const { ctx, statusMock } = makeContext({ certPem: cert.toString('pem') });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem: cert.toString('pem') });

    const guard = new FederationAuthGuard(makeGrantsService());
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(401);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }),
+      }),
+    );
  });

  // ── 403: Grant not found ─────────────────────────────────────────────────
@@ -275,13 +300,18 @@ describe('FederationAuthGuard', () => {
        .mockRejectedValue(new NotFoundException(`Grant ${GRANT_ID} not found`)),
    });

-    const { ctx, statusMock } = makeContext({ certPem });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem });

    const guard = new FederationAuthGuard(grantsService);
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(403);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }),
+      }),
+    );
  });

  // ── 403: Grant in `pending` status ───────────────────────────────────────
@@ -291,13 +321,18 @@ describe('FederationAuthGuard', () => {
      getGrantWithPeer: vi.fn().mockResolvedValue(makeGrantWithPeer({ status: 'pending' })),
    });

-    const { ctx, statusMock } = makeContext({ certPem });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem });

    const guard = new FederationAuthGuard(grantsService);
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(403);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }),
+      }),
+    );
  });

  // ── 403: Grant in `revoked` status ───────────────────────────────────────
@@ -309,13 +344,18 @@ describe('FederationAuthGuard', () => {
        .mockResolvedValue(makeGrantWithPeer({ status: 'revoked', revokedAt: new Date() })),
    });

-    const { ctx, statusMock } = makeContext({ certPem });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem });

    const guard = new FederationAuthGuard(grantsService);
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(403);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }),
+      }),
+    );
  });

  // ── 403: Grant in `expired` status ───────────────────────────────────────
@@ -325,13 +365,18 @@ describe('FederationAuthGuard', () => {
      getGrantWithPeer: vi.fn().mockResolvedValue(makeGrantWithPeer({ status: 'expired' })),
    });

-    const { ctx, statusMock } = makeContext({ certPem });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem });

    const guard = new FederationAuthGuard(grantsService);
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(403);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }),
+      }),
+    );
  });

  // ── 403: Cert serial mismatch ─────────────────────────────────────────────
@@ -360,13 +405,51 @@ describe('FederationAuthGuard', () => {
    });

    // Context presents cert with serial '01' but DB has 'DEADBEEF'
-    const { ctx, statusMock } = makeContext({ certPem, certSerialHex: '01' });
+    const { ctx, statusMock, sendMock } = makeContext({ certPem, certSerialHex: '01' });

    const guard = new FederationAuthGuard(grantsService);
    const result = await guard.canActivate(ctx);

    expect(result).toBe(false);
    expect(statusMock).toHaveBeenCalledWith(403);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }),
+      }),
+    );
+  });
+
+  // ── 403: subjectUserId cert/DB mismatch (CRIT-1 regression test) ─────────
+
+  it('returns 403 when the cert subjectUserId does not match the DB grant subjectUserId', async () => {
+    // Build a cert that claims an attacker's subjectUserId
+    const attackerSubjectUserId = 'attacker-user-id';
+    const attackerCertPem = await makeMosaicIssuedCert({
+      grantId: GRANT_ID,
+      subjectUserId: attackerSubjectUserId,
+    });
+
+    // DB returns a grant with the legitimate USER_ID
+    const grantsService = makeGrantsService({
+      getGrantWithPeer: vi.fn().mockResolvedValue(makeGrantWithPeer({ subjectUserId: USER_ID })),
+    });
+
+    // Cert presents attacker-user-id but DB has USER_ID — should be rejected
+    const { ctx, statusMock, sendMock } = makeContext({
+      certPem: attackerCertPem,
+      certSerialHex: CERT_SERIAL_HEX,
+    });
+
+    const guard = new FederationAuthGuard(grantsService);
+    const result = await guard.canActivate(ctx);
+
+    expect(result).toBe(false);
+    expect(statusMock).toHaveBeenCalledWith(403);
+    expect(sendMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }),
+      }),
+    );
  });

  // ── Happy path ────────────────────────────────────────────────────────────
--- a/apps/gateway/src/federation/server/federation-auth.guard.ts
+++ b/apps/gateway/src/federation/server/federation-auth.guard.ts
@@ -39,7 +39,13 @@
 * TLS layer (Node.js `rejectUnauthorized: true` with the CA cert pinned).
 */

-import { type CanActivate, type ExecutionContext, Injectable, Logger } from '@nestjs/common';
+import {
+  type CanActivate,
+  type ExecutionContext,
+  Inject,
+  Injectable,
+  Logger,
+} from '@nestjs/common';
 import type { FastifyReply, FastifyRequest } from 'fastify';
 import * as tls from 'node:tls';
 import { X509Certificate } from '@peculiar/x509';
@@ -74,7 +80,7 @@ function sendFederationError(
 export class FederationAuthGuard implements CanActivate {
  private readonly logger = new Logger(FederationAuthGuard.name);

-  constructor(private readonly grantsService: GrantsService) {}
+  constructor(@Inject(GrantsService) private readonly grantsService: GrantsService) {}

  async canActivate(context: ExecutionContext): Promise<boolean> {
    const http = context.switchToHttp();
@@ -146,16 +152,20 @@ export class FederationAuthGuard implements CanActivate {
    } catch {
      // getGrantWithPeer throws NotFoundException when not found
      this.logger.warn(`Grant not found: ${grantId}`);
-      return sendFederationError(reply, new FederationForbiddenError(`Grant ${grantId} not found`));
+      return sendFederationError(reply, new FederationForbiddenError('Federation access denied'));
    }

    // ── Step 5: Assert grant is active ──────────────────────────────────────
    if (grant.status !== 'active') {
      this.logger.warn(`Grant ${grantId} is not active — status=${grant.status}`);
-      return sendFederationError(
-        reply,
-        new FederationForbiddenError(`Grant ${grantId} is not active (status: ${grant.status})`),
-      );
+      return sendFederationError(reply, new FederationForbiddenError('Federation access denied'));
+    }
+
+    // ── Step 5b: Validate cert-extracted subjectUserId against DB (CRIT-1) ──
+    // The cert claim is untrusted input; the DB row is authoritative.
+    if (subjectUserId !== grant.subjectUserId) {
+      this.logger.warn(`subjectUserId mismatch for grant ${grantId}`);
+      return sendFederationError(reply, new FederationForbiddenError('Federation access denied'));
    }

    // ── Step 6: Defense-in-depth — cert serial must match registered peer ───
@@ -168,12 +178,7 @@ export class FederationAuthGuard implements CanActivate {
    if (!grant.peer.certSerial) {
      // Peer row exists but has no stored serial — something is wrong with enrollment
      this.logger.error(`Peer ${grant.peerId} has no stored certSerial — enrollment incomplete`);
-      return sendFederationError(
-        reply,
-        new FederationForbiddenError(
-          'Peer registration incomplete — no certificate serial on record',
-        ),
-      );
+      return sendFederationError(reply, new FederationForbiddenError('Federation access denied'));
    }

    // Normalize both to uppercase for comparison (Node.js serialNumber is
@@ -184,18 +189,14 @@ export class FederationAuthGuard implements CanActivate {
        `Cert serial mismatch for grant ${grantId}: ` +
          `inbound=${inboundSerial} registered=${grant.peer.certSerial}`,
      );
-      return sendFederationError(
-        reply,
-        new FederationForbiddenError(
-          'Client certificate serial does not match registered peer certificate',
-        ),
-      );
+      return sendFederationError(reply, new FederationForbiddenError('Federation access denied'));
    }

    // ── Step 7: Attach FederationContext to request ──────────────────────────
+    // Use grant.subjectUserId from DB (authoritative) — not the cert-extracted value.
    const federationContext: FederationContext = {
      grantId,
-      subjectUserId,
+      subjectUserId: grant.subjectUserId,
      peerId: grant.peerId,
      scope: grant.scope as Record<string, unknown>,
    };
@@ -203,7 +204,7 @@ export class FederationAuthGuard implements CanActivate {
    request.federationContext = federationContext;

    this.logger.debug(
-      `Federation auth OK — grantId=${grantId} peerId=${grant.peerId} subjectUserId=${subjectUserId}`,
+      `Federation auth OK — grantId=${grantId} peerId=${grant.peerId} subjectUserId=${grant.subjectUserId}`,
    );

    return true;