reviews/openai__gpt-5.5.md
On this page

Review by openai/gpt-5.5

commit: 67eb6e6f672501be1c9f67cde1b979efebdcf981
usage: {'prompt_tokens': 50170, 'completion_tokens': 8934, 'total_tokens': 59104, 'cost': 0.51887, '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.51887, 'upstream_inference_prompt_cost': 0.25085, 'upstream_inference_completions_cost': 0.26802}, 'completion_tokens_details': {'reasoning_tokens': 4911, 'image_tokens': 0, 'audio_tokens': 0}}
time: 111s

Critical findings

None found. I did not find a path that directly exposes request/response plaintext to the parent instance, relay provider, network attacker, or operator before the drand round, assuming the attested enclave binary is the one being run and tlock_age/age/rustls behave as intended.

High findings

1. Sealed-record queue is byte-unbounded and can OOM/crash the enclave, losing in-flight records

Severity: High
File/function: crates/enclave/src/records.rs, Sink::start, Sink::submit, worker
Attacker: A sender, with a slow or unavailable parent record sink; or the operator/parent deliberately refusing record delivery.
What the attacker does: Causes many large exchanges to complete while the parent record service is unavailable or not acknowledging records. Each completed exchange is sealed into a large Vec<u8> and queued.
Why the code allows it: The queue is bounded only by record count:

const QUEUE_CAPACITY: usize = 4096;
let (tx, rx) = mpsc::channel(QUEUE_CAPACITY);

But each queued item is (String, Vec<u8>), and the Vec<u8> can be very large: up to roughly request body capture + response body capture + JSON/base64 + age/tlock overhead. With max_body_bytes = 64 MiB, a single record can exceed 100 MiB after base64 serialization. The enclave is launched with only 1024 MiB:

nitro-cli run-enclave ... --memory 1024

The worker retries the first undeliverable record indefinitely:

while let Some((name, data)) = rx.recv().await {
    loop {
        match deliver(&transport, &name, &data).await {
            Ok(()) => break,
            Err(_) => { ... retry ... }
        }
    }
}

So subsequent records accumulate in memory. The README says resource use is bounded and records waiting for the parent are capped at 4096, but 4096 is not a meaningful memory bound here.

Impact: Enclave OOM/crash, loss of in-flight plaintext records before they are sealed/delivered. This does not reveal contents, but violates the availability/resource-exhaustion goals and the “never lose a record” claim.

Suggested fix: Bound the record queue by total bytes, not just count. Track queued ciphertext bytes with an atomic/semaphore and refuse/drop before enqueue when the byte budget is exhausted. Consider a much smaller byte budget, e.g. a measured config value such as max_queued_record_bytes. Also consider refusing new proxied requests while the record sink is backed up, if “never lose a record” is a real goal.

2. Lock-time fallback can shorten the effective lock after long pauses or finish-time NSM failure

Severity: High
File/function: crates/enclave/src/proxy.rs, RecordGuard::drop; design text in README “What ‘now’ means to the enclave”
Attacker: Operator/parent able to stall scheduling, induce long-running exchanges, or exploit transient NSM unavailability at completion.
What the attacker does: Allows a request/response exchange to complete substantially later than the enclave’s fallback finish estimate, while the final NSM timestamp call fails.
Why the code allows it: At finalization:

let elapsed_ms = u64::try_from(p.started_at.elapsed().as_millis()).unwrap_or(u64::MAX);
let monotonic_finished_at_ms = p.started_at_ms.saturating_add(elapsed_ms);

let finished_at_ms = match state.attester.trusted_time_ms() {
    Ok(t) => t.max(monotonic_finished_at_ms),
    Err(_) => {
        crate::log_record(&p.id, "trusted finish time unavailable; using start timestamp + monotonic elapsed");
        monotonic_finished_at_ms
    }
};
let round = p.initial_round.max(
    state.clock.lock_round_at_ms(finished_at_ms, state.cfg.lock_seconds),
);

The README says using a lower-bound finish time “can only be longer than seven days, never shorter.” That is backwards for a “seven days after completion” guarantee. If the fallback finished_at_ms is earlier than the real completion time, then finished_at_ms + lock_seconds is also earlier than “real completion + lock_seconds.”

Example: request starts at T. The enclave is paused/stalled for 24 hours during the exchange, or Instant does not advance through the relevant pause. Final trusted NSM time is unavailable. The record can be locked to approximately T + 7 days, even though the response finished at T + 1 day, making the effective lock about 6 days after completion.

The initial round computed at request start prevents locking earlier than start+7d, but the README/config claim is “after the request completes.”

Impact: The operator may be able to decrypt less than the configured delay after the response completed, if finish-time attestation fails and the fallback underestimates real completion time.

Suggested fix: Do not use a lower-bound timestamp to enforce “delay after completion.” If a fresh trusted finish timestamp is unavailable, keep retrying until one is available and seal using that timestamp, or lock to a deliberately conservative far-future round. If relying on monotonic time, document and test that the Nitro enclave monotonic clock continues across all relevant pause/scheduling conditions; otherwise it is not sufficient. Update the README: a lower bound makes the unlock earlier, not later, relative to real completion.

3. Unbounded pre-request TLS/HTTP connections can exhaust enclave resources outside max_in_flight

Severity: High
File/function: crates/enclave/src/main.rs, accept loop; crates/enclave/src/proxy.rs, forward_entry
Attacker: Network attacker or sender.
What the attacker does: Opens many TCP/TLS connections to the parent’s port 443, completes or stalls TLS handshakes, and then sends no HTTP request or sends headers very slowly.
Why the code allows it: The enclave spawns one task per accepted stream:

tokio::spawn(async move {
    let tls_stream = match tokio::time::timeout(
        std::time::Duration::from_secs(20),
        acceptor.accept(stream),
    ).await { ... };

    let io = TokioIo::new(tls_stream);
    let svc = service_fn(move |req| proxy::handle(state.clone(), req));
    http1::Builder::new()
        .keep_alive(true)
        .serve_connection(io, svc)
        .await
});

max_in_flight is enforced only inside forward_entry, after a full HTTP request has been parsed and classified as a proxied request:

if state.in_flight.fetch_add(1, Ordering::AcqRel) >= state.cfg.max_in_flight {
    ...
}

Idle TLS connections, slow HTTP header sends, keep-alive connections, and requests to non-forwarding endpoints are not counted. There is no global connection semaphore, no HTTP header read timeout, and keep-alive is enabled.

Impact: A network attacker can consume enclave memory/tasks/file descriptors and potentially crash the enclave, losing in-flight records. This violates the availability limits described in the README.

Suggested fix: Add a measured global connection semaphore covering TLS handshakes and HTTP connections, not just proxied exchanges. Add HTTP header/read timeouts and an idle keep-alive timeout. Consider disabling keep-alive or limiting requests per connection. Enforce limits before spawning long-lived per-connection work.

Medium findings

4. Independent reproducible verification is not possible from the provided tree as shown

Severity: Medium
File/function: build/Dockerfile, build/build-eif.sh; repository layout
Attacker: Operator claiming to run commit 67eb6e6f672501be1c9f67cde1b979efebdcf981 while building from different dependency/toolchain inputs.
What the attacker does: Publishes PCRs for an enclave image that users cannot independently reproduce from the claimed commit.
Why the code/source allows it: The README claims crates are pinned by Cargo.lock and the Rust toolchain is pinned. The Dockerfile also requires both:

COPY Cargo.toml Cargo.lock rust-toolchain.toml ./
RUN cargo build --release --locked ...

But in the supplied source tree, neither Cargo.lock nor rust-toolchain.toml is present. If they are truly absent at this commit, the Docker build fails as written; if users work around that, dependency and toolchain resolution are no longer independently pinned.

Impact: This weakens the main sender verification story. PCR comparison is only meaningful if the sender can reproduce the EIF from the reviewed source and compare PCR0/1/2. Without the lockfile/toolchain file, the operator’s published PCRs become harder to independently audit.

Suggested fix: Commit Cargo.lock and rust-toolchain.toml, and ensure ./build/build-eif.sh succeeds from a clean checkout of the claimed commit. Treat absence or mismatch as release-blocking. The verifier documentation should explicitly say users should not trust operator-published PCRs unless they can reproduce them.

5. tlproxy verify can print and check policy values that are not themselves signed unless PCRs are checked

Severity: Medium
File/function: crates/cli/src/attested.rs, connect; crates/cli/src/main.rs, check_recipient
Attacker: A genuine Nitro enclave with unknown PCRs, or any attested enclave serving misleading JSON policy fields.
What the attacker does: Runs some enclave that presents a valid Nitro attestation for its TLS key, but not the expected measured code/config. It returns arbitrary age_recipient, lock_seconds, drand_chain_hash, doh_resolvers, ech, and relay status in the JSON response.
Why the code allows it: The signed Nitro document binds only the TLS certificate SPKI via user_data. The policy values are ordinary JSON returned over the attested TLS connection:

let attestation: AttestationResponse = serde_json::from_slice(&body)?;

They are only meaningful if PCR0/1/2 are checked against a reproducible build containing config/enclave.toml. The CLI warns if no PCRs are supplied:

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

But it still prints policy as if it is what the enclave enforces and check_recipient checks the unsigned JSON value:

check_recipient(&conn.attestation.age_recipient, expect_recipient.as_deref())

Impact: A user running tlproxy verify without --pcrs can be misled into thinking recipient/lock policy was verified. This does not let a non-enclave endpoint pass as Nitro, but it weakens the “sender can verify all of this” UX and can produce false assurance.

Suggested fix: Make --pcrs mandatory by default for production verification. Require an explicit --no-pcrs-unsafe flag to print unverified policy. In summaries, label all policy fields as “unverified unless PCRs matched.” Optionally include a hash of the embedded config in Nitro user_data alongside the SPKI, though PCR checking is still needed to prove behavior.

6. Hostname validation allows underscores, which can produce DNS/TLS behavior mismatches and avoid intended validation assumptions

Severity: Medium
File/function: crates/enclave/src/transport.rs, valid_host; crates/enclave/src/proxy.rs, parse_target
Attacker: Sender choosing an upstream hostname.
What the attacker does: Sends a request for a host containing _, e.g. bad_name.example.
Why the code allows it: Host validation accepts underscores:

host.chars()
    .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '.' || c == '_')

DNS names used as TLS ServerNames generally cannot contain underscores in host labels. Later, rustls may reject the name, DNS may resolve it differently than expected, or behavior may vary across libraries. This is mostly correctness/availability, not plaintext exposure.

Impact: Sender-chosen names can trigger inconsistent DNS/TLS errors. Those errors are returned to the sender and recorded sealed, but they can also consume resources and complicate auditability. It weakens the clean “valid DNS host name” assumption.

Suggested fix: Use a strict DNS hostname validator: labels 1–63 chars, ASCII alnum plus hyphen only, no leading/trailing hyphen per label, total length ≤253, no underscores for TLS hostnames. Consider using an existing IDNA/DNS-name parser and converting to punycode explicitly if international names are desired.

7. Request and response headers are captured without an explicit measured size cap

Severity: Medium
File/function: crates/enclave/src/proxy.rs, headers_to_vec, Pending::new, response header capture in forward
Attacker: Sender controlling request headers; or sender-chosen upstream controlling response headers.
What the attacker does: Sends many/large headers, or points the proxy at an upstream that returns many/large headers.
Why the code allows it: Body capture is capped by max_body_bytes and max_capture_bytes_total, but headers are copied wholesale into the pending record:

request_headers: headers_to_vec(req.headers()),
...
p.response_headers = headers_to_vec(&rparts.headers);

headers_to_vec converts every header value to an owned String:

headers.iter()
    .map(|(k, v)| (k.as_str().to_string(),
                   String::from_utf8_lossy(v.as_bytes()).into_owned()))
    .collect()

Hyper has some internal parsing limits, but they are not part of the measured config or the README’s availability model. Across max_in_flight = 256, large headers can consume significant enclave memory outside the explicit capture budget.

Impact: Resource-exhaustion risk and possible crash/loss of in-flight records. Not a confidentiality break.

Suggested fix: Add measured limits for total request header bytes and total response header bytes captured per record. Reject requests exceeding the request header cap before forwarding. For response headers exceeding the cap, either fail the exchange or record them as truncated. Include the cap in attestation output.

Low findings

8. Full upstream error text is returned to the sender and stored in the sealed record

Severity: Low
File/function: crates/enclave/src/proxy.rs, forward_entry error path
Attacker: Sender choosing malformed or sensitive-looking upstream names.
What the attacker does: Causes DNS/TLS/connect errors and receives detailed error text.
Why the code allows it: On forwarding failure:

if let Some(p) = pending.lock().unwrap().as_mut() {
    p.error = Some(format!("{e:#}"));
}
...
let mut r = text(StatusCode::BAD_GATEWAY, format!("upstream error: {e:#}\n"));

The README’s diagnostic claim is about logs, and the logs do classify errors into fixed labels. The response body goes only to the TLS-authenticated sender, so this is not a parent/operator plaintext leak. Still, the returned text can contain resolver/connect/TLS details and the requested host.

Impact: Not a violation if the sender is considered authorized to know its own request and failure reason. But if clients are embedded in environments where response bodies are logged elsewhere, this can propagate request-derived values.

Suggested fix: Return fixed-category errors to the sender as well, e.g. upstream error: dns, tls, timeout, etc., and keep detailed text only inside the sealed record if desired.

9. README “never lose a record” wording is inconsistent with implemented drop behavior

Severity: Low / Info
File/function: crates/enclave/src/records.rs, Sink::submit; README “Deliberate choice: fail closed at the start, never lose a record” and “Availability limits”
Attacker: Parent keeps record sink unavailable; senders continue making requests.
What the attacker does: Fills the bounded record channel.
Why the code allows it: When the queue is full:

if self.tx.try_send((name, sealed)).is_err() {
    crate::log("record queue full; dropping encrypted record");
}

The README later says records are dropped after the queue fills, but an earlier section says “never lose a record.” The implemented behavior is “sender traffic is never blocked on logging; records may be dropped.”

Impact: Documentation/security-goal mismatch. Not a confidentiality issue, but important for operators and senders relying on record completeness.

Suggested fix: Decide which property is intended. If “never lose a record,” block or reject new requests when durable record delivery is unavailable. If availability is preferred, remove the “never lose a record” claim and make record loss explicit in the security goals.

Explicit non-findings at higher severity

I did not find a concrete bug that lets the parent instance, Mullvad, drand relay, ACME server, or a network attacker read proxied request/response contents before the tlock round is published. TLS termination for sender traffic is inside the enclave; upstream TLS is initiated inside the enclave with WebPKI validation; records are serialized and then encrypted with age and tlock before leaving the enclave.

I also did not find a concrete way for a non-enclave endpoint to pass tlproxy verify when the user supplies correct expected PCRs and does not allow dev mode. The verifier captures the TLS certificate from the actual handshake, fetches attestation over that same connection, checks nonce, checks the attestation document, rejects all-zero PCRs, and compares PCR0/1/2 when provided.

Overall verdict

I would not yet trust this deployment to fully enforce the stated properties as written. The core confidentiality design is sound-looking, and I found no direct pre-unlock plaintext exposure path, but the lock-time fallback logic appears to contradict the README and can shorten the effective post-completion delay if finish-time NSM attestation is unavailable. The availability/resource bounds are also materially weaker than claimed: queued sealed records are count-bounded but byte-unbounded, and pre-request connections are not covered by max_in_flight. Before trusting production use, I would require a fixed finish-time locking rule, byte-based record queue limits or request backpressure, global connection/header limits, and a reproducible build from a clean checkout with committed lockfile/toolchain inputs.