rlp: disallow trailing bytes in transactions (#10296) (#12175)

Cherry pick #10296

Co-authored-by: racytech <[email protected]>

core/types/encdec_test.go

@@ -532,7 +532,7 @@ func TestBodyEncodeDecodeRLP(t *testing.T) {
 		}
 
 		if err := compareBodies(t, enc, dec); err != nil {
-			t.Errorf("error: compareRawBodies: %v", err)
+			t.Errorf("error: compareBodies: %v", err)
 		}
 	}
 }

core/types/legacy_tx.go

@@ -289,10 +289,11 @@ func (tx *LegacyTx) EncodeRLP(w io.Writer) error {
 	return nil
 }
 
-// DecodeRLP decodes LegacyTx but with the list token already consumed and encodingSize being presented
-func (tx *LegacyTx) DecodeRLP(s *rlp.Stream, encodingSize uint64) error {
-	var err error
-	s.NewList(encodingSize)
+func (tx *LegacyTx) DecodeRLP(s *rlp.Stream) error {
+	_, err := s.List()
+	if err != nil {
+		return fmt.Errorf("legacy tx must be a list: %w", err)
+	}
 	if tx.Nonce, err = s.Uint(); err != nil {
 		return fmt.Errorf("read Nonce: %w", err)
 	}

core/types/transaction.go

@@ -78,6 +78,7 @@ type Transaction interface {
 	RawSignatureValues() (*uint256.Int, *uint256.Int, *uint256.Int)
 	EncodingSize() int
 	EncodeRLP(w io.Writer) error
+	DecodeRLP(s *rlp.Stream) error
 	MarshalBinary(w io.Writer) error
 	// Sender returns the address derived from the signature (V, R, S) using secp256k1
 	// elliptic curve and an error if it failed deriving or upon an incorrect
@@ -113,19 +114,19 @@ func (t BinaryTransactions) EncodeIndex(i int, w *bytes.Buffer) {
 }
 
 func DecodeRLPTransaction(s *rlp.Stream, blobTxnsAreWrappedWithBlobs bool) (Transaction, error) {
-	kind, size, err := s.Kind()
+	kind, _, err := s.Kind()
 	if err != nil {
 		return nil, err
 	}
 	if rlp.List == kind {
 		tx := &LegacyTx{}
-		if err = tx.DecodeRLP(s, size); err != nil {
+		if err = tx.DecodeRLP(s); err != nil {
 			return nil, err
 		}
 		return tx, nil
 	}
 	if rlp.String != kind {
-		return nil, fmt.Errorf("Not an RLP encoded transaction. If this is a canonical encoded transaction, use UnmarshalTransactionFromBinary instead. Got %v for kind, expected String", kind)
+		return nil, fmt.Errorf("not an RLP encoded transaction. If this is a canonical encoded transaction, use UnmarshalTransactionFromBinary instead. Got %v for kind, expected String", kind)
 	}
 	// Decode the EIP-2718 typed TX envelope.
 	var b []byte
@@ -163,7 +164,14 @@ func DecodeTransaction(data []byte) (Transaction, error) {
 		return UnmarshalTransactionFromBinary(data, blobTxnsAreWrappedWithBlobs)
 	}
 	s := rlp.NewStream(bytes.NewReader(data), uint64(len(data)))
-	return DecodeRLPTransaction(s, blobTxnsAreWrappedWithBlobs)
+	tx, err := DecodeRLPTransaction(s, blobTxnsAreWrappedWithBlobs)
+	if err != nil {
+		return nil, err
+	}
+	if s.Remaining() != 0 {
+		return nil, fmt.Errorf("trailing bytes after rlp encoded transaction")
+	}
+	return tx, nil
 }
 
 // Parse transaction without envelope.
@@ -172,32 +180,17 @@ func UnmarshalTransactionFromBinary(data []byte, blobTxnsAreWrappedWithBlobs boo
 		return nil, fmt.Errorf("short input: %v", len(data))
 	}
 	s := rlp.NewStream(bytes.NewReader(data[1:]), uint64(len(data)-1))
+	var t Transaction
 	switch data[0] {
 	case AccessListTxType:
-		t := &AccessListTx{}
-		if err := t.DecodeRLP(s); err != nil {
-			return nil, err
-		}
-		return t, nil
+		t = &AccessListTx{}
 	case DynamicFeeTxType:
-		t := &DynamicFeeTransaction{}
-		if err := t.DecodeRLP(s); err != nil {
-			return nil, err
-		}
-		return t, nil
+		t = &DynamicFeeTransaction{}
 	case BlobTxType:
 		if blobTxnsAreWrappedWithBlobs {
-			t := &BlobTxWrapper{}
-			if err := t.DecodeRLP(s); err != nil {
-				return nil, err
-			}
-			return t, nil
+			t = &BlobTxWrapper{}
 		} else {
-			t := &BlobTx{}
-			if err := t.DecodeRLP(s); err != nil {
-				return nil, err
-			}
-			return t, nil
+			t = &BlobTx{}
 		}
 	default:
 		if data[0] >= 0x80 {
@@ -206,6 +199,13 @@ func UnmarshalTransactionFromBinary(data []byte, blobTxnsAreWrappedWithBlobs boo
 		}
 		return nil, ErrTxTypeNotSupported
 	}
+	if err := t.DecodeRLP(s); err != nil {
+		return nil, err
+	}
+	if s.Remaining() != 0 {
+		return nil, fmt.Errorf("trailing bytes after rlp encoded transaction")
+	}
+	return t, nil
 }
 
 // Remove everything but the payload body from the wrapper - this is not used, for reference only

core/types/transaction_test.go

@@ -826,3 +826,46 @@ func TestShortUnwrapLib(t *testing.T) {
 
 	assertEqual(blobTx, &wrappedBlobTx.Tx)
 }
+
+func TestTrailingBytes(t *testing.T) {
+	// Create a valid transaction
+	valid_rlp_transaction := []byte{201, 38, 38, 128, 128, 107, 58, 42, 38, 42}
+
+	// Test valid transaction
+	transactions := make([][]byte, 1)
+	transactions[0] = valid_rlp_transaction
+
+	for _, txn := range transactions {
+		if TypedTransactionMarshalledAsRlpString(txn) {
+			panic("TypedTransactionMarshalledAsRlpString() error")
+		}
+	}
+
+	_, err := DecodeTransactions(transactions)
+	if err != nil {
+		fmt.Println("Valid transaction errored")
+		panic(err) // @audit this will pass
+	}
+
+	// Append excess bytes to the blob transaction
+	num_excess := 100
+	malicious_rlp_transaction := make([]byte, len(valid_rlp_transaction)+num_excess)
+	copy(malicious_rlp_transaction, valid_rlp_transaction)
+
+	// Validate transactions are different
+	assert.NotEqual(t, malicious_rlp_transaction, valid_rlp_transaction)
+
+	// Test malicious transaction
+	transactions[0] = malicious_rlp_transaction
+
+	for _, txn := range transactions {
+		if TypedTransactionMarshalledAsRlpString(txn) {
+			panic("TypedTransactionMarshalledAsRlpString() error")
+		}
+	}
+
+	_, err = DecodeTransactions(transactions)
+	if err == nil {
+		panic("Malicious transaction has not errored!") // @audit this panic is occurs
+	}
+}

rlp/decode.go

@@ -672,6 +672,11 @@ func NewListStream(r io.Reader, len uint64) *Stream {
 	return s
 }
 
+// Remaining returns number of bytes remaining to be read
+func (s *Stream) Remaining() uint64 {
+	return s.remaining
+}
+
 // Bytes reads an RLP string and returns its contents as a byte slice.
 // If the input does not contain an RLP string, the returned
 // error will be ErrExpectedString.