bellman: add VerificationError #254
Conversation
This adds a distinct VerificationError type to the crate and changes `verify_proof` to return `Result<(), VerificationError>` rather than `Result<bool, SynthesisError>`. This is significantly safer, because it avoids the need to mix pattern-matching logic with boolean logic (the cause of RUSTSEC-2019-0004).
Codecov Report
@@ Coverage Diff @@
## master #254 +/- ##
==========================================
- Coverage 33.94% 33.61% -0.33%
==========================================
Files 97 97
Lines 11462 11461 -1
==========================================
- Hits 3891 3853 -38
- Misses 7571 7608 +37
Continue to review full report at Codecov.
|
| return Err(()); | ||
| } | ||
| } | ||
| verify_proof(verifying_key, &proof, &public_input[..]).map_err(|_| ())?; |
There was a problem hiding this comment.
It'd be nice if the whole spend_proof function returned a meaningful error type rather than just throwing away all the information it could be providing to the caller. Just a note for future work.
|
@str4d Fixed the doctests in a fixup commit so it's nicely squashable. |
| if (public_inputs.len() + 1) != pvk.ic.len() { | ||
| return Err(SynthesisError::MalformedVerifyingKey); | ||
| return Err(VerificationError::InvalidVerifyingKey); |
There was a problem hiding this comment.
This will in practice cause InvalidVerifyingKey to be treated identically to an invalid proof by the calling code, when actually it indicates an error in how that code is using the API (e.g. using the wrong key for the given input). This seems like a (minor) regression, although I can't see how it could cause a security failure in tested code.
There was a problem hiding this comment.
I know it doesn't affect callers in zcashd who were already collapsing those two cases.
There was a problem hiding this comment.
I think this is only true in practice if the calling code discards error information and propagates boolean values. I think the zcash_proofs does this. Otherwise the error is propagated upward until it is handled (by matching on the error type) or reported (which report will include the information indicating a programming error). So I don't think there is any conflation here.
This adds a distinct VerificationError type to the crate and changes
verify_proofto returnResult<(), VerificationError>rather thanResult<bool, SynthesisError>. This is significantly safer, because it avoidsthe need to mix pattern-matching logic with boolean logic (the cause of
RUSTSEC-2019-0004).