reviews/openai__gpt-5.6-terra-pro.md

Review by openai/gpt-5.6-terra-pro

commit: 67eb6e6f672501be1c9f67cde1b979efebdcf981
usage: {'prompt_tokens': 223090, 'completion_tokens': 20153, 'total_tokens': 243243, 'cost': 0.5074148, 'is_byok': False, 'prompt_tokens_details': {'cached_tokens': 100334, 'cache_write_tokens': 0, 'audio_tokens': 0, 'video_tokens': 0}, 'cost_details': {'upstream_inference_cost': 0.5074148, 'upstream_inference_prompt_cost': 0.2655788, 'upstream_inference_completions_cost': 0.241836}, 'completion_tokens_details': {'reasoning_tokens': 10905, 'image_tokens': 0, 'audio_tokens': 0}}
time: 117s

Findings

High — fallback finish-time logic can shorten the lock after an enclave pause / stalled clock

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

Attacker action: The parent/operator causes the enclave to be paused, descheduled, or otherwise makes its monotonic elapsed-time measurement lag real time, while also causing the completion-time NSM call to fail or be unavailable. The request is allowed to complete and its record is sealed using the fallback.

Why the code allows it: On failure to obtain a finish attestation timestamp, the code uses:

let monotonic_finished_at_ms = p.started_at_ms.saturating_add(elapsed_ms);
...
Err(_) => monotonic_finished_at_ms

and then locks for lock_seconds after that value. This only safely enforces a delay after real completion if Instant::elapsed() is guaranteed to include every period for which the enclave can be paused or not scheduled. The code and README assume that a lower bound on real completion time makes the resulting lock longer:

“That value is a lower bound on the true time ... so the resulting lock can only be longer than seven days”

That is backwards. If estimated_finish <= real_finish, then:

estimated_finish + 7 days <= real_finish + 7 days

so the record can unlock before seven days after the exchange actually completed. initial_round prevents unlock before seven days after request start, but not before seven days after completion for a long-running or paused request.

The threat model explicitly includes a hostile parent/operator. A parent can at minimum cause scheduling starvation and network/transport failure; whether a particular Nitro pause stops the relevant Linux monotonic clock must not be assumed without an AWS/Nitro guarantee.

Suggested fix: Do not treat start_timestamp + elapsed as a safe completion timestamp unless there is a documented, tested hardware/OS guarantee that the elapsed clock advances through every parent-controlled suspension condition. The conservative fail-closed option is to retain the record in enclave memory until a fresh trusted completion timestamp is available, or seal it to a deliberately later round with a configured worst-case pause allowance. If preserving every record is more important than bounded availability, persist a sealed “pending completion” state only after selecting a conservatively future round. The README’s lower-bound argument should be corrected.

High — unbounded pre-request and post-request work defeats the stated resource bounds

Files / functions:

Attacker action: A network attacker or the parent opens many TLS connections and either completes TLS but never completes HTTP/1.1 headers, repeatedly queries the attestation endpoint, or submits many small completed exchanges quickly enough to create a sealing backlog.

Why the code allows it:

  1. max_in_flight is charged only in forward_entry, after TLS has completed, Hyper has parsed an HTTP request, the target has parsed, and trusted_time_ms() has succeeded. It does not limit:

    • accepted TCP connections;
    • concurrent TLS handshakes/tasks;
    • established HTTP/1.1 connections waiting indefinitely for headers;
    • attestation requests;
    • landing/health requests.

    Every accepted stream creates an unbounded Tokio task:

    tokio::spawn(async move {
    let tls_stream = ...
    ...
    serve_connection(io, svc).await
});

    There is a TLS handshake timeout, but no HTTP header/request timeout and no connection semaphore before spawning. A slowloris attacker can consume enclave sockets, tasks, parser state, and memory before max_in_flight applies.

  2. The sealed-record queue does not bound sealing work or memory. On each completed exchange, RecordGuard::drop spawns a blocking task that retains the full Record, serializes it, creates the age ciphertext, then creates the tlock ciphertext:

    handle.spawn(async move {
    let sealed = tokio::task::spawn_blocking(move || -> Result<Vec<u8>> {
        let json = serde_json::to_vec(&record)?;
        tlproxy_common::seal(...)
    }).await;
});

    in_flight and capture_bytes are decremented before this work is queued. Thus an attacker can cycle exchanges and build an unbounded backlog of records and spawn_blocking jobs. The Sink queue’s 4096-item cap only drops records after the expensive serialization/encryption has completed, so it is not a bound on the pending plaintext records or sealing allocations. A record can contain up to two 64 MiB captured bodies, plus JSON/base64 and encryption copies.

Suggested fix: Apply a semaphore before spawning per-connection work, with separate bounded limits for connections, TLS handshakes, HTTP header parsing, attestation generation, and exchanges. Configure Hyper/header read deadlines and idle connection limits. Replace unconstrained spawn_blocking sealing with a bounded worker queue that owns the record before releasing the in-flight/capture reservation; when full, choose an explicit policy (backpressure, reject new requests, or drop records) before allocating/encrypting more data. Account for serialized/base64/encryption expansion in the memory budget.

High — WireGuard userspace TCP stack has an unbounded per-connection transmit queue

File / function: crates/enclave/src/wg.rs, run and service_sockets

Attacker action: A sender streams a large request body to an upstream that is slow, unreachable after TCP establishment, flow-controlled, or simply does not consume data. This is especially effective across multiple concurrent proxied requests.

Why the code allows it: TunnelStream::poll_write initially has bounded backpressure through a USER_QUEUE channel, but the tunnel event loop drains that bounded channel into Conn.pending without a size bound:

UserMsg::Data(b) => c.pending.push_back(b),

service_sockets only removes from c.pending when sock.can_send():

while let Some(front) = c.pending.front_mut() {
    if !sock.can_send() {
        break;
    }
    ...
}

Therefore, once the smoltcp TCP send buffer is full, the event loop can continue draining the user channel and append arbitrary request data to VecDeque<Bytes>. This defeats both USER_QUEUE and the fixed 64 KiB smoltcp socket buffer. The proxy’s capture budget does not help: forwarding is intentionally allowed after capture truncation, and each connection’s forwarding queue can grow without limit.

At up to 256 exchanges, a sender can drive enclave memory exhaustion and crash the enclave, losing in-flight records.

Suggested fix: Give Conn.pending a strict byte limit and stop polling/draining UserMsg::Data when it is reached, so backpressure propagates through TunnelStream to Hyper and ultimately to the sender. Ideally eliminate the extra unbounded queue and write into smoltcp only when it has send capacity. Include all userspace TCP queues in the enclave-wide request-memory accounting.

Medium — record finalization can race an early upstream response and produce an incomplete request record

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

Attacker action: An upstream responds before it has consumed the entire request body—for example, immediately returning an error or success while the sender is still uploading. The sender continues sending, or the request body is still being polled by Hyper.

Why the code allows it: Ownership of RecordGuard, which triggers finalization, is transferred only to the response body:

let tee_resp = TeeBody::new(rbody, pending, state.clone(), Side::Response, Some(guard));

When the response body reaches EOF, TeeBody::poll_frame drops that guard immediately:

this.guard.take();

The request tee has no guard and may still be active. Finalization takes Pending out of the mutex:

let Some(mut p) = self.pending.lock().unwrap().take() else {
    return;
};

Subsequent request-body frames are then silently not captured because pending is None. The record can therefore be sealed before all request data that traversed or continued traversing the proxy has been recorded. It will likely have body_complete: false, but this violates the README’s stronger “sealed log of everything that passes through it” / “every exchange is recorded” language.

Suggested fix: Finalize only after both sides have reached a terminal state: request body completed/aborted and response body completed/aborted. Use shared completion state with reference-counted ownership for both tees, rather than making response EOF alone own finalization. If an early response causes request forwarding to be cancelled, explicitly record the cancellation point and ensure no later request bytes can be accepted and forwarded without being represented in the record.

Medium — tlproxy verify succeeds by default without checking a known measurement or recipient

Files / functions:

Attacker action: An attacker operates any genuine Nitro enclave with an attested but malicious image, or an image that does not implement the claimed sealing policy. They direct a user to run:

tlproxy verify --proxy https://attacker.example

without --pcrs and without --expect-recipient.

Why the code allows it: Both checks are optional. With no --pcrs, the CLI explicitly only warns:

eprintln!("warning: no --pcrs given; attestation is genuine but the enclave code was not checked against a known build");

With no recipient expectation, check_recipient succeeds unconditionally. The command then exits successfully after proving only that the endpoint possesses a certificate key bound to some Nitro-attested enclave. It does not prove that the enclave runs this repository, uses a seven-day lock, uses tlock at all, or encrypts to the intended operator.

This does not permit a non-enclave endpoint to pass normal verify: it still needs a valid AWS Nitro attestation document bound to the TLS certificate and fresh nonce. But it does permit an arbitrary enclave endpoint to pass with a successful exit status, undermining the README’s wording that a sender can verify the claimed behavior by running tlproxy verify.

--allow-dev additionally makes an entirely unattested endpoint pass, but that is explicit opt-in and prints a warning.

Suggested fix: Make --pcrs and --expect-recipient mandatory for production verify and request; require an explicit --insecure-no-measurement override if retaining diagnostic use cases. In production mode, also validate the expected policy values directly in the CLI—at least lock_seconds >= MIN_LOCK_SECONDS, expected drand chain hash, and expected recipient—rather than merely printing them.

Medium — Mullvad device eviction can delete the operator’s devices, contrary to the documented design

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

Attacker action: The account owner or another user of the same Mullvad account creates a recently created device. An enclave restart or registration occurs while the device count exceeds the enclave’s computed allocation.

Why the code allows it: The code has no way to identify devices created by prior enclave boots. It sorts all account devices by creation time and deletes the newest entries:

list.sort();
while list.len() > allowed_before_create {
    let Some((_, id)) = list.pop() else { break };
    ...
    DELETE /accounts/v1/devices/{id}
}

The comment and README assert that “the owner’s own long-lived devices are the oldest and must survive,” but that is an assumption, not an identity check. The operator’s newest device is indistinguishable from an earlier enclave device and can be deleted.

This is primarily an availability and operational correctness issue, but the README’s claim that the enclave “never” deletes owner devices is false.

Suggested fix: Persist identifiers of enclave-created Mullvad devices outside the image in a host-provided state file, or use a unique device label/metadata and delete only devices positively identified as enclave-created. If Mullvad’s API cannot support reliable identification, do not claim that owner devices are protected; refuse registration rather than deleting unknown devices.

Low — reproducible-build claim is not self-contained because nitro-cli is not pinned

Files / functions:

Attacker action: No active network attacker is required. A sender attempts to independently reproduce PCRs from the repository commit using a different nitro-cli version.

Why the code allows it: The README acknowledges that the nitro-cli version affects PCR0/PCR1, but the build script obtains whatever version is installed locally:

nitro-cli build-enclave ...

It records the version only after building:

NITRO_CLI_VERSION=$(nitro-cli --version ...)

There is no pinned package, digest, source revision, or supported version list in the repository. Consequently, a verifier cannot reproduce the measurement from the commit alone, despite the README presenting reproduction as a straightforward independent check. The Debian snapshot value is also 20260801T000000Z, which is not necessarily available at the time of review and can make the prescribed build fail.

Suggested fix: Pin and distribute or build a specific nitro-cli version in a reproducible environment, include its digest/source revision in the repository, and test reproduction from a clean host. If that cannot be done, narrow the README claim: PCR comparison against operator-published values remains useful, but it is not independently reproducible solely from this repository and commit.

Explicit negative results

Overall verdict

I would not yet trust this deployment to enforce all stated properties. The core confidentiality design is substantially better than a normal proxy and I found no obvious pre-unlock plaintext exfiltration path, but the finish-time fallback contains a material logical error relative to the promised seven-day-after-completion lock, and the availability controls do not actually bound several important memory/work queues. Before trusting it, I would require a corrected, conservatively safe completion-time design; bounded connection, HTTP, sealing, and WireGuard buffering; a fix for record-finalization ordering; production verification that requires known PCRs and policy expectations; and a genuinely reproducible, pinned enclave-image build process.