reviews/anthropic__claude-opus-5.md
On this page

Review by anthropic/claude-opus-5

commit: 67eb6e6f672501be1c9f67cde1b979efebdcf981
usage: {'prompt_tokens': 140311, 'completion_tokens': 41074, 'total_tokens': 181385, 'cost': 1.728405, 'is_byok': False, 'prompt_tokens_details': {'cached_tokens': 0, 'cache_write_tokens': 0, 'audio_tokens': 0, 'video_tokens': 0}, 'cost_details': {'upstream_inference_cost': 1.728405, 'upstream_inference_prompt_cost': 0.701555, 'upstream_inference_completions_cost': 1.02685}, 'completion_tokens_details': {'reasoning_tokens': 31674, 'image_tokens': 0, 'audio_tokens': 0}}
time: 578s

Security review: timelock-proxy @ 67eb6e6

I read the README as the spec, then the enclave, host, common and CLI crates against it. Below, findings are grouped by your priority areas. crates/enclave/static/index.html is referenced by proxy.rs but not present in the tree I was given, so the landing page is not reviewed.

1. Can anyone other than the recipient read request/response content before the lock?

Nothing at critical or high severity in the enclave's data path itself. I traced every place bytes cross the enclave boundary and the structure holds:

The one thing that does break this property in practice is on the verification side, below.

2. What a sender can verify (priority 3) — the most serious finding

F1. High (potentially critical): the attestation ↔ TLS-key binding is never checked in-tree, and the enclave uses a field the validator library probably does not look at

File/function: crates/cli/src/attested.rs::connect; crates/enclave/src/attest.rs::Attester::attest; crates/enclave/src/proxy.rs::attestation.

What the code does. The enclave puts the DER SubjectPublicKeyInfo in the NSM request's user_data and explicitly leaves the NSM's dedicated public_key field None:

let req = Request::Attestation {
    user_data: Some(serde_bytes::ByteBuf::from(user_data.to_vec())),
    nonce: nonce.map(...),
    public_key: None,
};

The CLI never compares anything to doc.user_data. It delegates the entire binding to one third-party call:

let doc = attestation_doc_validation::validate_attestation_doc_against_cert(&cert, &doc_bytes)

and then only checks the nonce and PCRs itself. The README claims verify "checks that user_data equals the captured certificate's public key" — no code in this repository does that.

Why this is dangerous. attestation-doc-validation is Evervault's crate, written for a convention where the enclave places the certificate's public key in the attestation document's public_key field. There are three possible behaviours and two of them are bad:

  1. The crate compares cert.public_key() to doc.public_key, and errors when it is None → tlproxy verify never succeeds against this enclave (functional break, fails closed).
  2. The crate compares only when the field is present (if let Some(pk) = doc.public_key { … }) — a very common fail-open pattern → the binding is not checked at all. Then the operator mounts a trivial, undetectable MITM: terminate TLS on the parent with a key of their own choosing, take the verifier's nonce, forward it to a genuine enclave built from this exact commit (PCRs match, nonce matches), and return that document. verify prints green, prints "PCR match", and the operator reads every request in cleartext. This defeats the headline property of the whole system.
  3. The crate happens to compare against user_data in exactly the DER SPKI encoding rcgen::PublicKeyData::subject_public_key_info produces → everything works, by luck.

There is no test anywhere in the repo that exercises verify against a captured attestation document, so which branch you are in is currently unknown to the operator and to the reviewer. Note also that even in case 3 you are relying on an undocumented field choice and encoding convention in an external crate for the single check the entire threat model rests on.

Fix. Do the comparison in attested.rs yourself, after signature validation, and do not rely on the library for it:

let ud = doc.user_data.as_ref().ok_or_else(|| anyhow!("no user_data"))?;
let spki = spki_der_from_cert(&cert_der)?;           // reuse the walk in tls.rs, or use x509-parser
if ud.as_slice() != spki.as_slice() { bail!("attestation does not bind this TLS key"); }

Also assert doc.public_key.is_none() (or set both fields in the enclave and check both), and add a unit test with a checked-in real attestation document plus its certificate, including a negative test where the certificate is swapped. Until that test exists, I would not accept "the attestation binds the TLS key" as established.

F2. Low: x-timelock-proxy-unlock-round under-reports the actual lock

File/function: proxy.rs::forward_entry sets the header from initial_round, but RecordGuard::drop seals to p.initial_round.max(lock_round_at_ms(finished_at_ms, …)). The advertised round is a lower bound on the real one, so the security direction is safe (the record unlocks later than advertised), but a sender who checks time_of_round(header) gets a number that does not match the file name the operator ends up with. Emit the final round in a trailer, or document the header as "no earlier than".

F3. Info: hand-rolled DER walk for the ACME key check

tls.rs::x509_leaf_spki is a manual TLV walk used by install_chain_pem to confirm the CA-issued leaf is for the enclave key. Exploiting a mis-parse buys an attacker nothing (the SigningKey handed to rustls is always the enclave key, so a mismatched cert only breaks handshakes), but a real parser (x509-parser is already in the lock file transitively) removes the question entirely — and the same helper is what you want for F1, where correctness does matter.

3. Time sources, rounding, round selection, sealing

I could not find a way to shorten the lock. Specifically checked:

Nothing at medium or above here. One note:

F4. Info: NSM timestamp is read from the COSE payload without verifying the COSE signature

attest::attestation_timestamp_ms parses cose_payload(signed)[2] and trusts it. That is fine because the bytes come straight from the /dev/nsm ioctl and the parent is not in that path, but the code would silently accept an unsigned document if this function were ever reused on a document received over the wire. Worth a #[doc(hidden)]-style comment or an assertion that the caller is the local NSM.

4. Leaks via diagnostics, errors, side channels

The log / log_record / log_infra split is real and, as far as I can tell, correctly applied on request paths: classify_upstream_error collapses upstream failures to fixed labels, dns.rs and net.rs only emit static strings on sender-driven paths, and no logger is installed so log/tracing output from rustls, hyper, boringtun and hickory is discarded. Bodies/URLs appear only in error responses to the sender, which is correct.

Two leaks that do escape:

F5. Medium: relay/Mullvad API error text is served publicly through the attestation endpoint and landing page

Files/functions: net::Dialer::json (bail!("{method} {host}{path}: status {status}: {}", String::from_utf8_lossy(&body[..300]))) → mullvad::run (net.set_mode(Mode::Down(format!("registration failed: {e:#}")))) → relay::Net::status().state → proxy::attestation (relay: state.net.status()) and proxy::landing.

Any anonymous client can GET /.well-known/attestation and read up to 300 bytes of the Mullvad API's response body, plus the exact API path and status. Mullvad account numbers are bearer credentials; an error body that echoes request context, or a future code path that formats the account into an error, becomes a public credential disclosure. It also gives an attacker a live view of the enclave's registration/handshake state.

Fix: reduce RelayStatus::state to a fixed enum (connecting / up / down) and keep the detailed text in log_infra only. Never format an HTTP response body into anything served to clients. As a secondary hardening, redact the account number from any error constructed in mullvad::register.

Related: proxy::landing string-substitutes {{relay}} into HTML without escaping, so this is also a stored-XSS vector on the proxy origin fed by a third-party API response. Given that the proxy already serves arbitrary upstream HTML on its own origin (see F13), the origin is worthless anyway, but escape it.

F6. Info: hostname exposure to Cloudflare/Google, and the "nobody but the upstream sees plaintext" framing

For Purpose::Upstream, dns.rs resolution goes through Net::connect_ip(…, Upstream), which requires the tunnel — so DoH queries exit from a Mullvad IP and the resolver cannot link them to the proxy. That is a good design and worth keeping. But the resolver still learns every upstream hostname a sender asks for, timestamped, and the README only says the parent doesn't. Say so explicitly in the threat model: DoH resolvers are a third party that learns request-derived metadata (hostnames), not just infrastructure.

F7. Info: production diagnostics go nowhere

Every claim of the form "the enclave logs it" (dropped records, sealing failures, WireGuard errors) is unobservable in production, because without --debug-mode the console is not attached and there is no other log sink. The operator cannot distinguish "no records because no traffic" from "records dropped for six hours". Consider a counters-only channel over vsock (monotonic counts, no request-derived data) so the operator has some signal without weakening the log-hygiene rule.

5. Correctness bugs that could drop/corrupt records or crash the enclave

panic = "abort" in [profile.release] makes every reachable panic a total loss of all in-flight records, which raises the stakes on everything in this section.

F8. High: memory exhaustion aborts the enclave; the documented bounds do not add up

Files: config/enclave.toml, crates/enclave/src/records.rs, crates/enclave/src/wg.rs::run/service_sockets, crates/enclave/src/main.rs accept loop, deploy/run-enclave.sh (--memory 1024).

The README says resource use is bounded so a sender cannot crash the enclave. Four separate accounting gaps mean it isn't:

  1. Sealing amplifies each record 3–4×, and the budget doesn't know. max_capture_bytes_total = 256 MiB bounds captured bytes. Then RecordGuard::drop decrements capture_bytes before sealing starts, and sealing allocates serde_json::to_vec(&record) (base64 inflates bodies 4/3 → ~340 MiB for a full budget), then age output, then tlock output. Two large exchanges finishing together comfortably exceed 1024 MiB. Decrement the budget only after the sealed bytes are handed to the sink, and count the sealing allocations against it.
  2. The record queue is bounded by count, not bytes. records::QUEUE_CAPACITY = 4096 records × up to ~180 MiB each. With max_body_bytes = 64 MiB, a handful of large queued records is fatal. Bound the queue in bytes.
  3. Per-connection tunnel buffers are large and multiply. Each wg::Cmd::Connect allocates 2 × TCP_BUF (128 KiB) plus a USER_QUEUE of 32 × up to 16 KiB (512 KiB). dns.rs::resolve opens two fresh DoH connections per uncached name (A and HTTPS, in tokio::join!, no request coalescing, no connection reuse), so one proxied request can hold three tunnel sockets ≈ 2 MiB. At max_in_flight = 256 that is ~500 MiB of WireGuard buffers alone. Coalesce concurrent lookups for the same name, reuse resolver connections, and shrink USER_QUEUE/TCP_BUF.
  4. Accepted connections are unbounded. main's loop spawns a task per accepted vsock stream with no cap and no idle timeout; only exchanges are limited. A client can open tens of thousands of TLS connections that never send a request (they pass the 20 s handshake timeout, then sit in keep-alive forever), each holding rustls buffers. Add a semaphore on accepted connections and an idle/keep-alive timeout.

Any one of these ends in abort() and the loss of every in-flight record — precisely the outcome the availability section says it is protecting against.

F9. Medium: one undeliverable record permanently stops all record delivery

File/function: records::worker.

loop { match deliver(...) { Ok(()) => break, Err(_) => { sleep(delay); delay = (delay*2).min(30s) } } }

The retry loop for the current record is unbounded and the worker is single-threaded, so any permanent failure (host disk full, ERR from handle_record, a data_len the host rejects) blocks the queue forever. The queue then fills and Sink::submit silently drops everything afterwards — with no observable log (F7). Fix: cap retries per record (e.g. 20 attempts / 10 minutes), then move on and increment a dropped counter; consider requeueing at the tail rather than head-of-line blocking.

F10. Medium: ECH rejection is not handled, and stale ECH configs break upstreams for up to an hour

Files/functions: net::Dialer::connect_tls, dns::Resolver::resolve.

connect_tls builds an EchMode::Enable config and never handles rejection. rustls surfaces server rejection of ECH as a handshake error (with retry configs attached), not as EchStatus::Rejected, so the "rejected" value in ech_status_str is effectively unreachable and the request fails with a 502 instead. Cloudflare rotates ECH keys routinely; combined with MAX_TTL = 3600 on the cached ech_config, a rotation can make an upstream unreachable through the proxy for up to an hour, and every failure is recorded as an errored exchange. Fix: on ECH rejection, retry once using the retry configs rustls returns (and refresh the cache entry), and fall back to non-ECH after that; drop the ECH config TTL cap to something short (60 s).

F11. Medium: parent-controlled beacon JSON is parsed in-process under panic = "abort"

File/function: beacon::fetch_from → DrandChain::verify_beacon → drand_core::beacon::ApiBeacon::verify.

The bytes are fetched through the parent (https_get to api.drand.sh; TLS-verified, so the parent can't inject — but the API host can, and a hostile relay/DNS answer path is exactly what this defends against). drand_core + ark-* deserialization of a crafted signature/randomness field is the one place untrusted bytes reach BLS deserialisation code that has not been hardened for adversarial input. A panic there aborts the enclave and loses in-flight records. Mitigate cheaply: validate the JSON shape and hex field lengths yourself before calling verify, and/or perform beacon verification in a spawn_blocking on a build without panic=abort — the latter isn't possible with the current profile, so pre-validation is the practical fix. More generally, reconsider panic = "abort" for the enclave: unwind plus a catch_unwind boundary per connection would turn "lose 256 records" into "lose one".

F12. Medium: trivial anonymous denial of service, with no possibility of rate limiting

Files/functions: proxy::handle (UPSTREAM_HEADERS_TIMEOUT = 600s), proxy::forward_entry (max_in_flight = 256), proxy::attestation, attest::Attester::trusted_time_ms.

Fixes: have the parent prepend a source address line on the vsock connection (it's untrusted, but useful for coarse limiting and cannot weaken confidentiality); add a total per-request deadline and lower the header timeout; move NSM calls onto a dedicated blocking task with a small cache (a timestamp reused for ≤1 s only raises the lock, so caching is safe in the conservative direction); rate-limit the attestation endpoint.

F13. Low: WireGuard/TCP stack correctness details

File: wg.rs::service_sockets.

F14. Low: record fidelity when the response finishes before the request body

TeeBody::poll_frame finalises the record (this.guard.take()) as soon as the response body ends. If the upstream answered early while the sender was still uploading, Pending has already been taken, so subsequent append calls are no-ops: request.body_len under-reports and body_truncated stays false even though bytes are missing (only body_complete: false hints at it). Also in_flight is decremented while the request body is still streaming, so the concurrency limit under-counts. Set body_truncated = true when finalising with request_complete == false, and hold the in-flight permit until both bodies are done.

F15. Info: attestation nonce and TLS-ALPN handling

6. Design points I disagree with

The browser-valid certificate is a net negative. The README is honest that it "adds no assurance", but it does add: an entire ACME stack (instant-acme, an extra HTTP client, a mutable certificate resolver, a challenge-certificate path) inside the enclave's TCB; a second reason for install_chain_pem's hand-rolled DER walk to exist; and — most importantly — an affordance that invites exactly the unverified use the design is supposed to make impossible. A padlock on proxy.girl.surgery plus a / landing page describing timelock guarantees is a very effective way to get people to send secrets through an endpoint they never attested. Combined with the fact that /<host>/<path> serves arbitrary attacker-controlled content on the proxy's own origin, I would drop [acme] and serve only the self-signed certificate, so that using the proxy at all requires a client that pinned the attested key.

panic = "abort" is the wrong choice for this enclave. The threat model explicitly values "never lose a record", and the code goes to some trouble (the RecordGuard, the fail-closed-at-start rule) to honour it. abort converts any single-request panic — in rustls, hyper, hickory, drand_core, smoltcp — into loss of up to 256 records. Unwind plus a per-connection catch_unwind costs a little binary size and preserves the property you actually care about. (Note the dead code this creates today: Err(_) => crate::log("record sealing task panicked") in RecordGuard::drop can never run.)

Verdict

The architecture is sound and the details are unusually careful: the two-layer seal genuinely requires both the operator's key and a published round; the time logic is conservative in every direction I could probe (signed NSM timestamps, monotonic floor, beacon floor, upward rounding, max at both start and finish); in-enclave DoH and the fail-closed Mullvad path really do keep upstream identities away from the parent; and the diagnostics discipline is a real, enforced boundary rather than a slogan. Against the parent instance and the network, I believe the confidentiality claim.

I would not yet trust this deployment to enforce its stated properties, for two reasons. First, F1: the single check on which the entire sender-facing guarantee rests — that the attestation document binds this TLS key — is not performed by any code in this repository, is delegated to a third-party function whose field convention (public_key) does not match what the enclave populates (user_data), and is covered by zero tests. Until that check is explicit, tested against a captured document, and negative-tested with a swapped certificate, an operator-run MITM that relays a genuine enclave's attestation is a live possibility and verify cannot distinguish it. Second, F8 and F11: the availability bounds the README advertises do not close (sealing amplification, byte-unbounded record queue, unbounded accepted connections, half a gigabyte of per-socket WireGuard buffers at the configured concurrency, all inside 1024 MiB with panic = "abort"), so a sender can very likely abort the enclave and destroy in-flight records — which is the operator's guarantee, not the sender's, but it is stated as a goal.

Before I used it: fix F1 with an in-CLI user_data == SPKI(cert) assertion plus a fixture test; fix the memory accounting in F8 (decrement the capture budget after sealing, bound the record queue in bytes, cap accepted connections, shrink tunnel buffers) and re-measure peak RSS under a 256-way load test with 64 MiB bodies; bound the retry loop in F9; stop serving API error text in RelayStatus::state (F5); handle ECH rejection (F10); pre-validate beacon JSON (F11); and switch the release profile to unwind with a per-connection panic boundary. I would also want the reproducible build actually reproduced by a second party at the claimed commit, since every property above is conditional on the PCRs meaning what the README says they mean.