-
Notifications
You must be signed in to change notification settings - Fork 61
fix: disallow leading zeros in RLP decoding #335
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
Changes from all commits
d19a360
a3c262c
5fe0d22
4dcf375
8d3b13b
901d01d
44cc9b1
0b6d46e
fca1d73
867476a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ const MAX_BITS: usize = 55 * 8; | |
|
|
||
| /// Allows a [`Uint`] to be serialized as RLP. | ||
| /// | ||
| /// See <https://eth.wiki/en/fundamentals/rlp> | ||
| /// See <https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp/> | ||
| impl<const BITS: usize, const LIMBS: usize> Encodable for Uint<BITS, LIMBS> { | ||
| #[inline] | ||
| fn length(&self) -> usize { | ||
|
|
@@ -72,11 +72,24 @@ impl<const BITS: usize, const LIMBS: usize> Encodable for Uint<BITS, LIMBS> { | |
|
|
||
| /// Allows a [`Uint`] to be deserialized from RLP. | ||
| /// | ||
| /// See <https://eth.wiki/en/fundamentals/rlp> | ||
| /// See <https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp/> | ||
| impl<const BITS: usize, const LIMBS: usize> Decodable for Uint<BITS, LIMBS> { | ||
| #[inline] | ||
| fn decode(buf: &mut &[u8]) -> Result<Self, Error> { | ||
| let bytes = Header::decode_bytes(buf, false)?; | ||
|
|
||
| // The RLP spec states that deserialized positive integers with leading zeroes | ||
| // get treated as invalid. | ||
| // | ||
| // See: | ||
| // https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp/ | ||
| // | ||
| // To check this, we only need to check if the first byte is zero to make sure | ||
| // there are no leading zeros | ||
| if !bytes.is_empty() && bytes[0] == 0 { | ||
| return Err(Error::LeadingZero); | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RLP spec explicitly says
meaning that the old accepting behaviour was not according to spec and a bug. Fixing bugs is not a breaking change. There's 'robustness principle' argument that could be made to make this error optionally a warning (i.e. introduce a non-strict mode). But that seems more effort than it's worth. Unless someone volunteers the strict mode should be default. |
||
|
|
||
| Self::try_from_be_slice(bytes).ok_or(Error::Overflow) | ||
| } | ||
| } | ||
|
|
@@ -148,4 +161,30 @@ mod test { | |
| }); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_invalid_uints() { | ||
| // these are non-canonical because they have leading zeros | ||
| assert_eq!( | ||
| U256::decode(&mut &hex!("820000")[..]), | ||
| Err(Error::LeadingZero) | ||
| ); | ||
| // 00 is not a valid uint | ||
| // See https://github.com/ethereum/go-ethereum/blob/cd2953567268777507b1ec29269315324fb5aa9c/rlp/decode_test.go#L118 | ||
| assert_eq!(U256::decode(&mut &hex!("00")[..]), Err(Error::LeadingZero)); | ||
| // these are non-canonical because they can fit in a single byte, i.e. | ||
| // 0x7f, 0x33 | ||
| assert_eq!( | ||
| U256::decode(&mut &hex!("8100")[..]), | ||
| Err(Error::NonCanonicalSingleByte) | ||
| ); | ||
| assert_eq!( | ||
| U256::decode(&mut &hex!("817f")[..]), | ||
| Err(Error::NonCanonicalSingleByte) | ||
| ); | ||
| assert_eq!( | ||
| U256::decode(&mut &hex!("8133")[..]), | ||
| Err(Error::NonCanonicalSingleByte) | ||
| ); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.