fix(prover): bound enclave request phases - #1469
0xrusowsky wants to merge 9 commits into
Conversation
|
cyclops audit fast |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This PR adds configurable receive, proving, and response deadlines to the prover enclave and introduces cooperative SPF cancellation. The change improves phase accounting, but verified availability issues remain.
Body-only finding (path not in the PR diff):
🚨 [POTENTIAL-VULNERABILITY] Encoded request limits do not bound decoded witness memory
Severity: High
File: crates/prover/src/connection.rs:93
ProverConnection::receive streams under-limit frames directly into unrestricted serde_json::from_reader and only checks cumulative encoded bytes. A request can remain below the 512 MiB encoded default while forcing huge decoded allocations in unbounded BatchWitness collections such as tempo_ancestry_headers: Vec<Bytes>, exhausting the 512 MiB Nitro enclave before semantic validation or the receive timeout can recover.
Recommended Fix: Add conservative per-field and per-collection limits during deserialization, and derive DEFAULT_MAX_REQUEST_BYTES from enclave memory plus worst-case decoded expansion rather than setting it equal to the full enclave allocation.
Reviewer Callouts
- ⚡ Inspected vs. plain EVM execution parity: The PR moves successful SPF replay onto the revm inspected execution path. Reviewers found no divergence at the pinned Tempo/revm revisions, but future dependency bumps should be gated by a differential test proving
prove_zone_batchandprove_zone_batch_with_cancelwith a never-cancelled token produce byte-identical outputs. - ⚡ Cancelled SPF error classification:
process_requeststill maps generic SPF errors toErrorCode::VerificationFailed. The current timeout branch discards worker responses after cancellation, but ifError::Cancelledever reaches this path in a future refactor it should be classified as a transient proving timeout rather than a validation failure.
| } | ||
| }; | ||
| handle_connection(connection, maximum, &specs).await; | ||
| handle_connection(connection, maximum, Arc::clone(&specs), &timeouts).await; |
There was a problem hiding this comment.
🚨 [POTENTIAL-VULNERABILITY] Queued idle sessions bypass the receive deadline
Both listeners accept one connection and await handle_connection before accepting the next, but the receive timeout starts inside handle_connection. VSOCK connections already queued in the listener backlog therefore have no active deadline, and the proxy has no overall session timeout after connect. An attacker can keep idle sockets queued so each consumes a fresh receive-timeout slot before legitimate prover traffic is accepted.
Recommended Fix: Start an end-to-end session/admission deadline in the proxy and accept/receive enclave connections concurrently under a strict semaphore, with a bounded proving queue and per-client authentication or rate limits.
There was a problem hiding this comment.
i think this is not applicable because the enclave is reachable only through our controlled VSOCK infrastructure (admission and client access are enforced by the host proxy, so considered trusted).
Co-authored-by: alpharush <0xalpharush@protonmail.com>
b431a6a to
fe1030e
Compare
0a44fd3 to
e7c004c
Compare
# Conflicts: # bin/prover/enclave/README.md # bin/prover/utils/src/main.rs # crates/prover/src/connection.rs # crates/prover/src/lib.rs
|
cyclops audit fast |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This PR adds request/response transport deadlines to the prover enclave and moves prover CBOR encoding/decoding onto blocking workers. One verified availability issue remains: the receive timeout does not actually cancel an already-started blocking CBOR decoder, so attacker-controlled request decoding can continue consuming enclave memory after the timed receive path has returned.
Reviewer Callouts
- None.
| let payload = payload.map_err(|error| classify_io_error(error, self.maximum))?; | ||
| self.last_received_bytes = Some(payload.len()); | ||
| decode_exact(&payload).map(Some) | ||
| join_worker(tokio::task::spawn_blocking(move || decode_exact(&payload)).await)?.map(Some) |
There was a problem hiding this comment.
🚨 [POTENTIAL-VULNERABILITY] Request timeout does not cancel the spawned CBOR decoder
handle_connection wraps connection.receive() in a deadline, but this line runs schema-driven CBOR deserialization in spawn_blocking. If a client completes a large under-limit frame just before the deadline, the timeout only drops the receive future/JoinHandle; an already-running Tokio blocking task continues and keeps allocating from attacker-controlled witness collections after the handler has returned. The configured request timeout therefore does not bound decoding CPU or memory and can be used to exhaust the Nitro enclave.
Recommended Fix:
Do not run timeout-bounded request decoding as an uncancellable spawn_blocking task. Use a cooperatively cancellable decoder that checks deadlines/work limits between collection elements, or isolate decoding in killable/strictly bounded workers, and enforce conservative per-field element and decoded-byte limits before allocating large vectors.
3 participants
ref:
SIGP-333this PR adds receive and response timeouts, so stalled enclave requests cannot monopolize the prover service.