From 2f15e281183f4448bf6d68479720346afa70616e Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 23 Oct 2023 23:23:03 -0400 Subject: [PATCH 1/6] fix: check payload length and consumed buf for pooled tx --- crates/primitives/src/transaction/pooled.rs | 85 +++++++++++++++++-- crates/primitives/src/transaction/tx_value.rs | 22 +++++ 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/crates/primitives/src/transaction/pooled.rs b/crates/primitives/src/transaction/pooled.rs index 1331c59c247d..20088754980f 100644 --- a/crates/primitives/src/transaction/pooled.rs +++ b/crates/primitives/src/transaction/pooled.rs @@ -324,22 +324,29 @@ impl Decodable for PooledTransactionsElement { return Err(RlpError::InputTooShort) } - // keep this around for buffer advancement post-legacy decoding - let mut original_encoding = *buf; - // If the header is a list header, it is a legacy transaction. Otherwise, it is a typed // transaction let header = Header::decode(buf)?; // Check if the tx is a list if header.list { + // only decode the length of the list, since we already decoded the header + let transaction_length = header.payload_length; + let original_len = buf.len(); + + if transaction_length > original_len { + return Err(RlpError::InputTooShort) + } + // decode as legacy transaction let (transaction, hash, signature) = - TransactionSigned::decode_rlp_legacy_transaction_tuple(&mut original_encoding)?; + TransactionSigned::decode_rlp_legacy_transaction_tuple(buf)?; - // advance the buffer based on how far `decode_rlp_legacy_transaction` advanced the - // buffer - *buf = original_encoding; + // check the new length, compared to the original length and the header length + let decoded = original_len - buf.len(); + if decoded != transaction_length { + return Err(RlpError::UnexpectedLength) + } Ok(Self::Legacy { transaction, signature, hash }) } else { @@ -517,3 +524,67 @@ impl From for PooledTransactionsElementEcRecovered Self { transaction, signer } } } + +#[cfg(test)] +mod tests { + use super::*; + use alloy_primitives::hex; + use assert_matches::assert_matches; + + #[test] + fn invalid_pooled_decoding_input_too_short() { + let input_too_short = [ + // this should fail because the payload length is longer than expected + &hex!("d90b0280808bc5cd028083c5cdfd9e407c56565656")[..], + // these should fail decoding + // + // The `c1` at the beginning is a list header, and the rest is a valid legacy + // transaction, BUT the payload length of the list header is 1, and the payload is + // obviously longer than one byte. + &hex!("c10b02808083c5cd028883c5cdfd9e407c56565656"), + &hex!("c10b0280808bc5cd028083c5cdfd9e407c56565656"), + // these are too long for `d3` + &hex!("d30b02808083c5cdeb8783c5acfd9e407c56565656"), + &hex!("d30102808083c5cd02887dc5cdfd9e64fd9e407c56"), + ]; + + for hex_data in input_too_short.iter() { + let input_rlp = &mut &hex_data[..]; + let res = PooledTransactionsElement::decode(input_rlp); + + assert!( + res.is_err(), + "expected err after decoding rlp input: {:x?}", + Bytes::copy_from_slice(hex_data) + ); + } + } + + #[test] + fn leading_zeros_tx_value() { + let leading_zeros = [ + // this one has a correct payload length but contains a txvalue with leading zeros + &hex!("d4141d80808300b21d88fcbab282e13a7e53525a54")[..], + ]; + + for hex_data in leading_zeros.iter() { + let input_rlp = &mut &hex_data[..]; + let res = PooledTransactionsElement::decode(input_rlp); + + assert!( + res.is_err(), + "expected err after decoding rlp input: {:x?}", + Bytes::copy_from_slice(hex_data) + ); + } + } + + #[test] + fn valid_pooled_decoding() { + let data = &mut &hex!("d30b02808083c5cdeb8783c5acfd9e407c565656")[..]; + + let res = PooledTransactionsElement::decode(data); + assert_matches!(res, Ok(_tx)); + assert!(data.is_empty()); + } +} diff --git a/crates/primitives/src/transaction/tx_value.rs b/crates/primitives/src/transaction/tx_value.rs index e9a5f536e9ff..5d2369f758cb 100644 --- a/crates/primitives/src/transaction/tx_value.rs +++ b/crates/primitives/src/transaction/tx_value.rs @@ -150,3 +150,25 @@ impl proptest::arbitrary::Arbitrary for TxValue { type Strategy = BoxedStrategy; } + +#[cfg(test)] +mod tests { + use super::*; + use alloy_primitives::hex; + + #[test] + fn tx_value_invalid_leading_zeros() { + let leading_zeros = [ + // this one has a correct payload length but contains a txvalue with leading zeros + &hex!("8300b21d")[..], + &hex!("840000b21d")[..], + ]; + + for hex_data in leading_zeros.iter() { + let input_rlp = &mut &hex_data[..]; + // try to decode + let res = U256::decode(input_rlp); + assert!(res.is_err()); + } + } +} From dd8cc156a1f7276473ff805ffb679b81f503a296 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 26 Oct 2023 14:38:27 -0400 Subject: [PATCH 2/6] perform legacy length checks in decode_legacy tuple method --- crates/primitives/src/transaction/mod.rs | 15 ++++- crates/primitives/src/transaction/pooled.rs | 69 ++++++++++++++------- 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/crates/primitives/src/transaction/mod.rs b/crates/primitives/src/transaction/mod.rs index 67384c50d6f5..86cbfce3dae7 100644 --- a/crates/primitives/src/transaction/mod.rs +++ b/crates/primitives/src/transaction/mod.rs @@ -958,8 +958,6 @@ impl TransactionSigned { /// /// Refer to the docs for [Self::decode_rlp_legacy_transaction] for details on the exact /// format expected. - // TODO: make buf advancement semantics consistent with `decode_enveloped_typed_transaction`, - // so decoding methods do not need to manually advance the buffer pub(crate) fn decode_rlp_legacy_transaction_tuple( data: &mut &[u8], ) -> alloy_rlp::Result<(TxLegacy, TxHash, Signature)> { @@ -967,6 +965,13 @@ impl TransactionSigned { let original_encoding = *data; let header = Header::decode(data)?; + let remaining_len = data.len(); + + let transaction_payload_len = header.payload_length; + + if transaction_payload_len > remaining_len { + return Err(RlpError::InputTooShort) + } let mut transaction = TxLegacy { nonce: Decodable::decode(data)?, @@ -980,6 +985,12 @@ impl TransactionSigned { let (signature, extracted_id) = Signature::decode_with_eip155_chain_id(data)?; transaction.chain_id = extracted_id; + // check the new length, compared to the original length and the header length + let decoded = remaining_len - data.len(); + if decoded != transaction_payload_len { + return Err(RlpError::UnexpectedLength) + } + let tx_length = header.payload_length + header.length(); let hash = keccak256(&original_encoding[..tx_length]); Ok((transaction, hash, signature)) diff --git a/crates/primitives/src/transaction/pooled.rs b/crates/primitives/src/transaction/pooled.rs index 20088754980f..65ede104ea14 100644 --- a/crates/primitives/src/transaction/pooled.rs +++ b/crates/primitives/src/transaction/pooled.rs @@ -324,29 +324,22 @@ impl Decodable for PooledTransactionsElement { return Err(RlpError::InputTooShort) } + // keep the original buf around for legacy decoding + let mut original_encoding = *buf; + // If the header is a list header, it is a legacy transaction. Otherwise, it is a typed // transaction let header = Header::decode(buf)?; // Check if the tx is a list if header.list { - // only decode the length of the list, since we already decoded the header - let transaction_length = header.payload_length; - let original_len = buf.len(); - - if transaction_length > original_len { - return Err(RlpError::InputTooShort) - } - // decode as legacy transaction let (transaction, hash, signature) = - TransactionSigned::decode_rlp_legacy_transaction_tuple(buf)?; + TransactionSigned::decode_rlp_legacy_transaction_tuple(&mut original_encoding)?; - // check the new length, compared to the original length and the header length - let decoded = original_len - buf.len(); - if decoded != transaction_length { - return Err(RlpError::UnexpectedLength) - } + // advance the buffer by however long the legacy transaction decoding advanced the + // buffer + *buf = original_encoding; Ok(Self::Legacy { transaction, signature, hash }) } else { @@ -532,7 +525,7 @@ mod tests { use assert_matches::assert_matches; #[test] - fn invalid_pooled_decoding_input_too_short() { + fn invalid_legacy_pooled_decoding_input_too_short() { let input_too_short = [ // this should fail because the payload length is longer than expected &hex!("d90b0280808bc5cd028083c5cdfd9e407c56565656")[..], @@ -543,8 +536,9 @@ mod tests { // obviously longer than one byte. &hex!("c10b02808083c5cd028883c5cdfd9e407c56565656"), &hex!("c10b0280808bc5cd028083c5cdfd9e407c56565656"), - // these are too long for `d3` - &hex!("d30b02808083c5cdeb8783c5acfd9e407c56565656"), + // this one is 19 bytes, and the buf is long enough, but the transaction will not + // consume that many bytes. + &hex!("d40b02808083c5cdeb8783c5acfd9e407c5656565656"), &hex!("d30102808083c5cd02887dc5cdfd9e64fd9e407c56"), ]; @@ -557,6 +551,17 @@ mod tests { "expected err after decoding rlp input: {:x?}", Bytes::copy_from_slice(hex_data) ); + + // this is a legacy tx so we can attempt the same test with decode_enveloped + let input_rlp = &mut &hex_data[..]; + let res = + PooledTransactionsElement::decode_enveloped(Bytes::copy_from_slice(input_rlp)); + + assert!( + res.is_err(), + "expected err after decoding enveloped rlp input: {:x?}", + Bytes::copy_from_slice(hex_data) + ); } } @@ -580,11 +585,33 @@ mod tests { } #[test] - fn valid_pooled_decoding() { - let data = &mut &hex!("d30b02808083c5cdeb8783c5acfd9e407c565656")[..]; + fn legacy_valid_pooled_decoding() { + // d3 <- payload length, d3 - c0 = 0x13 = 19 + // 0b + // 02 + // 80 + // 80 + // 83 c5cdeb + // 87 83c5acfd9e407c + // 56 + // 56 + // 56 + let data = &hex!("d30b02808083c5cdeb8783c5acfd9e407c565656")[..]; + + let input_rlp = &mut &data[..]; + let res = PooledTransactionsElement::decode(input_rlp); + assert_matches!(res, Ok(_tx)); + assert!(input_rlp.is_empty()); + + // this is a legacy tx so we can attempt the same test with + // decode_rlp_legacy_transaction_tuple + let input_rlp = &mut &data[..]; + let res = TransactionSigned::decode_rlp_legacy_transaction_tuple(input_rlp); + assert_matches!(res, Ok(_tx)); + assert!(input_rlp.is_empty()); - let res = PooledTransactionsElement::decode(data); + // we can also decode_enveloped + let res = PooledTransactionsElement::decode_enveloped(Bytes::copy_from_slice(data)); assert_matches!(res, Ok(_tx)); - assert!(data.is_empty()); } } From b5f8a210fab3baa4a3edc707e03079ae72a81119 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 26 Oct 2023 15:20:27 -0400 Subject: [PATCH 3/6] add length checks for other decodings --- crates/primitives/src/transaction/mod.rs | 7 ++++++ crates/primitives/src/transaction/pooled.rs | 14 ++++++++++++ crates/primitives/src/transaction/sidecar.rs | 24 ++++++++++++++++---- 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/crates/primitives/src/transaction/mod.rs b/crates/primitives/src/transaction/mod.rs index 86cbfce3dae7..7c5cc142c4a9 100644 --- a/crates/primitives/src/transaction/mod.rs +++ b/crates/primitives/src/transaction/mod.rs @@ -1040,6 +1040,8 @@ impl TransactionSigned { return Err(RlpError::Custom("typed tx fields must be encoded as a list")) } + let remaining_len = data.len(); + // length of tx encoding = tx type byte (size = 1) + length of header + payload length let tx_length = 1 + header.length() + header.payload_length; @@ -1053,6 +1055,11 @@ impl TransactionSigned { let signature = Signature::decode(data)?; + let bytes_consumed = remaining_len - data.len(); + if bytes_consumed != header.payload_length { + return Err(RlpError::UnexpectedLength) + } + let hash = keccak256(&original_encoding[..tx_length]); let signed = TransactionSigned { transaction, hash, signature }; Ok(signed) diff --git a/crates/primitives/src/transaction/pooled.rs b/crates/primitives/src/transaction/pooled.rs index 65ede104ea14..e7590800ce08 100644 --- a/crates/primitives/src/transaction/pooled.rs +++ b/crates/primitives/src/transaction/pooled.rs @@ -345,6 +345,7 @@ impl Decodable for PooledTransactionsElement { } else { // decode the type byte, only decode BlobTransaction if it is a 4844 transaction let tx_type = *buf.first().ok_or(RlpError::InputTooShort)?; + let remaining_len = buf.len(); if tx_type == EIP4844_TX_TYPE_ID { // Recall that the blob transaction response `TranactionPayload` is encoded like @@ -362,12 +363,25 @@ impl Decodable for PooledTransactionsElement { // Now, we decode the inner blob transaction: // `rlp([[chain_id, nonce, ...], blobs, commitments, proofs])` let blob_tx = BlobTransaction::decode_inner(buf)?; + + // check that the bytes consumed match the payload length + let bytes_consumed = remaining_len - buf.len(); + if bytes_consumed != header.payload_length { + return Err(RlpError::UnexpectedLength) + } + Ok(PooledTransactionsElement::BlobTransaction(blob_tx)) } else { // DO NOT advance the buffer for the type, since we want the enveloped decoding to // decode it again and advance the buffer on its own. let typed_tx = TransactionSigned::decode_enveloped_typed_transaction(buf)?; + // check that the bytes consumed match the payload length + let bytes_consumed = remaining_len - buf.len(); + if bytes_consumed != header.payload_length { + return Err(RlpError::UnexpectedLength) + } + // because we checked the tx type, we can be sure that the transaction is not a // blob transaction or legacy match typed_tx.transaction { diff --git a/crates/primitives/src/transaction/sidecar.rs b/crates/primitives/src/transaction/sidecar.rs index 1ab6bc117fc3..0f51c0df7303 100644 --- a/crates/primitives/src/transaction/sidecar.rs +++ b/crates/primitives/src/transaction/sidecar.rs @@ -237,27 +237,37 @@ impl BlobTransaction { /// represent the full RLP decoding of the `PooledTransactionsElement` type. pub(crate) fn decode_inner(data: &mut &[u8]) -> alloy_rlp::Result { // decode the _first_ list header for the rest of the transaction - let header = Header::decode(data)?; - if !header.list { + let outer_header = Header::decode(data)?; + if !outer_header.list { return Err(RlpError::Custom("PooledTransactions blob tx must be encoded as a list")) } + let outer_remaining_len = data.len(); + // Now we need to decode the inner 4844 transaction and its signature: // // `[chain_id, nonce, max_priority_fee_per_gas, ..., y_parity, r, s]` - let header = Header::decode(data)?; - if !header.list { + let inner_header = Header::decode(data)?; + if !inner_header.list { return Err(RlpError::Custom( "PooledTransactions inner blob tx must be encoded as a list", )) } + let inner_remaining_len = data.len(); + // inner transaction let transaction = TxEip4844::decode_inner(data)?; // signature let signature = Signature::decode(data)?; + // the inner header only decodes the transaction and signature, so we check the length here + let inner_consumed = inner_remaining_len - data.len(); + if inner_consumed != inner_header.payload_length { + return Err(RlpError::UnexpectedLength) + } + // All that's left are the blobs, commitments, and proofs let sidecar = BlobTransactionSidecar::decode_inner(data)?; @@ -281,6 +291,12 @@ impl BlobTransaction { transaction.encode_with_signature(&signature, &mut buf, false); let hash = keccak256(&buf); + // the outer header is for the entire transaction, so we check the length here + let outer_consumed = outer_remaining_len - data.len(); + if outer_consumed != outer_header.payload_length { + return Err(RlpError::UnexpectedLength) + } + Ok(Self { transaction, hash, signature, sidecar }) } } From 1006a3e91d4682a17bd0eb7e612f9e24d7167757 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 26 Oct 2023 15:47:07 -0400 Subject: [PATCH 4/6] add length check for TransactionSigned::decode --- crates/primitives/src/transaction/mod.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/primitives/src/transaction/mod.rs b/crates/primitives/src/transaction/mod.rs index 7c5cc142c4a9..5cd7cfdf6b93 100644 --- a/crates/primitives/src/transaction/mod.rs +++ b/crates/primitives/src/transaction/mod.rs @@ -1159,9 +1159,21 @@ impl Decodable for TransactionSigned { let mut original_encoding = *buf; let header = Header::decode(buf)?; + let remaining_len = buf.len(); + // if the transaction is encoded as a string then it is a typed transaction if !header.list { - TransactionSigned::decode_enveloped_typed_transaction(buf) + let tx = TransactionSigned::decode_enveloped_typed_transaction(buf)?; + + let bytes_consumed = remaining_len - buf.len(); + // because Header::decode works for single bytes (including the tx type), returning a + // string Header with payload_length of 1, we need to make sure this check is only + // performed for transactions with a string header + if bytes_consumed != header.payload_length && original_encoding[0] > EMPTY_STRING_CODE { + return Err(RlpError::UnexpectedLength) + } + + Ok(tx) } else { let tx = TransactionSigned::decode_rlp_legacy_transaction(&mut original_encoding)?; From 19df3ea3fba7fa50a3c6bd2bfc2e25bad6d89fcd Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 26 Oct 2023 16:10:49 -0400 Subject: [PATCH 5/6] remove tx_value tests ruint PR should be merged first --- crates/primitives/src/transaction/pooled.rs | 19 ---------------- crates/primitives/src/transaction/tx_value.rs | 22 ------------------- 2 files changed, 41 deletions(-) diff --git a/crates/primitives/src/transaction/pooled.rs b/crates/primitives/src/transaction/pooled.rs index e7590800ce08..2eab3484f18a 100644 --- a/crates/primitives/src/transaction/pooled.rs +++ b/crates/primitives/src/transaction/pooled.rs @@ -579,25 +579,6 @@ mod tests { } } - #[test] - fn leading_zeros_tx_value() { - let leading_zeros = [ - // this one has a correct payload length but contains a txvalue with leading zeros - &hex!("d4141d80808300b21d88fcbab282e13a7e53525a54")[..], - ]; - - for hex_data in leading_zeros.iter() { - let input_rlp = &mut &hex_data[..]; - let res = PooledTransactionsElement::decode(input_rlp); - - assert!( - res.is_err(), - "expected err after decoding rlp input: {:x?}", - Bytes::copy_from_slice(hex_data) - ); - } - } - #[test] fn legacy_valid_pooled_decoding() { // d3 <- payload length, d3 - c0 = 0x13 = 19 diff --git a/crates/primitives/src/transaction/tx_value.rs b/crates/primitives/src/transaction/tx_value.rs index 5d2369f758cb..e9a5f536e9ff 100644 --- a/crates/primitives/src/transaction/tx_value.rs +++ b/crates/primitives/src/transaction/tx_value.rs @@ -150,25 +150,3 @@ impl proptest::arbitrary::Arbitrary for TxValue { type Strategy = BoxedStrategy; } - -#[cfg(test)] -mod tests { - use super::*; - use alloy_primitives::hex; - - #[test] - fn tx_value_invalid_leading_zeros() { - let leading_zeros = [ - // this one has a correct payload length but contains a txvalue with leading zeros - &hex!("8300b21d")[..], - &hex!("840000b21d")[..], - ]; - - for hex_data in leading_zeros.iter() { - let input_rlp = &mut &hex_data[..]; - // try to decode - let res = U256::decode(input_rlp); - assert!(res.is_err()); - } - } -} From 5bffcc68c84aaa6abb814da1f6864a8c21bb8126 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 26 Oct 2023 16:14:18 -0400 Subject: [PATCH 6/6] finish test comment --- crates/primitives/src/transaction/pooled.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/primitives/src/transaction/pooled.rs b/crates/primitives/src/transaction/pooled.rs index 2eab3484f18a..2d2d8a322f20 100644 --- a/crates/primitives/src/transaction/pooled.rs +++ b/crates/primitives/src/transaction/pooled.rs @@ -582,15 +582,15 @@ mod tests { #[test] fn legacy_valid_pooled_decoding() { // d3 <- payload length, d3 - c0 = 0x13 = 19 - // 0b - // 02 - // 80 - // 80 - // 83 c5cdeb - // 87 83c5acfd9e407c - // 56 - // 56 - // 56 + // 0b <- nonce + // 02 <- gas_price + // 80 <- gas_limit + // 80 <- to (Create) + // 83 c5cdeb <- value + // 87 83c5acfd9e407c <- input + // 56 <- v (eip155, so modified with a chain id) + // 56 <- r + // 56 <- s let data = &hex!("d30b02808083c5cdeb8783c5acfd9e407c565656")[..]; let input_rlp = &mut &data[..];