fix: check payload length and consumed buf for pooled tx (#5153)

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)?;
 

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<TransactionSignedEcRecovered> 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));
+    }
+}

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<Self> {
         // 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 })
     }
 }