Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zcash_proofs: Theoretical possibility of overflow leading to panic #786

Closed
mpguerra opened this issue Mar 9, 2023 · 1 comment · Fixed by #805
Closed

zcash_proofs: Theoretical possibility of overflow leading to panic #786

mpguerra opened this issue Mar 9, 2023 · 1 comment · Fixed by #805

Comments

@mpguerra
Copy link

mpguerra commented Mar 9, 2023

As part of the zebra audit the following potential issue has been highlighted:

In src/hashreader.rs, a streamed hashing structure is defined, which hashes bytes (with the BLAKE2b hash function) as they flow, but also keeps track of how many bytes have been processed so far; the byte_count field maintains that information, and has type usize :

/// Abstraction over a reader which hashes the data being read.
pub struct HashReader<R: Read> {
reader: R,
hasher: State,
byte_count: usize,
}

On 32-bit architectures, usize has size 32 bits, and thus any inputs larger than about 4.29 gigabytes will overflow that counter. In particular, the addition on line 51 may then trigger a panic (if the code was compiled in debug mode), or silently truncate the count to its low 32 bits (if compilation used release mode). This issue cannot be triggered with the implementation in its current state, since hashing is performed only on files whose size has been explicitly verified to match the expected size for parameter files, with a maximum of about 0.73 gigabytes (for Sprout Groth16 parameters). Defining byte_count to have type u64 would make the implementation more robust with regard to future development and protocol versions.

@mpguerra mpguerra changed the title zcash_proofs: Theoretical possibility of overflow leading to panic zcash_proofs: Theoretical possibility of overflow leading to panic Mar 9, 2023
@str4d
Copy link
Contributor

str4d commented Apr 14, 2023

I looked into this again during the 0.11 release process, and the only effect in release mode of this would be that error messages report incorrect byte counts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants