Fix(Continuations): Make private input private#758
Conversation
MauroToscano
left a comment
There was a problem hiding this comment.
Check the following issues:
-
Plaintext private-input leak through serialized boundaries
-
Severity: High/Critical
-
EpochProof.boundary serializes CellBoundary.init.value, which contains genesis values for touched cells. For private-input reads, those values are the private
bytes. -
This defeats the PR’s headline privacy goal: the bundle no longer carries private_inputs, but read private bytes still appear in plaintext.
-
Fix: serialize only verifier-needed boundary data, likely touched addresses/page bases. The verifier uses boundary via b.address; values are redundant.
-
-
Unconstrained OFFSET on non-preprocessed private pages
- Severity: Critical
- Private GLOBAL_MEMORY pages make OFFSET a prover-controlled main-trace column, while OFFSET is used to derive bus addresses.
- This lets a malicious prover forge genesis tokens for same-high-32-bit addresses by choosing invalid offsets.
- Caveats: this appears shared with monolithic private PAGE handling, so it is pre-existing; it is structurally verified but not PoC’d.
- Fix: keep OFFSET preprocessed for private pages, or constrain it to the row index/range.
-
Loader reservation misses the honest max private-input spill page
- Severity: High
- The loader reserves [START, START + MAX_PRIVATE_INPUT_SIZE), but the wire image is 4 + len.
- At max legal input length, the 4-byte prefix spills into page index 256 (0x103000000), which can be classified private but is not reserved from ELF segments.
- Fix: reserve the full page-rounded wire span: ceil((MAX_PRIVATE_INPUT_SIZE + 4) / PAGE_SIZE) pages.
-
Verifier bound allows one extra private page
-
Severity: High
-
The verifier accepts ceil((MAX_PRIVATE_INPUT_SIZE + 4) / PAGE_SIZE) + 1, allowing page index 257 (0x103040000) even though honest max is 257 total pages,
indexed 0..256. -
This is complementary to #3, not redundant: #3 is the honest max spill page; #4 is the extra slack page.
-
Fix: remove the + 1; check both continuation and monolithic verifier bounds.
-
-
Overflowing PT_LOAD ranges are accepted
- Severity: Medium
- Elf::load uses saturating arithmetic and never rejects p_vaddr + p_memsz overflow.
- Downstream code uses unchecked base_addr + ..., causing malformed-input acceptance and debug/release divergence.
- Fix: use checked_add and return the existing ElfError::AddrTooLarge.
-
Missing regression tests
- Severity: Medium
- Missing serialized-bundle byte-absence test; this would fail today because of #1.
- Missing continuation test for multi-page private input.
- Missing in-bounds inflated-count tamper test. This guards bundle tampering via Fiat-Shamir divergence, not the self-consistent forgery path in #2.
- Fix: add all three, with comments that correctly describe what each guards.
-
Residual privacy claim overstates non-ZK STARK privacy
-
Severity: Low/Medium
-
Even after fixing #1, non-preprocessed private columns are still opened as deterministic STARK trace evaluations unless the proof system is zero-knowledge/
blinded. -
Fix: phrase the claim as “raw private input is not bundled/recomputed by the verifier,” unless actual blinding is added.
-
-
Stale/misleading docs and comments
- Severity: Low
- Several comments now encode false reasoning, including “verifier never sees” private genesis and boundary-value tampering being inconsequential.
- These claims mask #1 and #2 and should be corrected with the code changes.
- Fix: update continuation.rs comments and docs/continuations_design.md to match the actual security model.
…the continuation bundle by dropping EpochProof.boundary and instead carrying only the value-free, sorted touched page-base set the verifier needs, derived from a single source shared with the committed GLOBAL_MEMORY tables, canonicalized on the verify side, and bound into the global Fiat-Shamir statement.
|
Benchmark Results for unmodified programs 🚀
|
|
Issues checked:
|
|
/ai-review |
Codex Code ReviewFindings
I did not run builds or tests per the review constraints. |
|
Review: Fix(Continuations) — Make private input private Reviewed statically against the base tree (the diff, plus the monolithic PAGE path in Verified as sound
Minor / non-blocking
Docs ( |
AI ReviewPR #758 · 8 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-001: test_private_input_multipage.s loads length into unused register
Claim
Evidence Lines 13-27 of the .s file load t3 then never use it. The test (test_continuation_multipage_private_input in continuation.rs:1517-1554) constructs a valid input, so the program always behaves correctly regardless of the read value. The unused register is a minor readability wart but not a bug. Suggested fix Either drop the AI-002: ELF reservation criterion is broader than necessary
Claim The check Evidence continuation.rs:316-322 documents the intent: nothing legitimate loads there because the stack is at STACK_TOP=0xFFFFFFFFFFFFFFF0 (registers.rs:3) and is initialized outside load_program, while private input is at 0xFF000000. So functionally fine, but the rejection criterion rejects any segment reaching into Suggested fix This is a deliberate design choice (the comment explicitly says "we reserve the whole high area"). Add a brief code comment near the check (not just the test docstring) explaining that the broader reservation is intentional, so a future contributor doesn't tighten it and accidentally break a legitimate use. AI-003: Multipage test hard-codes the 4-byte length-prefix assumption
Claim
Evidence continuation.rs:1527 computes Suggested fix Export a AI-004: InitClaim.timestamp is dead data (tracked but never read)
Claim InitClaim.timestamp is populated in epoch_boundary and propagated via provenance, but is never read by any bus interaction or prover/verifier code. Evidence The L2G memory bus interactions (local_to_global.rs:367-396) hard-code ts=0 as Suggested fix Drop the AI-005: with_private_input doc comment is misleading on the verifier side
Claim The doc comment on Evidence continuation.rs:299-300 calls Suggested fix Either accept Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
…erde derives, fix stale docs - Extract private_input_page_count / is_private_input_page / private_input_page_bases / max_private_input_pages into tables::page as the single source of truth; the monolithic trace builder, monolithic verifier bound, continuation prover, and continuation verifier all previously re-derived the same wire-format math independently and could drift. - Add PRIVATE_INPUT_LENGTH_PREFIX_BYTES to executor::vm::memory next to the wire-format writer and use it everywhere instead of a bare 4. - Remove the now-unused serde derives from InitClaim/FiniClaim/CellBoundary: nothing serializes them since the bundle dropped EpochProof.boundary, and keeping them off makes re-introducing the value leak a compile error. - Make the verifier-side private-page config explicitly data-free (include_private_genesis flag) instead of a dead init_page_data lookup. - Reject a non-page-aligned touched_page_bases entry as a malformed bundle (new Error::MalformedContinuationBundle) instead of Ok(None), matching the count bound's Err semantics for structural validation of untrusted fields. - Fix bin/cli/README.md, which still claimed the continuation bundle ships the raw private input bytes.
Review fixes for #758: dedupe private-input page math, drop leaky serde derives, fix stale CLI README
Motivation
The continuation prover shipped the raw private input inside the proof bundle, and the verifier recomputed the starting memory from it, so private input was exposed to the verifier.
Description
GLOBAL_MEMORYpages are now built non-preprocessed: their genesis is a committed, bus-enforced column the verifier never recomputes (mirroring the monolithic PAGE table).private_inputsbytes for anum_private_input_pagescount, classified count-based fromPRIVATE_INPUT_START_INDEX(like the monolithic verifier), bound-checked and absorbed into the global proof's Fiat-Shamir statement.verify_continuationnow works from the bundle + ELF alone, it never sees the raw private input.Elf::loadnow rejects any loadable segment overlapping the reserved private-input region, so ELF data can never sit on a page whose genesis isn't recomputed from the ELF (closes a pre-existing forgeable-genesis gap shared with the monolithic path).