Review by moonshotai/kimi-k3

commit: 765b6710b621474e53c575a0ae3f02b76643355d
usage: {'prompt_tokens': 93450, 'completion_tokens': 19155, 'total_tokens': 112605, 'cost': 0.5675022, 'is_byok': False, 'prompt_tokens_details': {'cached_tokens': 64, 'cache_write_tokens': 0, 'audio_tokens': 0, 'video_tokens': 0}, 'cost_details': {'upstream_inference_cost': 0.5675022, 'upstream_inference_prompt_cost': 0.2801772, 'upstream_inference_completions_cost': 0.287325}, 'completion_tokens_details': {'reasoning_tokens': 16159, 'image_tokens': 0, 'audio_tokens': 0}}
time: 146s

Security review: timelock-proxy @ 765b6710

I read the README's threat model first and then the enclave, host, CLI, and build/deploy code against it. Overall the architecture matches the claims: TLS terminates in the enclave, upstream TLS is verified in the enclave, sealing is age∘tlock with a pinned chain, round selection rounds up, diagnostics are disciplined, and the CLI binds the attestation to the handshake certificate. I found no critical or high-severity confidentiality break. Findings below, in priority order.

High

H1. tlproxy verify nonce check compares against base64(nonce), not the raw nonce — verification always fails (or worse, gets patched around)

• File/function: crates/cli/src/attested.rs, connect (validate_expected_nonce(&doc, &base64::engine::general_purpose::STANDARD.encode(nonce))).
• What happens: The enclave hex-decodes the query nonce and passes the raw bytes to the NSM (proxy.rs → attest.rs: ByteBuf::from(n.to_vec())), so the attestation document's nonce field is the raw 32 bytes. The CLI then asks attestation-doc-validation to compare the document nonce against the base64 encoding of those bytes. validate_expected_nonce compares the document's raw nonce bytes against the provided value, so this never matches.
• Why it matters: Every tlproxy verify against a genuine, correctly-built enclave fails at the nonce step. That is fail-closed, but it means the documented verification flow has never worked end-to-end, and the realistic operator response is to use --insecure-skip-measurement or patch the check out — which destroys the whole verification property. (If the crate's signature is &[u8], note also that &String would not compile, so the compiled behavior depends on the exact crate API — either way, the value passed is the wrong encoding.)
• Fix: Pass the raw nonce: validate_expected_nonce(&doc, &nonce). Add an integration test that runs verify against a dev-mode or mocked attestation flow so this can't regress silently.

Medium

M2. Self-referential upstream: /<proxy's-own-name>/... amplifies one request into hundreds of in-flight exchanges and records

• File/function: crates/enclave/src/proxy.rs, parse_target / forward.
• Attack: A sender requests https://proxy.girl.surgery/proxy.girl.surgery/proxy.girl.surgery/.../x. Each hop strips one path segment, so the enclave connects to itself (via the Mullvad exit → parent :443 → forwarder → enclave) once per path segment. With a multi-KB path, one sender request occupies ~hundreds of the 256 max_in_flight slots simultaneously, each holding a slot until the innermost resolves and the chain unwinds, and each producing a sealed record. A handful of such requests 503s all other senders and floods the record queue.
• Why the code allows it: parse_target rejects IP literals and localhost but never rejects the proxy's own names (state.tls.dns_names) or any name resolving to the parent's public IP.
• Fix: Reject targets whose host matches cfg.tls_dns_names (case-insensitive). Optionally cap path depth or add a recursion-prevention header stripped from outside and set on forwarded requests.

M3. smoltcp set_timeout(120s) silently aborts idle upstream connections, truncating long exchanges

• File/function: crates/enclave/src/wg.rs, run (sock.set_timeout(Some(smoltcp::time::Duration::from_secs(120)))).
• Attack/bug: Any upstream exchange with a >120 s gap in data — slow streaming responses, server-sent events, a long-running upload followed by a slow server — has its TCP socket aborted by smoltcp's inactivity timeout. The sender's exchange is cut and the record is marked incomplete. This also contradicts UPSTREAM_HEADERS_TIMEOUT = 600s in proxy.rs: a slow upstream that sends headers at t=300 s is fine only if the socket happened to see traffic; a headers wait past 120 s of silence is killed early by the stack, not by the intended 600 s policy.
• Fix: Set the smoltcp timeout to at least the maximum intended exchange lifetime (or None and rely on keepalive + the connection-lifetime cap), so the documented timeouts are the ones that actually fire.

M4. socket→user path can drop already-consumed bytes, corrupting the stream and the record

• File/function: crates/enclave/src/wg.rs, service_sockets (sock.recv(|data| ...) consumes from the socket, then to_user.try_send(chunk)).
• Bug: sock.recv consumes n bytes from the TCP receive buffer; if try_send then fails (channel closed because the user stream was dropped but the Gone message hasn't been processed yet), the chunk is discarded — those bytes are gone from the socket and never delivered. The connection is usually being torn down anyway, so the practical impact is a truncated tail of a response that the record marks body_complete: false only if the error path notices; in the user_gone race the record can look cleaner than the delivery was.
• Fix: Peek instead of consume (sock.recv only after a successful send, or use recv_queue/peek-style APIs), or check to_user.is_closed() before consuming and abort the socket instead of draining it.

M5. Memory budgets sum to more than the 1 GiB enclave — OOM crash loses all in-flight records

• File/function: config/enclave.toml vs deploy/run-enclave.sh (--memory 1024); crates/enclave/src/proxy.rs Finalizer::drop (sealing), records.rs (queue).
• Analysis: max_capture_bytes_total = 256 MiB and max_queued_record_bytes = 256 MiB are independent budgets, and sealing multiplies: each of the 4 concurrent seal jobs holds the captured bodies, a base64-JSON copy (~1.34×), the age ciphertext, and the tlock ciphertext. Worst case is roughly 256 MiB captured + ~2.5× that in sealing copies + 256 MiB queued + smoltcp buffers (256 conns × ~400 KiB) + the runtime — comfortably past 1 GiB. A sender can drive this with 64 MiB bodies. panic = "abort" plus OOM kill means the enclave dies and every in-flight record is lost (the property "every exchange is recorded" fails), and the TLS key is gone.
• Fix: Either raise --memory, or make the budgets jointly exhaustive: count queued record bytes and sealing working memory against the same 256 MiB capture budget, and make seal_permits byte-weighted rather than a flat 4.

M6. No per-sender fairness: one sender can hold all 256 in-flight slots for 10 minutes each

• File/function: crates/enclave/src/proxy.rs (UPSTREAM_HEADERS_TIMEOUT = 600s), main.rs (limits).
• Attack: 256 connections each POSTing a slow body to a slow upstream (or just trickling) occupy every in-flight slot for up to 600 s; all other senders get 503. Connection cap (1024) and lifetime (3600 s) don't help because the bottleneck is in_flight. This is an availability-only issue (nothing is exposed), and the README is honest that availability is best-effort, but the cost to an attacker is trivial.
• Fix: Lower the headers timeout (e.g. 60–120 s), and/or add a per-source-IP in-flight sub-limit. (Per-IP is imperfect behind NAT but raises the cost substantially.)

Low

L7. Attestation endpoint accepts a missing nonce — crates/enclave/src/proxy.rs, attestation: nonce is Option; GET /.well-known/attestation with no query returns a valid nonce-less document. The CLI always sends one, so this is not exploitable today, but a nonce-less document is replayable by construction and future/other verifiers may not be as careful. Fix: require nonce (400 otherwise).

L8. Finalizer::drop leaks in_flight/capture_bytes when no runtime is available — crates/enclave/src/proxy.rs: if Handle::try_current() fails, the record is dropped and the counters are never decremented; enough of these permanently degrades the enclave to 503s. Only reachable during shutdown/teardown, hence low. Fix: decrement the counters before the early return.

L9. ECH config that fails to parse silently downgrades to plaintext SNI — crates/enclave/src/net.rs, connect_tls: on ech_config() error it logs and uses the plain config, exposing the SNI to the parent for that upstream. The x-timelock-proxy-ech response header does let the sender detect it (not-offered/grease instead of accepted), which mitigates this; still, consider failing closed for hosts that published an ECH config, or at least documenting that a poisoned/unparseable HTTPS record downgrades privacy (not content).

L10. relay.state strings surface third-party API error text in the attestation response and landing page — crates/enclave/src/relay.rs status() + net.rs Dialer::json (error includes up to 300 bytes of the Mullvad API response body) → Mode::Down(format!(...)) → RelayStatus.state → AttestationResponse.relay and the landing HTML. This is infrastructure data, not sender data, so it doesn't violate the diagnostics rule, but it is uncontrolled third-party text rendered into an HTML page and a security-relevant API response. Fix: reduce to a fixed category like the upstream-error classifier.

L11. Request/response trailers are forwarded but never captured — crates/enclave/src/proxy.rs, TeeBody::poll_frame copies only data frames; trailer frames pass through unrecorded, and the record has no trailer field, so a record can show body_complete: true while trailers (which can carry meaningful data, e.g. gRPC status) are absent. Fix: capture trailers into the record, or strip te: trailers (already stripped) and drop trailer frames.

Info

I12. The seven-day guarantee rests solely on the NSM clock whenever the parent withholds beacons. The beacon lower bound only helps if refresh_loop can fetch; the parent carries those fetches and can blackhole them indefinitely (Purpose::Bootstrap goes direct). Then lock_round_at_ms uses only the NSM attestation timestamp. If the NSM/hypervisor clock were ever behind real time by more than the lock period, the target round would already be published at seal time and the operator could open records immediately. This is within the README's stated AWS trust assumption, but the README's "withholding beacons can only make a lock longer" is only half the story — beacons are also the only cross-check on the NSM clock. Consider an optional measured fail-closed mode: refuse to seal (or refuse requests) if no verified beacon is newer than some measured age.

I13. DoH resolvers (Cloudflare/Google) learn every upstream host name. TLS to the resolver hides queries from the parent, but the resolver itself sees them. This is implied by the design but absent from the README's explicit trust-assumption list; add it.

I14. ACME challenge window serves a throwaway-key certificate to any client offering acme-tls/1 ALPN (tls.rs CertResolver::resolve). A sender who offers that ALPN during a challenge gets a cert under a different key; they can't do anything with it (attestation binding fails), but it's worth a comment that this is intentional.

Nothing found at critical severity, and nothing else at high: the sealing construction, round math (round_after with div_ceil, max of start/finish/beacon bounds, 7-day floor enforced at parse and at seal time), the fail-closed relay, the diagnostics discipline, and the attestation binding (handshake-signature verification + user_data == cert SPKI + nonce + PCR0/1/2 + all-zero-PCR rejection) all check out as designed.

Verdict

The design is sound and the implementation is unusually careful about the things that matter — content never leaves the enclave unsealed, diagnostics are scrubbed, the lock math is conservative, and the CLI's attestation check is structurally correct. I would not deploy it as-is, for two reasons: (1) the nonce encoding bug in tlproxy verify means the entire sender-verification story currently fails closed — before trusting any deployment I would require that fixed and covered by an end-to-end test that runs verify against a real (dev-mode) enclave; and (2) the memory budgets provably exceed the 1 GiB enclave under adversarial load, so a sender can crash the enclave and destroy in-flight records. I'd also want M2 (self-proxy amplification) and M3/M4 (userspace-TCP truncation/byte-loss) fixed before relying on the "every exchange is recorded" property. With those addressed, the remaining findings are hardening, and I would trust the deployment to enforce its stated confidentiality and timelock properties under its documented trust assumptions (AWS Nitro, drand League of Entropy, operator key hygiene).