From 7ec4a5f046762278d9181bc07779e9abb253144b Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Sun, 16 Apr 2023 14:29:05 -0500 Subject: [PATCH 01/15] add potential fix for OOR access on Vector --- ssz-rs-derive/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 0eb3f20a..0396028a 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -199,6 +199,11 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { } else { let encoded_length = <#field_type>::size_hint(); let end = start + encoded_length; + if end > encoding.len() { + return Err(ssz_rs::DeserializeError::ExpectedFurtherInput { + provided: encoding.len(), + expected: end, + })}; let result = <#field_type>::deserialize(&encoding[start..end])?; container.#field_name = result; encoded_length From b64549d13937f40b7b0fce48cd6932d170361f9c Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Mon, 17 Apr 2023 10:34:32 -0500 Subject: [PATCH 02/15] more robust fix --- ssz-rs-derive/src/lib.rs | 11 +++++------ ssz-rs/src/de.rs | 4 ++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 0396028a..9a836f68 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -199,12 +199,11 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { } else { let encoded_length = <#field_type>::size_hint(); let end = start + encoded_length; - if end > encoding.len() { - return Err(ssz_rs::DeserializeError::ExpectedFurtherInput { - provided: encoding.len(), - expected: end, - })}; - let result = <#field_type>::deserialize(&encoding[start..end])?; + let enc = &encoding.get(start..end).ok_or(DeserializeError::InvalidRange{ + range: start..end, + buffer_length:encoding.len(), + })?; + let result = ::deserialize(&enc)?; container.#field_name = result; encoded_length }; diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index 55cf9e18..d4b473a5 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -1,3 +1,4 @@ +use std::ops::Range; use crate::{ error::{InstanceError, TypeError}, lib::*, @@ -23,6 +24,8 @@ pub enum DeserializeError { InvalidInstance(InstanceError), /// An invalid type was encountered. InvalidType(TypeError), + /// Failed to get the range from encoding buffer + InvalidRange{range: Range, buffer_length: usize}, } impl From for DeserializeError { @@ -48,6 +51,7 @@ impl Display for DeserializeError { ), DeserializeError::InvalidInstance(err) => write!(f, "invalid instance: {err}"), DeserializeError::InvalidType(err) => write!(f, "invalid type: {err}"), + DeserializeError::InvalidRange{range, buffer_length} => write!(f, "failure on getting range: {range:#?}, for length: {buffer_length}"), } } } From 25338e7ec584c7f43e8729f1a17ecaa5c1dfb794 Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Mon, 17 Apr 2023 11:07:39 -0500 Subject: [PATCH 03/15] revert bad line from CoPilot --- ssz-rs-derive/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 9a836f68..6f060c01 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -203,7 +203,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { range: start..end, buffer_length:encoding.len(), })?; - let result = ::deserialize(&enc)?; + let result = <#field_type>::deserialize(&enc)?; container.#field_name = result; encoded_length }; From c4126a5070f4b76a223a3c3a8f53b2771c2b7286 Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Wed, 19 Apr 2023 17:02:34 -0500 Subject: [PATCH 04/15] remove && and unnecessary var. --- ssz-rs-derive/src/lib.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 6f060c01..e9c99098 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -199,11 +199,13 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { } else { let encoded_length = <#field_type>::size_hint(); let end = start + encoded_length; - let enc = &encoding.get(start..end).ok_or(DeserializeError::InvalidRange{ - range: start..end, - buffer_length:encoding.len(), - })?; - let result = <#field_type>::deserialize(&enc)?; + let result = <#field_type>::deserialize( + encoding.get(start..end) + .ok_or(DeserializeError::InvalidRange { + range: start..end, + buffer_length:encoding.len(), + })? + )?; container.#field_name = result; encoded_length }; From 428fade11bde160cc4df740c6591a0ab8e2ba1cd Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Wed, 19 Apr 2023 17:04:18 -0500 Subject: [PATCH 05/15] add similar fix for variable size fields --- ssz-rs-derive/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index e9c99098..bfca6ec5 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -192,7 +192,13 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { Some(field_name) => quote_spanned! { f.span() => let bytes_read = if <#field_type>::is_variable_size() { let end = start + #BYTES_PER_LENGTH_OFFSET; - let next_offset = u32::deserialize(&encoding[start..end])?; + let next_offset = u32::deserialize( + encoding.get(start..end) + .ok_or(DeserializeError::InvalidRange { + range: start..end, + buffer_length:encoding.len(), + })? + )?; offsets.push((#i, next_offset as usize)); #BYTES_PER_LENGTH_OFFSET From cc79840b5363b7ce906a82c6949eb1cee2429eb6 Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Thu, 27 Apr 2023 14:15:39 -0500 Subject: [PATCH 06/15] replace invalid-range with expected-further-input --- ssz-rs-derive/src/lib.rs | 12 ++++++------ ssz-rs/src/de.rs | 4 ---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index bfca6ec5..02b51bf6 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -194,9 +194,9 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let end = start + #BYTES_PER_LENGTH_OFFSET; let next_offset = u32::deserialize( encoding.get(start..end) - .ok_or(DeserializeError::InvalidRange { - range: start..end, - buffer_length:encoding.len(), + .ok_or(DeserializeError::ExpectedFurtherInput { + expected: end, + provided:encoding.len(), })? )?; offsets.push((#i, next_offset as usize)); @@ -207,9 +207,9 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let end = start + encoded_length; let result = <#field_type>::deserialize( encoding.get(start..end) - .ok_or(DeserializeError::InvalidRange { - range: start..end, - buffer_length:encoding.len(), + .ok_or(DeserializeError::ExpectedFurtherInput { + expected: end, + provided:encoding.len(), })? )?; container.#field_name = result; diff --git a/ssz-rs/src/de.rs b/ssz-rs/src/de.rs index d4b473a5..55cf9e18 100644 --- a/ssz-rs/src/de.rs +++ b/ssz-rs/src/de.rs @@ -1,4 +1,3 @@ -use std::ops::Range; use crate::{ error::{InstanceError, TypeError}, lib::*, @@ -24,8 +23,6 @@ pub enum DeserializeError { InvalidInstance(InstanceError), /// An invalid type was encountered. InvalidType(TypeError), - /// Failed to get the range from encoding buffer - InvalidRange{range: Range, buffer_length: usize}, } impl From for DeserializeError { @@ -51,7 +48,6 @@ impl Display for DeserializeError { ), DeserializeError::InvalidInstance(err) => write!(f, "invalid instance: {err}"), DeserializeError::InvalidType(err) => write!(f, "invalid type: {err}"), - DeserializeError::InvalidRange{range, buffer_length} => write!(f, "failure on getting range: {range:#?}, for length: {buffer_length}"), } } } From fab5fbedbc23d81fa25d953a834e34eb2e4b8568 Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Thu, 27 Apr 2023 14:56:24 -0500 Subject: [PATCH 07/15] adds simple test for vector --- ssz-rs/tests/containers.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ssz-rs/tests/containers.rs b/ssz-rs/tests/containers.rs index ed8bface..42c99c6c 100644 --- a/ssz-rs/tests/containers.rs +++ b/ssz-rs/tests/containers.rs @@ -21494,6 +21494,14 @@ fn test_containers_bits_struct_one_chaos_2() { assert_eq!(root, expected_root); } +#[test] +fn test_containers_not_enough_input() { + type Goal = Vector; + let source = vec![]; + let result = Goal::deserialize(&source); + assert!(result.is_err()); +} + #[test] #[should_panic] fn test_containers_bits_struct_lengthy_offset_6_plus_one() { From 0f23579cf5719f621734697d3e7093f96f18e338 Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Sat, 6 May 2023 17:57:56 -0400 Subject: [PATCH 08/15] simplify oor vector test --- ssz-rs/tests/containers.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ssz-rs/tests/containers.rs b/ssz-rs/tests/containers.rs index 42c99c6c..cbb481f3 100644 --- a/ssz-rs/tests/containers.rs +++ b/ssz-rs/tests/containers.rs @@ -21496,9 +21496,8 @@ fn test_containers_bits_struct_one_chaos_2() { #[test] fn test_containers_not_enough_input() { - type Goal = Vector; let source = vec![]; - let result = Goal::deserialize(&source); + let result = Vector::::deserialize(&source); assert!(result.is_err()); } From a1315c3f27ec5242ed7c865d18943ac7a4c6cacb Mon Sep 17 00:00:00 2001 From: 0xTylerHolmes Date: Sat, 6 May 2023 18:17:30 -0400 Subject: [PATCH 09/15] use target var to check and return result for field deserialization --- ssz-rs-derive/src/lib.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 02b51bf6..cdf9e7ea 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -192,26 +192,25 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { Some(field_name) => quote_spanned! { f.span() => let bytes_read = if <#field_type>::is_variable_size() { let end = start + #BYTES_PER_LENGTH_OFFSET; - let next_offset = u32::deserialize( - encoding.get(start..end) - .ok_or(DeserializeError::ExpectedFurtherInput { - expected: end, - provided:encoding.len(), - })? + let target = encoding.get(start..end).ok_or( + ssz_rs::DeserializeError::ExpectedFurtherInput{ + provided: encoding.len(), + expected: #BYTES_PER_LENGTH_OFFSET,} )?; + let next_offset = u32::deserialize(target)?; offsets.push((#i, next_offset as usize)); #BYTES_PER_LENGTH_OFFSET } else { let encoded_length = <#field_type>::size_hint(); let end = start + encoded_length; - let result = <#field_type>::deserialize( - encoding.get(start..end) - .ok_or(DeserializeError::ExpectedFurtherInput { - expected: end, - provided:encoding.len(), - })? + let target = encoding.get(start..end).ok_or( + ssz_rs::DeserializeError::ExpectedFurtherInput{ + provided: encoding.len(), + expected: encoded_length, + } )?; + let result = <#field_type>::deserialize(target)?; container.#field_name = result; encoded_length }; From bf698d532480a2cb036ba544a419a59ec185154c Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 5 Jun 2023 15:37:08 -0600 Subject: [PATCH 10/15] add explicit test for `Vector` with no input --- ssz-rs/src/vector.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ssz-rs/src/vector.rs b/ssz-rs/src/vector.rs index 987ddccf..a4d1dac5 100644 --- a/ssz-rs/src/vector.rs +++ b/ssz-rs/src/vector.rs @@ -336,6 +336,13 @@ mod tests { assert_eq!(result, expected); } + #[test] + fn decode_vector_with_no_input() { + let source = vec![]; + let result = Vector::::deserialize(&source); + assert!(matches!(result, Err(DeserializeError::ExpectedFurtherInput { .. }))); + } + #[test] fn decode_variable_vector() { const COUNT: usize = 4; From 2f0d5cc8a8eedc37987b8c02b0c97fcded3ff7a8 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 5 Jun 2023 15:37:25 -0600 Subject: [PATCH 11/15] add explicit test for container with no input --- ssz-rs/src/container.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ssz-rs/src/container.rs b/ssz-rs/src/container.rs index a319f7fb..1a142836 100644 --- a/ssz-rs/src/container.rs +++ b/ssz-rs/src/container.rs @@ -162,6 +162,13 @@ mod tests { assert!(result.is_err()); } + #[test] + fn decode_container_with_no_input() { + let data = vec![]; + let result = VarTestStruct::deserialize(&data); + assert!(matches!(result, Err(DeserializeError::ExpectedFurtherInput { .. }))); + } + #[test] fn can_derive_struct_with_const_generics() { let value = VarWithGenericTestStruct { From bb9676976bef9c731ef4a33f9fedd814435174cb Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 5 Jun 2023 15:38:09 -0600 Subject: [PATCH 12/15] rollback change to generated tests --- ssz-rs/tests/containers.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ssz-rs/tests/containers.rs b/ssz-rs/tests/containers.rs index cbb481f3..ed8bface 100644 --- a/ssz-rs/tests/containers.rs +++ b/ssz-rs/tests/containers.rs @@ -21494,13 +21494,6 @@ fn test_containers_bits_struct_one_chaos_2() { assert_eq!(root, expected_root); } -#[test] -fn test_containers_not_enough_input() { - let source = vec![]; - let result = Vector::::deserialize(&source); - assert!(result.is_err()); -} - #[test] #[should_panic] fn test_containers_bits_struct_lengthy_offset_6_plus_one() { From 0ed404285478f84b843ede82ceea31107619632e Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 5 Jun 2023 15:43:35 -0600 Subject: [PATCH 13/15] scope error more precisely --- ssz-rs-derive/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index cdf9e7ea..8c08a835 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -193,9 +193,10 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let bytes_read = if <#field_type>::is_variable_size() { let end = start + #BYTES_PER_LENGTH_OFFSET; let target = encoding.get(start..end).ok_or( - ssz_rs::DeserializeError::ExpectedFurtherInput{ - provided: encoding.len(), - expected: #BYTES_PER_LENGTH_OFFSET,} + ssz_rs::DeserializeError::ExpectedFurtherInput { + provided: encoding.len() - start, + expected: #BYTES_PER_LENGTH_OFFSET, + } )?; let next_offset = u32::deserialize(target)?; offsets.push((#i, next_offset as usize)); @@ -206,7 +207,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { let end = start + encoded_length; let target = encoding.get(start..end).ok_or( ssz_rs::DeserializeError::ExpectedFurtherInput{ - provided: encoding.len(), + provided: encoding.len() - start, expected: encoded_length, } )?; From 6e645e39d0eb9a18007c654a409adb7cc50d982a Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 5 Jun 2023 15:52:52 -0600 Subject: [PATCH 14/15] add tests for the fixed and variable decoding cases --- ssz-rs/src/container.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/ssz-rs/src/container.rs b/ssz-rs/src/container.rs index 1a142836..0b573474 100644 --- a/ssz-rs/src/container.rs +++ b/ssz-rs/src/container.rs @@ -61,6 +61,13 @@ mod tests { #[derive(Default, Debug, PartialEq, Eq, SimpleSerialize)] struct TupleStruct(u8); + #[derive(Default, Debug, PartialEq, Eq, SimpleSerialize)] + struct AnotherVarTestStruct { + a: List, + b: u16, + c: u8, + } + #[test] fn encode_container() { let value = Foo { a: 5u32 }; @@ -163,9 +170,16 @@ mod tests { } #[test] - fn decode_container_with_no_input() { + fn decode_variable_container_with_no_input() { let data = vec![]; - let result = VarTestStruct::deserialize(&data); + let result = AnotherVarTestStruct::deserialize(&data); + assert!(matches!(result, Err(DeserializeError::ExpectedFurtherInput { .. }))); + } + + #[test] + fn decode_fixed_container_with_no_input() { + let data = vec![]; + let result = BasicContainer::deserialize(&data); assert!(matches!(result, Err(DeserializeError::ExpectedFurtherInput { .. }))); } From 1dcb199a734f144bd96d35ed8c1c66799c36a75d Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 5 Jun 2023 15:56:46 -0600 Subject: [PATCH 15/15] avoid always computing error --- ssz-rs-derive/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ssz-rs-derive/src/lib.rs b/ssz-rs-derive/src/lib.rs index 8c08a835..d3aee4a5 100644 --- a/ssz-rs-derive/src/lib.rs +++ b/ssz-rs-derive/src/lib.rs @@ -192,7 +192,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { Some(field_name) => quote_spanned! { f.span() => let bytes_read = if <#field_type>::is_variable_size() { let end = start + #BYTES_PER_LENGTH_OFFSET; - let target = encoding.get(start..end).ok_or( + let target = encoding.get(start..end).ok_or_else(|| ssz_rs::DeserializeError::ExpectedFurtherInput { provided: encoding.len() - start, expected: #BYTES_PER_LENGTH_OFFSET, @@ -205,7 +205,7 @@ fn derive_deserialize_impl(data: &Data) -> TokenStream { } else { let encoded_length = <#field_type>::size_hint(); let end = start + encoded_length; - let target = encoding.get(start..end).ok_or( + let target = encoding.get(start..end).ok_or_else(|| ssz_rs::DeserializeError::ExpectedFurtherInput{ provided: encoding.len() - start, expected: encoded_length,