0ad7c31549
REVIEW_RELAY_SECURITY.md: two-round multi-agent audit — F1–F6 with fix plan/ resolution, refuted-but-recorded items, and the e2e harness coverage table. PROGRESS_LOG.md: cross-session record of the audit + fixes + harness.
170 lines
13 KiB
Markdown
170 lines
13 KiB
Markdown
# Security Audit — relay-e2e + relay-auth (pre-production go-live gate)
|
||
|
||
**Date:** 2026-07-02 · **Branch:** `security/relay-auth-audit-fixes` · **Prior verdict:** approve-with-changes
|
||
**Method:** multi-agent deep audit — 11 security dimensions fanned out across both packages, every
|
||
candidate finding adversarially verified by ≥3 independent skeptic lenses (exploitability /
|
||
crypto-spec / refuter, majority vote), a second "repeat-review" deep-dive pass, a per-package
|
||
completeness critic, then dedup + severity re-rank. 99 agents, ~6.9M tokens. The orchestrator
|
||
independently read 100% of both `src/` trees to adjudicate.
|
||
|
||
## Verdict
|
||
|
||
**relay-e2e (crypto core): PASS — no changes required.** Every candidate against the E2E core
|
||
(replay-key nonce reuse, sequence/handshake ordering, key-hygiene, X25519 low-order) was **refuted**
|
||
on verification: the deterministic-nonce discipline is safe because the direction split + the real
|
||
agent-side `createReplaySealer` (in `term-relay/agent`) each own an independent monotonic counter;
|
||
recv `accept()` is a strict-successor guard whose pre-auth commit cannot cause a replay-accept.
|
||
|
||
**relay-auth: APPROVE-WITH-CHANGES — 5 confirmed findings (2 HIGH, 1 MED, 2 LOW), all in the
|
||
human-auth / enforcement layer.** All fixes are contained in relay-auth; the P5-local types
|
||
(`AuthenticatedPrincipal`, `StepUpPolicy`) are extendable without touching any frozen contract.
|
||
|
||
### Refuted (verified NOT exploitable — recorded so they are not re-litigated)
|
||
- E2E replay `K_content` nonce reuse — real consumer keeps an independent monotonic seq (no reset-to-0).
|
||
- mTLS `verifyChain` missing EKU/keyUsage — every abuse cert fails the later mandatory SPIFFE-SAN →
|
||
enrolled-&-non-revoked-host registry gate.
|
||
- SPIFFE trust-domain not pinned on verify — gated by chain-valid + registry; defense-in-depth only.
|
||
- Reattach host-scope "bypass" — `token.host === requestedHostId` bind at `decide.ts:62` blocks it;
|
||
session↔host binding is enforced at connect by P1/P3. (Kept as a cheap D-i-D hardening note.)
|
||
- TOTP non-constant-time compare / device-proof `acct` canonicalization / DPoP ±future-iat — LOW, refuted.
|
||
|
||
---
|
||
|
||
## Confirmed findings + fix plan
|
||
|
||
### F1 · HIGH — Step-up factor downgrade (`human/stepup/stepup.ts:33`)
|
||
`needsStepUp()` tests `policy.requiredMethod ∈ principal.amr` and freshness via a single
|
||
method-agnostic `stepUpAt`. `amr` is cumulative across login + every step-up, so a **passkey**
|
||
requirement is satisfied by a passkey used at *login*, while the freshness timestamp can be produced
|
||
by a *weaker* step-up (e.g. phished TOTP). The phishing-resistant control silently downgrades.
|
||
**Fix:** bind freshness to the method. Add `stepUpMethod` to the principal; `needsStepUp` requires
|
||
`stepUpMethod === requiredMethod` AND fresh; `recordStepUp` stamps the method. (Group A)
|
||
|
||
### F2 · HIGH — Mandatory step-up skipped on the new-session path (`enforce/onUpgrade.ts:128`)
|
||
The gate is wrapped in `if (ctx.principal !== null)`, but the production caller
|
||
(`term-relay/data-plane/upgrade.ts:105`) hardcodes `principal: null` on **both** connect and reattach
|
||
(session principal exists only *after* authz). Result: **step-up never runs in production** — a valid
|
||
DPoP-bound token opens a root shell with the mandatory passkey ceremony never performed.
|
||
Deny-by-default is violated (absence of proof → allow).
|
||
**Fix:** make step-up **host-driven and fail-closed**: when `stepUpPolicyFor(host).required`, DENY
|
||
(403 `step_up_required`) unless an authenticated principal proves fresh step-up. Applies to connect
|
||
and reattach. Default v0.9 policy is `required:false` → existing non-step-up hosts unaffected.
|
||
**Behavioral note:** once a host sets `required:true`, the P1 caller must supply a step-up principal;
|
||
until then such hosts correctly deny. (Group A)
|
||
|
||
### F3 · MEDIUM — WebAuthn assertion not bound to the stored credential (`human/webauthn/authenticate.ts:40`)
|
||
`finishAuthentication` forwards only `signCount` to the verifier — never `cred.publicKey` /
|
||
`cred.credentialId`, and there is no `credentialId` cross-check. Any reasonable verifier can only
|
||
check challenge/origin/rpId, so an assertion from a *different* registered credential authenticates.
|
||
**Fix:** add `credentialPublicKey` + `expectedCredentialId` to the verifier interface & forward them;
|
||
assert `resp.credentialId === cred.credentialId` before trusting `verified`. (Group B)
|
||
|
||
### F4 · LOW — DPoP verify throws (unhandled) → INV15 gate fails un-audited (`capability/verify.ts:142`)
|
||
`importEd25519PublicRaw(rawPub)` (and `readCnfJkt`) run outside try/catch; a thumbprint-matching but
|
||
malformed key makes `verifyDpopProof` **reject** instead of returning false. Callers (`decide.ts:57`,
|
||
`onUpgrade.ts:112`) don't catch → the sole authz gate throws an unhandled exception and emits **no**
|
||
`deny` AuditEvent (audit-evasion + repeatable unhandled-rejection DoS). Fail-closed but off-contract.
|
||
**Fix (defense-in-depth):** (a) wrap the risky calls in `verifyDpopProof` → return false; (b) wrap the
|
||
`verifyDpopProof` call in `coreAuthorize` → clean audited `deny(401)`; (c) validate `cnfJkt` format at
|
||
issue. (Group C)
|
||
|
||
### F5 · LOW — WebAuthn verified `signCount` discarded → clone detection inert (`human/webauthn/authenticate.ts:52`)
|
||
The advanced `newSignCount` is used only for the regression check and never returned/persisted, so the
|
||
guard forever compares against the registration-time count — a cloned authenticator is never detected.
|
||
**Fix:** return `newSignCount` (updated credential) so the caller can persist it. (Group B)
|
||
|
||
---
|
||
|
||
## Execution plan (multi-agent, loop mode)
|
||
|
||
- **Foundation (barrier):** `types.ts` — add `StepUpPolicy.required: boolean` + `AuthenticatedPrincipal.stepUpMethod?: AuthMethod|null` (+ Zod). Optional `stepUpMethod` ⇒ no ripple to other principal constructors; absent ⇒ treated as null ⇒ needs-step-up (fail-safe).
|
||
- **Fix groups (parallel, file-disjoint):**
|
||
- **Group A (F1,F2):** `stepup.ts`, `enforce/onUpgrade.ts`, `test/{stepup,enforce}.test.ts`, `test/tripwire/cross-tenant.test.ts`.
|
||
- **Group B (F3,F5):** `human/webauthn/{verifier,authenticate}.ts`, `test/webauthn.test.ts`.
|
||
- **Group C (F4):** `capability/{verify,issue}.ts`, `authz/decide.ts`, `test/capability.test.ts`.
|
||
- **Verify (loop until green):** `tsc --noEmit` + full `vitest run` in relay-auth; repair any breakage; repeat. Baseline to preserve/extend: relay-e2e 76, relay-auth 104.
|
||
- **Re-audit (parallel adversarial):** one verifier per finding re-reads the fixed code and confirms the specific exploit is closed.
|
||
|
||
Each fix ships with a **regression test** encoding the exploit (F2: STRICT+null→403; F1: fresh TOTP ≠ passkey; F3: credentialId mismatch→reject; F4: malformed-jwk DPoP→false+audited deny; F5: newSignCount surfaced).
|
||
|
||
---
|
||
|
||
## Second independent review (2026-07-02, Fable 5) — RECORD CORRECTION
|
||
|
||
A second review (fix-regression hunt + fresh-angle sweep + adversarial re-attack of refuted items),
|
||
run on a **different model** for reviewer diversity, returned:
|
||
- **Fix-regression: 0** — the 5 fixes (F1–F5) survive independent scrutiny, no regressions.
|
||
- **Re-attack of refuted/D-i-D items: 0 survived** — mTLS EKU, SPIFFE domain, reattach host-scope, TOTP
|
||
replay, device-proof canonicalization are re-confirmed **non-exploitable**.
|
||
- **1 new HIGH** — which the first audit had **REFUTED in error** (a false-negative). Corrected below.
|
||
|
||
### F6 · HIGH — Recoverable `K_content` replay key reuses `(key, nonce)` across sealer generations
|
||
**⚠️ This corrects the earlier "relay-e2e PASS" verdict — relay-e2e's replay surface has a real nonce-reuse hazard.**
|
||
`relay-e2e/src/replay-key.ts` `deriveContentKey` keys `K_content = HKDF(hostContentSecret, salt=sessionId,
|
||
info=const)` — **deterministic**, and *intended* to be recoverable/stable per `(host, sessionId)`. The
|
||
nonce is deterministic `f(seq)` and `sealReplayFrame` is stateless (no SequenceGuard). The sole real
|
||
consumer, `agent/src/e2e/replaySeal.ts:41` `createReplaySealer`, holds `let seq = 0n` **in memory** and
|
||
**resets to 0 on every reconstruction** (agent restart/redeploy, or a new attach), while `hostContentSecret`
|
||
is persisted 0600 (Keystore) and `sessionId` is stable (localStorage). Unlike the LIVE h2c path — safe
|
||
because each reconnect runs a fresh ECDH → brand-new keys — the replay path reuses the **same K_content**
|
||
with `seq` restarting at 0 ⇒ two generations of distinct plaintext sealed under identical `(key, nonce)`.
|
||
An untrusted relay (INV2 sees ciphertext) that observes both generations recovers plaintext-XOR (terminal
|
||
output: commands/tokens/code) and, for AES-256-GCM, recovers the GHASH auth-subkey → forgery, which
|
||
`openReplayCiphertext` (no cross-frame replay check) will accept.
|
||
**Why round 1 missed it:** the round-1 verifier located `let seq = 0n`, correctly noted it's an independent
|
||
counter, but wrongly concluded "safe" by analogy to the live path — missing that `K_content` (unlike the
|
||
live ephemeral keys) is STABLE across restarts, so the seq reset *does* collide. Round 2's dedicated
|
||
re-attack + orchestrator's direct read of `replaySeal.ts`/`hostEndpoint.ts` confirm the exploit.
|
||
**Fix options (design decision — touches a frozen contract and/or the agent package, both outside the two
|
||
originally-scoped packages):**
|
||
- **(A) Agent-side durable seq** — persist the replay `SequenceGuard` with the ring buffer / Keystore so
|
||
`seq` never resets for a given `(sessionId, K_content)`. Keeps `K_content` recoverable; blast radius =
|
||
`agent/` only. Requires durable monotonic-seq guarantee across restart.
|
||
- **(B) Per-generation epoch in `K_content`** — add an epoch/generation to `ReplayKeyParams` (frozen in
|
||
`relay-contracts/src/e2e/types.ts`), mixed into the HKDF salt, and carry it with each frame so the browser
|
||
re-derives the right key. Cryptographically robust (fresh key per generation) but changes a **frozen
|
||
contract** + `relay-e2e` + `relay-web` (browser re-derivation) + `agent`.
|
||
- Regression test (either path): two sealer generations for the same `(secret, sessionId)` must never emit
|
||
two frames sharing `(key, nonce)`.
|
||
|
||
**RESOLUTION — FIXED (option B, epoch-in-key), 2026-07-02.** Implemented across all 4 packages:
|
||
`ReplayKeyParams` (relay-contracts) gains a required `epoch: string`; `deriveContentKey` (relay-e2e) folds it
|
||
into the HKDF salt as `utf8(sessionId) ‖ 0x1F ‖ utf8(epoch)` (unit-separator, no concat aliasing);
|
||
`createReplaySealer` (agent) mints a fresh `randomUUID()` epoch per generation and exposes it; `ReplaySource`
|
||
(relay-web) carries `epoch` and the browser re-derives with it. A fresh epoch per generation ⇒ a fresh key,
|
||
so a `seq=0` reset after restart/re-attach can never collide with a prior generation's `(key, nonce)`;
|
||
recoverability within a generation (same epoch ⇒ same key) is preserved. **Verified:** all 4 packages
|
||
tsc-clean and green (relay-contracts 81, relay-e2e 78 [+2 F6 regressions], agent 133, relay-web 99); re-audit
|
||
confirms closure (regression proves two generations share the seq=0 nonce yet derive different keys).
|
||
**Residual (fail-closed, not reuse):** the ring-buffer transport (P1/P2) must persist each generation's epoch
|
||
and serve the epoch matching those exact frames; documented as a TODO at `relay-web/.../manage-page.ts`
|
||
(`loadReplay` is still a throwing stub — the replay path is not wired end-to-end yet, so F6 was latent).
|
||
|
||
---
|
||
|
||
## Dynamic end-to-end security harness (`e2e/`, 2026-07-02)
|
||
|
||
A cross-package harness (`e2e/`) now wires the **real** P5(auth) + P4(crypto) + P2(agent replay) exports
|
||
through in-memory seams and an untrusted-relay "RelaySpy" attacker vantage — every security check runs the
|
||
production code path; only true I/O boundaries (registries/buckets/revocation/audit/sockets) are faked.
|
||
`buildRelayWorld()` exposes the full flow (issue cap → upgrade/authz → handshake → sealed session via relay
|
||
→ reattach → revoke). **21 tests pass, tsc clean.** This upgrades the findings from *static + unit* validation
|
||
to *dynamic* validation — the attacker actually attempts each exploit:
|
||
|
||
| Attack (dynamic) | Asserted defense |
|
||
|---|---|
|
||
| **F1** step-up factor downgrade | fresh TOTP step-up → `needsStepUp` true + upgrade 403; passkey → allowed |
|
||
| **F2** step-up skipped (fail-open) | STRICT + `principal:null` → 403 `step_up_required` + audit; `required:false`+null → allowed |
|
||
| **F3** WebAuthn credential binding | `credentialId` mismatch → `WebAuthnError` even with a verifier that returns `verified:true` |
|
||
| **F4** DPoP unhandled throw | malformed thumbprint-matching jwk → `verifyDpopProof` resolves `false` (no throw) → clean audited `dpop_proof_failed` deny |
|
||
| **F6** replay `(key,nonce)` reuse | two sealer generations → same seq-0 nonce but different key ⇒ ciphertext+tag differ; cross-gen open throws; within-gen recovers |
|
||
| MITM | wrong `agentPubkey` / tampered `hostEphPub` → `FingerprintMismatchError`, no keys derived |
|
||
| reflection / replay / reorder | c2h→client.open rejects; dup/out-of-order `open` throws (SequenceGuard) |
|
||
| INV2 | RelaySpy ciphertext (live + replay path) never contains the plaintext marker |
|
||
| cross-tenant (INV1) | acct-A token at acct-B host → 403 `cross_tenant` + `cross-tenant-attempt` audit |
|
||
| capability single-use | same jti twice (fresh DPoP) → 2nd 403 `token_replayed` |
|
||
|
||
**Not dynamically covered:** F5 (signCount persistence) is a return-shape change, asserted by its relay-auth
|
||
unit regression. The harness fakes the P1 relay/transport (RelaySpy) rather than running term-relay's mux
|
||
and the control-plane HTTP — a true multi-process/browser e2e would need the (not-yet-built) run tooling.
|