Review by z-ai/glm-5.3-flash

commit: 765b6710b621474e53c575a0ae3f02b76643355d
usage: {'prompt_tokens': 96969, 'completion_tokens': 21892, 'total_tokens': 118861, 'cost': 0.012745675, '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.012745675, 'upstream_inference_prompt_cost': 0.007272675, 'upstream_inference_completions_cost': 0.005473}, 'completion_tokens_details': {'reasoning_tokens': 18954, 'image_tokens': 0, 'audio_tokens': 0}}
time: 661s

Security review: timelock-proxy @ 765b671

Summary of what I checked against

The README's goals: (1) no one but the recipient reads content before the lock, (2) the operator can't read before the drand round, (3) senders can verify via attestation + reproducible build, (4) no request-derived data in diagnostics/side channels, (5) correctness of the userspace network stack and capture, (6) availability.

No critical findings. Goals (1) and (2) hold on the code as written: TLS terminates on a boot-generated key that never leaves the enclave; the only bytes handed to the parent are sealed age ∘ tlock ciphertext (crates/common/src/lib.rs::seal), the vsock record path carries only ciphertext (records.rs, framing.rs), and the host daemon writes opaque blobs. The lock round is derived, not chosen: beacon.rs::lock_round_at_ms takes max(NSM signed timestamp, newest verified beacon), adds lock_seconds, rounds up via round_after (verified by the unit test), and proxy.rs's finalizer takes max(initial_round, finish-based round) with a re-applied MIN_LOCK_SECONDS floor outside dev mode. The parent can withhold beacons (lengthening only) and cannot forge NSM timestamps or future drand signatures. Fail-closed at start (trusted_time_ms error → 503) and fail-closed on relay-down for upstream traffic (relay.rs::connect_ip) are both implemented as claimed.

High

H-1. Sealing a large record can OOM the enclave, crashing it and losing all in-flight and queued records

File/function: crates/enclave/src/proxy.rs (Finalizer::drop, Pending::into_record) + crates/common/src/lib.rs::seal + deploy/run-enclave.sh (1024 MB) + config/enclave.toml.

Attack: A sender uploads a 64 MiB body and receives a 64 MiB response (both within max_body_bytes). At finalization, spawn_blocking builds the full JSON record (base64 inflates 128 MiB to ~176 MiB), then seal holds the JSON + the age output (~352 MB) and then the age output + the tlock output (~352 MB) simultaneously — a peak of roughly 500 MB for one sealing job, with up to 4 permitted concurrently (seal_permits = 4). Meanwhile the 256 MB capture_bytes budget is not released until sealing completes, and up to 256 MB of sealed records may sit in the sink queue. One large seal plus a full capture budget plus a full queue exceeds the 1 GB enclave; two concurrent large seals certainly do. The process is killed (or panic="abort" on any allocation failure path), destroying the TLS key, every in-flight record, and everything in the undelivered queue.

Why the code allows it: the README claims "resource use is bounded … crashing it would lose in-flight records, so resource use is bounded", but the bounds are not composed: max_capture_bytes_total bounds capture, seal_permits bounds count (not bytes) of sealing jobs, and max_queued_record_bytes bounds the queue — nothing bounds their sum against the enclave's actual memory.

Fix: weight seal_permits by record size (a Semaphore of bytes, e.g. 256 MB, acquiring record_len * 3), and/or lower max_body_bytes to something that provably fits (e.g. 8–16 MiB), and/or release capture_bytes before serialization by moving the buffers into the sealing task and accounting them there. Also state the real memory arithmetic in the README.

Medium

M-1. Head-of-line blocking in the record delivery worker turns any persistent parent-side failure into silent, total record loss

File/function: crates/enclave/src/records.rs (worker).

Attack/trigger: the parent returns ERR (e.g. disk full — handle_record in crates/host/src/main.rs fails on tokio::fs::write) or the connection keeps failing. The worker's inner loop retries the same record forever with backoff; rx.recv() is never reached again. The mpsc queue (4096 / 256 MB) fills, and every subsequent record is dropped with only a log line. A full disk on the parent therefore loses all records indefinitely while the proxy otherwise looks healthy.

Why the code allows it: delivery is strictly sequential with unbounded per-record retry; there is no skip/requeue or per-record attempt budget.

Fix: on repeated failure of the head record, either move it to a side queue and continue, or bound total retries and drop that record (logged) rather than all of them; or deliver over a persistent connection with per-record ack/continue semantics.

Low

L-1. No consistency check between the NSM clock and a verified beacon before the first beacon arrives

File/function: crates/enclave/src/beacon.rs::lock_round_at_ms, crates/enclave/src/attest.rs::trusted_time_ms.

The lock is max(NSM timestamp, beacon lower bound), which is correct once a beacon has been verified. But requests served in the window before the first refresh_loop success (boot + relay/DoH setup time) rely solely on the NSM timestamp. If the hypervisor-supplied clock were badly stale, records would be sealed to rounds that are already published — the seven-day property silently fails (content is still age-protected, so only the "not before" property breaks, and only the operator could read anyway). The beacon machinery exists precisely to catch this but is not required to have succeeded before serving.

Fix: refuse (503) requests until verified_round() > 0, or until trusted_now_ms >= time_of_round(verified_round) once known. Cost: a few seconds of unavailability at boot.

L-2. Mullvad API response bodies (up to 300 bytes) reach the public landing page and attestation response

File/function: crates/enclave/src/net.rs::json (bail!("{method} {host}{path}: status {status}: {}", …body…)) → crates/enclave/src/mullvad.rs::run (Mode::Down(format!("registration failed: {e:#}"))) → relay.rs::status → proxy.rs::landing/attestation.

The stated policy confines dynamic text to infrastructure inputs, and this is infrastructure-derived, but unlike the logs it is published to every sender and to the world. A Mullvad error body could echo account-identifying detail. Fix: reduce relay status to fixed states plus the relay hostname; keep detailed error text out of Mode::* strings.

L-3. classify_upstream_error substring-matches error text that embeds sender-controlled hostnames

File/function: crates/enclave/src/proxy.rs::classify_upstream_error with contexts like format!("tls to {host} ({addr})") from net.rs::connect_tls.

A host named e.g. my-timeout.example.com that fails TLS is labeled "timeout" instead of "tls". No data leak (the sender-facing body is always a fixed label), but the classification is fragile and the invariant "labels come from strings this crate attaches" is only accidentally true. Fix: pass typed error variants rather than matching formatted text.

L-4. Finalizer::drop outside a tokio runtime leaks in_flight permanently

File/function: crates/enclave/src/proxy.rs::Finalizer::drop. If Handle::try_current() fails, the record is never sealed and in_flight/capture_bytes are never decremented — after 256 such leaks the proxy 503s forever. I could not construct a path where the drop happens off-runtime (guards live in hyper tasks), so this is defensive only. Fix: decrement the counters before the try_current check, or use tokio::runtime::Handle::current() captured at guard creation.

Info

• I-1. crates/enclave/src/net.rs::ech_status_str can return "grease"/"offered", outside the accepted|rejected|not-offered set the README documents for x-timelock-proxy-ech.
• I-2. proxy.rs::parse_target and the attestation error path echo sender-controlled input (host, nonce-parse error) back to the sender in response bodies. Reflected-to-sender only; no record is created and nothing is logged. Acceptable, but worth noting the "no request-derived text" rule is about diagnostics, and these are responses.
• I-3. attest.rs NSM ioctls are blocking calls made directly on the async runtime (2 workers, cpu-count 2), including per-request and per-finalization. Millisecond-scale today; a semaphore (nsm_permits) bounds concurrency but blocked workers can still stall the accept loop briefly.
• I-4. tls.rs::x509_leaf_spki is a hand-rolled DER walker used to validate the ACME leaf's SPKI. It looks correct (version skip, five fields, SPKI tag check) and only ever rejects mismatching certs, but a crate parser would be more robust.
• I-5. If the response body finishes before the request body (early response), the finalizer takes pending and subsequent request bytes are neither captured nor counted (append is skipped entirely) — the record correctly shows body_complete: false, so this is fidelity, not loss of the guarantee. Similarly, TeeBody::PinnedDrop relies on is_end_stream(); a conservative false for a genuinely empty body would produce a spurious "response body did not complete" error in the record.
• I-6. crates/cli/src/attested.rs relies on attestation-doc-validation::validate_attestation_doc_against_cert to (a) verify the COSE chain to the AWS Nitro root and (b) check user_data == the presented cert's SPKI. The comment asserts both; I would add an explicit local re-check of doc.user_data == spki(cert_der) so the binding does not silently depend on a third-party crate's semantics. Everything else in verify is sound: fresh 32-byte nonce, same-connection fetch, pem_der == cert_der, PCR0–2 comparison with all-zero (debug) rejection, mandatory --pcrs behind an explicitly named flag, and the CaptureCert verifier still validates handshake signatures so the captured cert is provably the key the peer holds. I found no way for a non-enclave endpoint to pass tlproxy verify short of breaking the Nitro PKI.
• I-7. WireGuard/smoltcp stack (wg.rs): backpressure accounting, EOF semantics, connect timeouts, and cleanup all look correct; the one softness is the for _ in 0..4 settle loop in run, which can leave socket work to the next 50 ms tick under burst load (throughput, not correctness). Data silently dropped on send_slice error would break the inner TLS handshake rather than corrupt it undetected — acceptable failure mode.

Explicit "nothing found" statements

• Critical: none. I found no path by which the operator, the parent, the relay, or a network attacker obtains request/response content before the lock, and no way for the operator to shorten the lock below the configured floor.
• Goal 1 (pre-lock confidentiality): no findings beyond documented metadata (timing, sizes, upstream IP, SNI without ECH).
• Goal 2 (lock shortening): no findings; rounding is consistently upward and both time sources are conservative. L-1 is a narrow boot-window robustness gap, not an exploitable shortening.
• Goal 4 (leaks): no request-derived data reaches logs or sender-facing errors; L-2 is infrastructure-derived text on public endpoints.
• Goal 5 (correctness): no record-corrupting or crash-on-input bugs found in the capture path, framing, or ACME handling; I-5 covers the minor fidelity gaps.

Verdict

The core design is sound and, unusually for this genre, the implementation largely matches the README's claims: sealing is done before any byte leaves the enclave, the lock round is derived from signed time with conservative rounding and a hard floor, the attestation binding is tight, and the diagnostics discipline is real and mostly enforced by construction. I would not yet trust the deployment to enforce its stated properties unconditionally, because the availability story undermines the recording guarantee: H-1 means a single sender can crash the enclave within the advertised limits, destroying every in-flight and queued record, and M-1 means a full disk on the parent silently loses everything thereafter. Before trusting it I would require: (a) byte-weighted sealing or a reduced max_body_bytes with the memory arithmetic published, (b) non-blocking record delivery, (c) a verified-beacon gate at boot (L-1), and (d) an explicit in-CLI re-check of the user_data↔SPKI binding rather than reliance on the third-party validator. With those in place, and having reproduced the PCRs from a clean checkout, I would consider the stated properties enforced.