-
Notifications
You must be signed in to change notification settings - Fork 808
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
Protect against OOB offset in variable list SSZ decoding #974
Conversation
This is a great catch by the fuzzer! I had written a test that I thought covered this case: lighthouse/eth2/utils/ssz/src/decode/impls.rs Lines 573 to 576 in 5a6e904
However, I was getting an lighthouse/eth2/utils/ssz/src/decode/impls.rs Lines 421 to 427 in 5a6e904
What we really needed to test this functionality was: assert_eq!(
<Vec<Vec<u16>>>::from_ssz_bytes(&[8, 0, 0, 0, 16, 0, 0, 0]),
Err(DecodeError::OutOfBoundsByte { i: 16 })
); I think there's a lesson to be learned about being specific with error types. If I had more specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The updates appear to faithfully implement all the checks from the hack.md doc, while preserving existing behaviour.
Issue Addressed
NA
Proposed Changes
As raised by @pventuzelo during fuzzing (thank you Patrick!!), SSZ decoding was failing to ensure that all offsets were in-bounds. This resulted in trying to allocation a billion-element
Vec
which caused a panic due to a failed memory allocation.Ironically, I documented this bug (SSZ Offset Exploits #4) last year but failed to enforce it here. 🤦♂️
This PR:
DecodeError
more granular to be more certain that tests trigger certain code paths.Vec::with_capacity
for objects that specify a maximum length.pretty-ssz
CLI command which allowed to me to parse the SSZ file from @pventuzelo. It should parse some SSZ then pretty-print it as YAML, but is useful for finding errors in SSZ decoding.lcli -s minimal pretty-ssz SignedBeaconBlock crash1_min.ssz