docs(relay): security audit report + progress log for the relay findings
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.
This commit is contained in:
Yaojia Wang
169
docs/REVIEW_RELAY_SECURITY.md
Normal file
169
docs/REVIEW_RELAY_SECURITY.md
Normal file
@@ -0,0 +1,169 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user