Review by moonshotai/kimi-k2-0905

commit: 67eb6e6f672501be1c9f67cde1b979efebdcf981
usage: {'prompt_tokens': 50077, 'completion_tokens': 2531, 'total_tokens': 52608, 'cost': 0.0363737, '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.0363737, 'upstream_inference_prompt_cost': 0.0300462, 'upstream_inference_completions_cost': 0.0063275}, 'completion_tokens_details': {'reasoning_tokens': 0, 'image_tokens': 0, 'audio_tokens': 0}}
time: 42s

I'll perform a thorough security review of the timelock-proxy codebase, focusing on the priority areas specified.

Finding 1: Trusted Time Fallback Could Allow Shorter Locks (HIGH)

File: crates/enclave/src/proxy.rs, function RecordGuard::drop

Severity: High

Issue: When the NSM is unavailable at completion time, the code falls back to monotonic_finished_at_ms = p.started_at_ms.saturating_add(elapsed_ms). However, elapsed_ms is computed using p.started_at which is std::time::Instant::now() - a local, untrusted clock. While the README states this "cannot be later than reality," the actual concern is different: if the enclave is paused (e.g., via Nitro's pause/resume or live migration), Instant::now() continues from where it left off, but wall clock time has advanced. This means elapsed_ms could be significantly shorter than real elapsed time if the enclave was paused.

Why the code allows it: The started_at Instant is captured at request start, but Instants are not monotonic across suspend/resume in the same way that CLOCK_MONOTONIC would be in a real OS - they're based on tokio's internal timing which may use different backends.

Suggested fix: Remove the fallback entirely and refuse to seal records when trusted time is unavailable at completion. The README's stated rationale ("fail closed at the start, never lose a record") prioritizes availability over the security guarantee, but this is the wrong tradeoff. Alternatively, if keeping the record is essential, use a much more conservative estimate: finished_at_ms = started_at_ms + max(elapsed_ms, MIN_LOCK_SECONDS * 1000) to ensure at least the minimum lock duration.

Finding 2: No Binding of TLS Key to Specific Proxy Instance in Attestation (MEDIUM)

File: crates/enclave/src/tls.rs and crates/enclave/src/attest.rs

Severity: Medium

Issue: The attestation document's user_data contains only the SPKI of the TLS key. A malicious parent instance could theoretically run multiple enclaves with the same code (same PCRs) but different TLS keys, and present any of them. While the sender verifies the TLS key matches the attestation, there's no binding to a specific "instance" or boot session. More critically, the nonce in the attestation request is not incorporated into any long-term state that would bind the verification to a specific TLS session.

Why the code allows it: The attestation is fetched over a TLS connection, but there's no cryptographic binding between the TLS session and the attestation document beyond the certificate being presented. A sophisticated attacker could potentially race or relay.

Suggested fix: Include the TLS session's key exchange transcript or a derived value in the attestation user_data, or have the enclave sign the attestation document with the TLS key itself to prove simultaneous possession.

Finding 3: ECH Status Leaks Upstream Identity Information (MEDIUM)

File: crates/enclave/src/proxy.rs, function forward

Severity: Medium

Issue: The response header x-timelock-proxy-ech reveals whether ECH was accepted, rejected, or not offered. This leaks information about whether the upstream host publishes ECH configs, which correlates with specific hosting providers (notably Cloudflare). A network observer or the parent instance seeing this header can narrow down which upstreams are being contacted.

Why the code allows it: The header is added unconditionally to all responses.

Suggested fix: Remove this header or make it optional. The sender already knows whether they requested ECH-capable upstreams; they don't need confirmation from the proxy.

Finding 4: Record ID Generation Uses Untrusted Time (LOW)

File: crates/enclave/src/proxy.rs, function forward_entry

Severity: Low

Issue: The record ID is generated as format!("{}-{:06}", tlproxy_common::unix_now_ms(), seq). While unix_now_ms() is only used for ID generation and not for security-critical timing, this uses the local clock which could be manipulated by the parent through various side channels. The seq counter provides uniqueness within a boot, but the timestamp component could be manipulated to make records appear older or newer than they are.

Why the code allows it: The ID is meant to be opaque, but if used for ordering or debugging, manipulated timestamps could confuse analysis.

Suggested fix: Use the trusted NSM timestamp for the ID prefix, or use a purely random value.

Finding 5: No Explicit Verification of Minimum Lock Duration in Sealing (MEDIUM)

File: crates/enclave/src/proxy.rs, function RecordGuard::drop

Severity: Medium

Issue: While EnclaveConfig::parse validates lock_seconds >= MIN_LOCK_SECONDS, the actual sealing in RecordGuard::drop uses state.cfg.lock_seconds without re-verification. If there's any code path that could modify cfg.lock_seconds after parsing (e.g., dev mode override), the minimum could be bypassed.

Why the code allows it: The dev mode override at main.rs:78-81 explicitly allows shorter locks via TLPROXY_DEV_LOCK_SECONDS.

Suggested fix: Add a runtime assertion before sealing: assert!(state.cfg.lock_seconds >= MIN_LOCK_SECONDS || state.dev), or move the check to the sealing function itself.

Finding 6: Mullvad Account Number Not Authenticated (INFO)

File: crates/enclave/src/mullvad.rs, function register

Severity: Info

Issue: The Mullvad account number is passed from the parent via fetch_host_config() and used without any authentication. A malicious parent could supply a different account number, causing the enclave to register with an attacker's account. This doesn't break confidentiality (the tunnel still encrypts), but could affect availability or allow traffic correlation if the attacker controls the account.

Why the code allows it: The design explicitly states this is "untrusted input; only used to reach the relay."

Suggested fix: Document this more prominently. Consider having the operator include a hash of the expected account number in the enclave config, verified at runtime.

Finding 7: DNS Cache TTL Manipulation (LOW)

File: crates/enclave/src/dns.rs, function resolve

Severity: Low

Issue: The DNS cache uses TTL values from upstream responses with only MIN_TTL and MAX_TTL bounds. A malicious DoH resolver (or one compromised) could return very short TTLs to force frequent re-resolution, potentially enabling more timing analysis.

Why the code allows it: The code trusts resolver-provided TTLs within broad bounds.

Suggested fix: Enforce a minimum cache duration regardless of TTL, or pin resolutions for the lifetime of a connection.

Finding 8: No PCR Binding in Sealed Records (INFO)

File: crates/common/src/lib.rs, function seal

Severity: Info

Issue: Sealed records don't include any binding to the enclave's PCR values. While this isn't required for the stated security goals, it means records can't be cryptographically tied to a specific code version after the fact.

Why the code allows it: Not part of the threat model.

Suggested fix: Consider including PCR values in the record JSON before sealing, for auditability.

Finding 9: WireGuard Tunnel Key Generation Timing (LOW)

File: crates/enclave/src/mullvad.rs, function register

Severity: Low

Issue: The WireGuard private key is generated with StaticSecret::from(rand::random::<[u8; 32]>()). The rand crate's default RNG in Rust uses getrandom() which in an enclave context should use the NSM's entropy source, but this isn't explicitly verified.

Why the code allows it: Assumes proper entropy source configuration.

Suggested fix: Explicitly verify or document that getrandom is properly configured to use NSM entropy in the Nitro environment.

Finding 10: ACME Challenge Certificate Key Reuse (INFO)

File: crates/enclave/src/tls.rs, function set_challenge

Severity: Info

Issue: The ACME TLS-ALPN-01 challenge certificate uses a freshly generated throwaway key, which is correct. However, the comment says "A throwaway key is fine: the challenge cert carries no trust" - this is true, but the implementation generates a new key for each challenge, which is good practice.

Why the code allows it: This is actually correct behavior, noting as verification.

Finding 11: No Rate Limiting on Attestation Endpoint (MEDIUM)

File: crates/enclave/src/proxy.rs, function handle

Severity: Medium

Issue: The attestation endpoint (/.well-known/attestation) has no rate limiting. An attacker could request many attestations with different nonces, potentially consuming NSM resources or causing availability issues.

Why the code allows it: No rate limiting infrastructure in place.

Suggested fix: Add a simple per-connection or global rate limit for attestation requests.

Finding 12: Host Can Manipulate UDP Datagram Timing (MEDIUM)

File: crates/host/src/main.rs, function handle_udp

Severity: Medium

Issue: The UDP relay between enclave and Mullvad relay is length-prefixed but has no integrity protection. The parent can delay, reorder, or drop datagrams. While WireGuard has its own crypto, timing manipulation could affect handshake success or keepalive timing, potentially causing unnecessary re-registration and device slot exhaustion.

Why the code allows it: The design treats the parent as untrusted for confidentiality but availability is assumed.

Suggested fix: Document this limitation. The WireGuard protocol should be robust to this, but device slot exhaustion is a concern.

Finding 13: Record Truncation Not Cryptographically Bound (INFO)

File: crates/enclave/src/proxy.rs, Pending::append

Severity: Info

Issue: When bodies are truncated due to size limits, the body_truncated flag is set in the JSON record, but this isn't cryptographically protected beyond the age+tlock encryption. An operator with the age key could potentially modify this flag after decryption (though this would require re-encryption which they can't do without the tlock round key).

Why the code allows it: The threat model assumes the operator is the legitimate recipient.

Suggested fix: Not needed - the operator is the intended recipient.

Overall Verdict

The timelock-proxy implementation is carefully engineered and largely achieves its stated security goals, with notable strengths in its clean separation of trust, comprehensive use of Nitro attestation, and defense-in-depth for upstream privacy (DoH, ECH, WireGuard relay).

However, I would not fully trust this deployment without modifications, specifically:

Critical: The trusted time fallback in RecordGuard::drop must be fixed. The current implementation could allow shorter locks than intended if the enclave experiences pauses. The simplest fix is to refuse sealing when trusted time is unavailable at completion, accepting that some records may be lost rather than weakening the time guarantee.
Important: The dev mode override for lock_seconds needs stronger guards to ensure it cannot be accidentally enabled in production, and runtime verification of the minimum lock duration should be added.
Recommended: Remove or make optional the x-timelock-proxy-ech header to reduce information leakage.
Recommended: Add rate limiting on the attestation endpoint.

The codebase demonstrates strong security engineering discipline with its compile-time string constraints on logging, careful memory budgeting, and reproducible build infrastructure. The threat model is well-articulated and the implementation largely follows it. With the time fallback issue addressed, this would be a trustworthy deployment for its intended use case.