diff --git a/crates/primitives/src/transaction/mod.rs b/crates/primitives/src/transaction/mod.rs index 67384c50d6f5..5cd7cfdf6b93 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)) @@ -1029,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; @@ -1042,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) @@ -1141,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)?; diff --git a/crates/primitives/src/transaction/pooled.rs b/crates/primitives/src/transaction/pooled.rs index 1331c59c247d..2d2d8a322f20 100644 --- a/crates/primitives/src/transaction/pooled.rs +++ b/crates/primitives/src/transaction/pooled.rs @@ -324,7 +324,7 @@ impl Decodable for PooledTransactionsElement { return Err(RlpError::InputTooShort) } - // keep this around for buffer advancement post-legacy decoding + // 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 @@ -337,7 +337,7 @@ impl Decodable for PooledTransactionsElement { let (transaction, hash, signature) = TransactionSigned::decode_rlp_legacy_transaction_tuple(&mut original_encoding)?; - // advance the buffer based on how far `decode_rlp_legacy_transaction` advanced the + // advance the buffer by however long the legacy transaction decoding advanced the // buffer *buf = original_encoding; @@ -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 { @@ -517,3 +531,82 @@ 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_legacy_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"), + // this one is 19 bytes, and the buf is long enough, but the transaction will not + // consume that many bytes. + &hex!("d40b02808083c5cdeb8783c5acfd9e407c5656565656"), + &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) + ); + + // 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) + ); + } + } + + #[test] + fn legacy_valid_pooled_decoding() { + // d3 <- payload length, d3 - c0 = 0x13 = 19 + // 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[..]; + 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()); + + // we can also decode_enveloped + let res = PooledTransactionsElement::decode_enveloped(Bytes::copy_from_slice(data)); + assert_matches!(res, Ok(_tx)); + } +} 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 }) } }