From d19a360c1c21988c58e275576296f9ead7143ad8 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 19 Oct 2023 14:23:55 -0400 Subject: [PATCH 1/9] add tests for uint leading zeros --- src/support/alloy_rlp.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/support/alloy_rlp.rs b/src/support/alloy_rlp.rs index 27b10476..8e9146fd 100644 --- a/src/support/alloy_rlp.rs +++ b/src/support/alloy_rlp.rs @@ -148,4 +148,24 @@ 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) + ); + assert_eq!( + U256::decode(&mut &hex!("8100")[..]), + 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, 0x assert_eq!(U256::decode(&mut &hex!("817f")[..]), + // Err(Error::LeadingZero)); assert_eq!(U256::decode(&mut + // &hex!("8133")[..]), Err(Error::LeadingZero)); + } } From a3c262c47009737d0de5ef92e6a2f93429997ad2 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 19 Oct 2023 14:24:45 -0400 Subject: [PATCH 2/9] todo for the checks --- src/support/alloy_rlp.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/support/alloy_rlp.rs b/src/support/alloy_rlp.rs index 8e9146fd..fff6db9c 100644 --- a/src/support/alloy_rlp.rs +++ b/src/support/alloy_rlp.rs @@ -77,6 +77,7 @@ impl Decodable for Uint { #[inline] fn decode(buf: &mut &[u8]) -> Result { let bytes = Header::decode_bytes(buf, false)?; + // TODO: leading zero and canonical representation checks Self::try_from_be_slice(bytes).ok_or(Error::Overflow) } } From 5fe0d22a437603e6da50f8dc663557dde1652fa8 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 19 Oct 2023 15:40:36 -0400 Subject: [PATCH 3/9] implement leading zero check --- src/support/alloy_rlp.rs | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/support/alloy_rlp.rs b/src/support/alloy_rlp.rs index fff6db9c..8139b6e2 100644 --- a/src/support/alloy_rlp.rs +++ b/src/support/alloy_rlp.rs @@ -77,7 +77,13 @@ impl Decodable for Uint { #[inline] fn decode(buf: &mut &[u8]) -> Result { let bytes = Header::decode_bytes(buf, false)?; - // TODO: leading zero and canonical representation checks + + // 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); + } + Self::try_from_be_slice(bytes).ok_or(Error::Overflow) } } @@ -157,16 +163,22 @@ mod test { U256::decode(&mut &hex!("820000")[..]), Err(Error::LeadingZero) ); - assert_eq!( - U256::decode(&mut &hex!("8100")[..]), - 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, 0x assert_eq!(U256::decode(&mut &hex!("817f")[..]), - // Err(Error::LeadingZero)); assert_eq!(U256::decode(&mut - // &hex!("8133")[..]), Err(Error::LeadingZero)); + // 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) + ); } } From 8d3b13b8fcd52c1f7bcf365456771bda8fd4db73 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Sun, 29 Oct 2023 13:19:01 -0400 Subject: [PATCH 4/9] update changelog and clarify comments with spec link --- CHANGELOG.md | 3 +++ src/support/alloy_rlp.rs | 12 +++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a93d220f..6e917019 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `leading_ones` failed for non-aligned sizes. +- Restricted RLP decoding to match the RLP spec and disallow leading zeros ([#335]) + +[#335]: https://github.com/recmo/uint/pulls/335 ## [1.10.1] - 2023-07-30 diff --git a/src/support/alloy_rlp.rs b/src/support/alloy_rlp.rs index 8139b6e2..b0eaae9b 100644 --- a/src/support/alloy_rlp.rs +++ b/src/support/alloy_rlp.rs @@ -13,7 +13,7 @@ const MAX_BITS: usize = 55 * 8; /// Allows a [`Uint`] to be serialized as RLP. /// -/// See +/// See impl Encodable for Uint { #[inline] fn length(&self) -> usize { @@ -72,13 +72,19 @@ impl Encodable for Uint { /// Allows a [`Uint`] to be deserialized from RLP. /// -/// See +/// See impl Decodable for Uint { #[inline] fn decode(buf: &mut &[u8]) -> Result { let bytes = Header::decode_bytes(buf, false)?; - // We only need to check if the first byte is zero to make sure there are no + // 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); From 901d01dfc840d5940cb05117af2ef89c662ec1a6 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Sun, 29 Oct 2023 13:21:43 -0400 Subject: [PATCH 5/9] cargo fmt --- src/support/alloy_rlp.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/support/alloy_rlp.rs b/src/support/alloy_rlp.rs index b0eaae9b..50ebb976 100644 --- a/src/support/alloy_rlp.rs +++ b/src/support/alloy_rlp.rs @@ -78,14 +78,14 @@ impl Decodable for Uint { fn decode(buf: &mut &[u8]) -> Result { let bytes = Header::decode_bytes(buf, false)?; - // The RLP spec states that deserialized positive integers with leading zeroes get treated - // as invalid. + // 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 + // 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); } From 44cc9b1276211deb4dca811c5522112c4f2e143b Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 30 Oct 2023 12:57:49 -0400 Subject: [PATCH 6/9] copy alloy rlp implementation to fastrlp --- src/support/fastrlp.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/support/fastrlp.rs b/src/support/fastrlp.rs index c2ab8a3e..9951f11f 100644 --- a/src/support/fastrlp.rs +++ b/src/support/fastrlp.rs @@ -13,7 +13,7 @@ const MAX_BITS: usize = 55 * 8; /// Allows a [`Uint`] to be serialized as RLP. /// -/// See +/// See impl Encodable for Uint { #[inline] fn length(&self) -> usize { @@ -72,16 +72,24 @@ impl Encodable for Uint { /// Allows a [`Uint`] to be deserialized from RLP. /// -/// See +/// See impl Decodable for Uint { #[inline] fn decode(buf: &mut &[u8]) -> Result { - let header = Header::decode(buf)?; - if header.list { - return Err(DecodeError::UnexpectedList); + 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); } - let bytes = &buf[..header.payload_length]; - *buf = &buf[header.payload_length..]; + Self::try_from_be_slice(bytes).ok_or(DecodeError::Overflow) } } From 0b6d46e534d04a682328fa6fffb07315bf6875bd Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 30 Oct 2023 13:05:31 -0400 Subject: [PATCH 7/9] fix error --- src/support/fastrlp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/support/fastrlp.rs b/src/support/fastrlp.rs index 9951f11f..e46625a0 100644 --- a/src/support/fastrlp.rs +++ b/src/support/fastrlp.rs @@ -87,7 +87,7 @@ impl Decodable for Uint { // 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); + return Err(DecodeError::LeadingZero); } Self::try_from_be_slice(bytes).ok_or(DecodeError::Overflow) From fca1d73498fff89dcc38fb4b3bf64f3fe53d679b Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 30 Oct 2023 13:08:02 -0400 Subject: [PATCH 8/9] no decode_bytes for fastrlp header --- src/support/fastrlp.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/support/fastrlp.rs b/src/support/fastrlp.rs index e46625a0..c8551833 100644 --- a/src/support/fastrlp.rs +++ b/src/support/fastrlp.rs @@ -76,7 +76,14 @@ impl Encodable for Uint { impl Decodable for Uint { #[inline] fn decode(buf: &mut &[u8]) -> Result { - let bytes = Header::decode_bytes(buf, false)?; + // let bytes = Header::decode_bytes(buf, false)?; + let header = Header::decode(buf)?; + if header.list { + return Err(DecodeError::UnexpectedList); + } + + let bytes = &buf[..header.payload_length]; + *buf = &buf[header.payload_length..]; // The RLP spec states that deserialized positive integers with leading zeroes // get treated as invalid. From 867476a2a3cc5a03550612fbe79996bb9de46281 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 30 Oct 2023 20:11:19 -0400 Subject: [PATCH 9/9] move bugfix into unreleased fixed section --- CHANGELOG.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e917019..499d93bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Restricted RLP decoding to match the RLP spec and disallow leading zeros ([#335]) + +[#335]: https://github.com/recmo/uint/pulls/335 + ## [1.11.0] - 2023-10-27 ### Added @@ -32,9 +38,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `leading_ones` failed for non-aligned sizes. -- Restricted RLP decoding to match the RLP spec and disallow leading zeros ([#335]) - -[#335]: https://github.com/recmo/uint/pulls/335 ## [1.10.1] - 2023-07-30