Review by qwen/qwen3.8-max-0902

commit: 765b6710b621474e53c575a0ae3f02b76643355d
usage: {'prompt_tokens': 112600, 'completion_tokens': 14500, 'total_tokens': 127100, 'cost': 0.3122, 'is_byok': False, 'prompt_tokens_details': {'cached_tokens': 0, 'cache_write_tokens': 0, 'audio_tokens': 0, 'video_tokens': 0}, 'cost_details': {'upstream_inference_cost': 0.3122, 'upstream_inference_prompt_cost': 0.2252, 'upstream_inference_completions_cost': 0.087}, 'completion_tokens_details': {'reasoning_tokens': 12906, 'image_tokens': 0, 'audio_tokens': 0}}
time: 219s

Security Review: timelock-proxy @ 765b6710

Findings

No Critical Findings

After careful review of the sealing construction, key management, attestation binding, and data flow, I found no path by which the operator, parent instance, relay provider, or network attacker can read request/response content before the drand round is published.

No High Findings

The lock-period enforcement is sound. The round selection uses max(trusted_start + lock_seconds, verified_beacon_time + lock_seconds) with upward rounding, and the Finalizer takes max(initial_round, finish_based_round), so neither the operator nor the parent can shorten the effective lock.

Medium Findings

M-1: Relay error details leak into the public attestation response

File: crates/enclave/src/mullvad.rs, run(); crates/enclave/src/relay.rs, Net::status()
What the attacker does: Fetches GET /.well-known/attestation repeatedly while the relay is failing.
Why the code allows it: Mode::Down(format!("registration failed: {e:#}")) stores the full error chain (which can include up to 300 bytes of Mullvad API response body from Dialer::json()'s error path) in the Mode enum. Net::status() then exposes this string in the RelayStatus.state field, which is serialized into the attestation response.
Impact: An observer learns operational details about the Mullvad account (e.g., "account not found", "device limit reached", HTTP error bodies). This is infrastructure metadata, not sender request data, so it does not violate the primary confidentiality goal, but it could aid an attacker in disrupting the relay (e.g., confirming a guessed account number is valid).
Suggested fix: Reduce the Mode::Down string to a fixed category (e.g., "registration failed", "handshake timeout") and log the full error via log_infra only.

Low Findings

L-1: x-timelock-proxy-unlock-round is a lower bound, not the actual round

File: crates/enclave/src/proxy.rs, forward_entry()
What the sender observes: The header announces initial_round, computed from the start timestamp. The actual sealing round is max(initial_round, finish_based_round), which can be higher for long exchanges.
Why this matters: A sender who computes time_of_round(initial_round) to know when the record becomes readable may be surprised when it remains locked longer. The README's wording ("the drand round before which the operator cannot open it") is technically correct but could mislead a sender into thinking it is the exact round.
Suggested fix: Document explicitly that the header is a guaranteed minimum, or add a note in the response that the actual round may be higher.

L-2: Hand-rolled X.509 DER parser for SPKI extraction

File: crates/enclave/src/tls.rs, x509_leaf_spki()
What the attacker does: Presents a malformed certificate via a compromised ACME CA (extremely unlikely given Let's Encrypt's controls).
Why the code allows it: The function manually walks DER TLVs without a full ASN.1 parser. It does not validate tag types for intermediate fields (serial, sigalg, issuer, validity, subject) beyond skipping them.
Impact: A crafted certificate could theoretically cause the parser to misidentify the SPKI offset, but the subsequent byte-for-byte comparison with self.spki_der makes a false positive astronomically unlikely. A false negative (rejecting a valid cert) would only cause ACME to fail, falling back to self-signed.
Suggested fix: Use the x509-parser crate (already in the dependency tree via rcgen) for robustness, or add tag-type assertions for each skipped field.

L-3: No explicit bound on concurrent ACME HTTP requests

File: crates/enclave/src/acme.rs, DialerHttp::request()
What the attacker does: Cannot directly trigger this (ACME is enclave-initiated), but a misbehaving ACME server could cause the enclave to open many concurrent outbound connections during the poll_ready / poll_certificate loops.
Why the code allows it: instant-acme's RetryPolicy::default() retries with backoff, but there is no cap on total attempts. Each retry opens a new TLS connection through the relay.
Impact: Bounded by the relay's own connection limits and the fact that the enclave has a single ACME task. Not exploitable by a sender.
Suggested fix: Add a maximum retry count or total timeout to the ACME flow.

Informational

I-1: TLPROXY_WG_DEBUG environment variable enables state-change logging

File: crates/enclave/src/wg.rs, service_sockets()
Note: The log messages are fixed strings ("wireguard connection state changed", "wireguard peer closed connection") and carry no request-derived data. In production the variable is unset. No action needed.

I-2: The Finalizer::drop() fallback for finish time

File: crates/enclave/src/proxy.rs, Finalizer::drop()
Note: If the NSM is unavailable for 30 seconds at record-sealing time, the code falls back to start_timestamp + monotonic_elapsed. The README explicitly documents this as a deliberate choice and explains why it is safe (it is a lower bound on the true finish time, so the lock is never shorter than the start-based guarantee). I agree with the design choice.

I-3: Reproducible build depends on nitro-cli version pinning

File: build/build-eif.sh
Note: The script refuses to run with a different nitro-cli version. This is correct and well-documented. A verifier on a different version gets an explicit error rather than a silent PCR mismatch.

Overall Verdict

I would trust this deployment to enforce its stated properties, with the following caveats:

The core security architecture is sound: TLS terminates inside the enclave, records are sealed (age inside tlock) before leaving enclave memory, the lock round is computed from hardware-signed timestamps with conservative rounding, the attestation binds the TLS key to the measured code, and the verification tool correctly checks the full chain from nonce to PCRs to recipient. The diagnostic discipline (compile-time strings on request paths, fixed error categories) is consistently applied. The WireGuard relay fails closed for upstream traffic. I found no path for the operator, parent, relay, or network attacker to read content early.

Before relying on this in production I would require: (1) a fix for M-1 to avoid leaking Mullvad account state publicly, (2) confirmation that the drand_core 0.0.19 BLS verification has been independently tested against known-good quicknet beacons (the unit test in lib.rs only checks that a garbage signature fails), and (3) an operational runbook for the case where the record queue fills and records are dropped, since the README's "no record is ever lost" property depends entirely on the parent daemon's availability.