Conversation
There was a problem hiding this comment.
Pull request overview
Implements OpenVM proof verification in the openvm-glue FFI layer by deserializing a proof “receipt”, checking it against the target ELF, and invoking the OpenVM SDK verifier.
Changes:
- Implement
openvm_verifyto deserialize a proof package, validate the ELF hash, and verify the proof viaSdk::verify_app_proof. - Add
openvm-circuitdependency to access the concrete proof type (ContinuationVmProof). - Update lockfile to include the new dependency.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rust/openvm-glue/src/lib.rs | Implements the verifier logic and introduces proof/VK deserialization + ELF hash check. |
| rust/openvm-glue/Cargo.toml | Adds openvm-circuit dependency required for proof type deserialization. |
| rust/Cargo.lock | Lockfile update reflecting the new dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deserialize the app verifying key | ||
| let app_vk: AppVerifyingKey = match bincode::deserialize(&proof_package.vk_bytes) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| eprintln!("openvm_verify: failed to deserialize verifying key: {}", e); | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| // Verify the proof using the SDK | ||
| let sdk = Sdk::new(); | ||
| match sdk.verify_app_proof(&app_vk, &proof) { | ||
| Ok(_) => true, |
There was a problem hiding this comment.
The verifier currently trusts vk_bytes embedded in the receipt and verifies the proof against that key. This lets a malicious prover choose verification parameters/key material (e.g., different AppConfig / security level) and still pass verification, because the verifier never derives or pins the expected verifying key/config independently. Consider deriving the expected AppVerifyingKey from a verifier-side config (or at least validating that the receipt’s config matches a verifier-side policy and that the provided VK matches the derived VK) instead of accepting the VK from the receipt.
| // Deserialize the continuation VM proof | ||
| let proof: ContinuationVmProof<SC> = match bincode::deserialize(&proof_package.proof_bytes) { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| eprintln!("openvm_verify: failed to deserialize proof: {}", e); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
bincode::deserialize(&proof_package.proof_bytes) will also perform potentially unbounded allocations based on attacker-controlled contents of proof_bytes. Even if you cap the outer receipt size, a crafted payload can still request large nested allocations during proof decoding. Consider using bincode::DefaultOptions::new().with_limit(MAX_BYTES) (or another bounded decoding strategy) for the inner proof/VK deserializations as well.
| // Deserialize the proof package from receipt bytes | ||
| let proof_package: OpenVMProofPackage = match bincode::deserialize(receipt_slice) { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| eprintln!("openvm_verify: failed to deserialize proof package: {}", e); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
openvm_verify deserializes receipt_slice via bincode::deserialize with no size limits. Since receipt is untrusted FFI input, this can trigger unbounded allocations / OOM (DoS) via large Vec lengths inside the payload. Consider using bincode::DefaultOptions::new().with_limit(MAX_BYTES) (and/or validate receipt_len up front) before deserializing OpenVMProofPackage.
gballet
force-pushed
the
prove-and-verify-openvm
branch
from
e6feebf to
38069bd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deserialize the continuation VM proof | ||
| let proof: ContinuationVmProof<SC> = match bincode::deserialize(&proof_package.proof_bytes) { | ||
| Ok(p) => p, | ||
| Err(e) => { | ||
| eprintln!("openvm_verify: failed to deserialize proof: {}", e); | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| // Deserialize the app verifying key | ||
| let app_vk: AppVerifyingKey = match bincode::deserialize(&proof_package.vk_bytes) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| eprintln!("openvm_verify: failed to deserialize verifying key: {}", e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The proof and verifying key are also deserialized from attacker-controlled bytes with no explicit size/alloc limit (proof_bytes / vk_bytes). To avoid DoS via crafted receipts, cap total receipt size and/or use bounded bincode options for these deserializations too.
| return false; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
OpenVMProofPackage includes app_log_blowup, but openvm_verify never uses or validates it. Either remove it from the serialized package (if unnecessary) or use it to sanity-check that the receipt was created with the expected security/config parameters (and update the struct field comment accordingly).
| // Sanity-check that the proof was generated with the expected app_log_blowup | |
| let expected_app_log_blowup = SdkVmConfig::<SC>::default().app_log_blowup; | |
| if proof_package.app_log_blowup != expected_app_log_blowup { | |
| eprintln!( | |
| "openvm_verify: app_log_blowup mismatch — expected {}, got {}", | |
| expected_app_log_blowup, proof_package.app_log_blowup | |
| ); | |
| return false; | |
| } |
| extern "C" fn openvm_verify( | ||
| _binary_path: *const u8, | ||
| _binary_path_len: usize, | ||
| _receipt: *const u8, | ||
| _receipt_len: usize, | ||
| binary_path: *const u8, | ||
| binary_path_len: usize, | ||
| receipt: *const u8, | ||
| receipt_len: usize, | ||
| ) -> bool { | ||
| // TODO: Implement verification | ||
| eprintln!("OpenVM verification not yet implemented"); | ||
| true | ||
| let binary_path_slice = unsafe { | ||
| if !binary_path.is_null() { | ||
| std::slice::from_raw_parts(binary_path, binary_path_len) | ||
| } else { | ||
| eprintln!("openvm_verify: binary_path is null"); | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| let receipt_slice = unsafe { | ||
| if !receipt.is_null() { | ||
| std::slice::from_raw_parts(receipt, receipt_len) | ||
| } else { | ||
| eprintln!("openvm_verify: receipt is null"); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
openvm_verify adds a fair amount of parsing/validation logic (null checks, UTF-8 handling, bincode decode failures, ELF hash mismatch, proof verification). Please add unit tests that cover at least the negative/error paths so FFI callers get stable behavior when inputs are malformed (these can be done without generating a real proof).
No description provided.