diff --git a/blockchain/chainio.go b/blockchain/chainio.go index 5d2c033c..9ce58b12 100644 --- a/blockchain/chainio.go +++ b/blockchain/chainio.go @@ -1277,8 +1277,7 @@ func (b *BlockChain) initChainState() error { if err != nil { return err } - var block wire.MsgBlock - err = block.Deserialize(bytes.NewReader(blockBytes)) + block, err := btcutil.NewBlockFromBytes(blockBytes) if err != nil { return err } @@ -1304,8 +1303,8 @@ func (b *BlockChain) initChainState() error { // Initialize the state related to the best block. blockSize := uint64(len(blockBytes)) - blockWeight := uint64(GetBlockWeight(btcutil.NewBlock(&block))) - numTxns := uint64(len(block.Transactions)) + blockWeight := uint64(GetBlockWeight(block)) + numTxns := uint64(len(block.MsgBlock().Transactions)) b.stateSnapshot = newBestState(tip, blockSize, blockWeight, numTxns, state.totalTxns, CalcPastMedianTime(tip)) diff --git a/blockchain/chainio_test.go b/blockchain/chainio_test.go index 6620f0c8..272bf61d 100644 --- a/blockchain/chainio_test.go +++ b/blockchain/chainio_test.go @@ -9,9 +9,12 @@ import ( "errors" "math/big" "reflect" + "strings" "testing" + "github.com/btcsuite/btcd/btcutil/v2" "github.com/btcsuite/btcd/database" + "github.com/btcsuite/btcd/txscript/v2" "github.com/btcsuite/btcd/wire/v2" ) @@ -37,6 +40,52 @@ func TestErrNotInMainChain(t *testing.T) { } } +// TestInitChainStateRejectsTrailingBestBlockBytes ensures startup rejects a +// stored best block whose bytes contain a valid block plus trailing data. +func TestInitChainStateRejectsTrailingBestBlockBytes(t *testing.T) { + chain, params, teardown := utxoCacheTestChain( + "TestInitChainStateRejectsTrailingBestBlockBytes") + defer teardown() + + tip := btcutil.NewBlock(params.GenesisBlock) + tip.SetHeight(0) + + block, _, err := newBlock(chain, tip, nil) + if err != nil { + t.Fatalf("failed to build block: %v", err) + } + + var serialized bytes.Buffer + err = block.MsgBlock().Serialize(&serialized) + if err != nil { + t.Fatalf("failed to serialize block: %v", err) + } + + trailingBytes := append([]byte(nil), serialized.Bytes()...) + trailingBytes = append(trailingBytes, 0x00) + trailingBlock := btcutil.NewBlockFromBlockAndBytes( + block.MsgBlock(), trailingBytes, + ) + + _, _, err = chain.ProcessBlock(trailingBlock, BFNone) + if err != nil { + t.Fatalf("failed to process block: %v", err) + } + + _, err = New(&Config{ + DB: chain.db, + ChainParams: params, + TimeSource: NewMedianTime(), + SigCache: txscript.NewSigCache(1000), + }) + if err == nil { + t.Fatal("expected trailing best block bytes to fail startup") + } + if !strings.Contains(err.Error(), "trailing bytes") { + t.Fatalf("expected trailing byte error, got: %v", err) + } +} + // TestStxoSerialization ensures serializing and deserializing spent transaction // output entries works as expected. func TestStxoSerialization(t *testing.T) { diff --git a/btcec/schnorr/musig2/sign.go b/btcec/schnorr/musig2/sign.go index 67d194db..fadae01f 100644 --- a/btcec/schnorr/musig2/sign.go +++ b/btcec/schnorr/musig2/sign.go @@ -91,7 +91,7 @@ func (p *PartialSignature) Decode(r io.Reader) error { var sBytes [32]byte if _, err := io.ReadFull(r, sBytes[:]); err != nil { - return nil + return err } overflows := p.S.SetBytes(&sBytes) diff --git a/btcec/schnorr/musig2/sign_test.go b/btcec/schnorr/musig2/sign_test.go index a967cfe4..19647398 100644 --- a/btcec/schnorr/musig2/sign_test.go +++ b/btcec/schnorr/musig2/sign_test.go @@ -311,6 +311,27 @@ func pSigsFromIndices(t *testing.T, sigs []string, indices []int) []*PartialSign return pSigs } +// TestPartialSignatureDecodeRejectsShortReads verifies that Decode rejects +// inputs that do not contain a full scalar. +func TestPartialSignatureDecodeRejectsShortReads(t *testing.T) { + t.Parallel() + + testCases := map[string][]byte{ + "empty": nil, + "truncated": bytes.Repeat([]byte{0x01}, 31), + } + + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + var sig PartialSignature + err := sig.Decode(bytes.NewReader(testCase)) + require.Error(t, err) + }) + } +} + // TestMusig2SignCombine tests that we pass the musig2 sig combination tests. func TestMusig2SignCombine(t *testing.T) { t.Parallel() diff --git a/btcutil/block.go b/btcutil/block.go index da7a47e6..4b36dc72 100644 --- a/btcutil/block.go +++ b/btcutil/block.go @@ -253,6 +253,9 @@ func NewBlockFromBytes(serializedBlock []byte) (*Block, error) { if err != nil { return nil, err } + if br.Len() > 0 { + return nil, fmt.Errorf("block has %d trailing bytes", br.Len()) + } b.serializedBlock = serializedBlock // This initializes []btcutil.Tx to have the serialized raw diff --git a/btcutil/block_test.go b/btcutil/block_test.go index e5ae9830..b626990c 100644 --- a/btcutil/block_test.go +++ b/btcutil/block_test.go @@ -200,6 +200,23 @@ func TestNewBlockFromBytes(t *testing.T) { } } +// TestNewBlockFromBytesRejectsTrailingData verifies that NewBlockFromBytes +// rejects bytes after the serialized block. +func TestNewBlockFromBytesRejectsTrailingData(t *testing.T) { + var block100000Buf bytes.Buffer + err := Block100000.Serialize(&block100000Buf) + if err != nil { + t.Errorf("Serialize: %v", err) + } + + _, err = btcutil.NewBlockFromBytes( + append(block100000Buf.Bytes(), 0x00), + ) + if err == nil { + t.Fatal("expected error for block with trailing data") + } +} + // TestNewBlockFromBlockAndBytes tests creation of a Block from a MsgBlock and // raw bytes. func TestNewBlockFromBlockAndBytes(t *testing.T) { diff --git a/btcutil/bloom/filter_test.go b/btcutil/bloom/filter_test.go index a16d21e0..1b61eb0d 100644 --- a/btcutil/bloom/filter_test.go +++ b/btcutil/bloom/filter_test.go @@ -325,7 +325,7 @@ func TestFilterBloomMatch(t *testing.T) { 0x02, 0x00, 0x00, 0x00, 0x00, 0x19, 0x76, 0xa9, 0x14, 0xc1, 0x09, 0x32, 0x48, 0x3f, 0xec, 0x93, 0xed, 0x51, 0xf5, 0xfe, 0x95, 0xe7, 0x25, 0x59, 0xf2, 0xcc, 0x70, - 0x43, 0xf9, 0x88, 0xac, 0x00, 0x00, 0x00, 0x00, 0x00} + 0x43, 0xf9, 0x88, 0xac, 0x00, 0x00, 0x00, 0x00} spendingTx, err := btcutil.NewTxFromBytes(spendingTxBytes) if err != nil { diff --git a/btcutil/tx.go b/btcutil/tx.go index c66f81bd..f47a82e9 100644 --- a/btcutil/tx.go +++ b/btcutil/tx.go @@ -6,6 +6,7 @@ package btcutil import ( "bytes" + "fmt" "io" "github.com/btcsuite/btcd/chainhash/v2" @@ -173,7 +174,16 @@ func (t *Tx) setBytes(bytes []byte) { // serialized bytes. See Tx. func NewTxFromBytes(serializedTx []byte) (*Tx, error) { br := bytes.NewReader(serializedTx) - return NewTxFromReader(br) + tx, err := NewTxFromReader(br) + if err != nil { + return nil, err + } + if br.Len() > 0 { + return nil, fmt.Errorf("transaction has %d trailing bytes", + br.Len()) + } + + return tx, nil } // NewTxFromReader returns a new instance of a bitcoin transaction given a diff --git a/btcutil/tx_test.go b/btcutil/tx_test.go index 52447aed..1a051bcf 100644 --- a/btcutil/tx_test.go +++ b/btcutil/tx_test.go @@ -76,6 +76,22 @@ func TestNewTxFromBytes(t *testing.T) { } } +// TestNewTxFromBytesRejectsTrailingData verifies that NewTxFromBytes rejects +// bytes after the serialized transaction. +func TestNewTxFromBytesRejectsTrailingData(t *testing.T) { + testTx := Block100000.Transactions[0] + var testTxBuf bytes.Buffer + err := testTx.Serialize(&testTxBuf) + if err != nil { + t.Errorf("Serialize: %v", err) + } + + _, err = btcutil.NewTxFromBytes(append(testTxBuf.Bytes(), 0x00)) + if err == nil { + t.Fatal("expected error for transaction with trailing data") + } +} + // TestTxErrors tests the error paths for the Tx API. func TestTxErrors(t *testing.T) { // Serialize the test transaction. diff --git a/go.mod b/go.mod index 5a5b9fd6..577d70a8 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,11 @@ require ( gopkg.in/yaml.v3 v3.0.1 // indirect ) +replace ( + github.com/btcsuite/btcd/btcutil/v2 => ./btcutil + github.com/btcsuite/btcd/wire/v2 => ./wire +) + // The retract statements below fixes an accidental push of the tags of a btcd // fork. retract ( diff --git a/psbt/partial_input.go b/psbt/partial_input.go index 2e784be3..8ac5d7d3 100644 --- a/psbt/partial_input.go +++ b/psbt/partial_input.go @@ -90,9 +90,7 @@ func (pi *PInput) deserialize(r io.Reader) error { if keyData != nil { return ErrInvalidKeyData } - tx := wire.NewMsgTx(2) - - err := tx.Deserialize(bytes.NewReader(value)) + tx, err := readTransaction(value, false) if err != nil { return err } diff --git a/psbt/psbt.go b/psbt/psbt.go index 264f671a..d8e39f8c 100644 --- a/psbt/psbt.go +++ b/psbt/psbt.go @@ -188,12 +188,12 @@ func NewFromUnsignedTx(tx *wire.MsgTx) (*Packet, error) { // NOTE: To create a Packet from one's own data, rather than reading in a // serialization from a counterparty, one should use a psbt.New. func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { - // If the PSBT is encoded in bas64, then we'll create a new wrapper - // reader that'll allow us to incrementally decode the contents of the - // io.Reader. if b64 { - based64EncodedReader := r - r = base64.NewDecoder(base64.StdEncoding, based64EncodedReader) + decoded, err := decodeBase64Strict(r) + if err != nil { + return nil, err + } + r = bytes.NewReader(decoded) } // The Packet struct does not store the fixed magic bytes, but they @@ -225,11 +225,9 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { if err != nil { return nil, err } - msgTx := wire.NewMsgTx(2) - // BIP-0174 states: "The transaction must be in the old serialization // format (without witnesses)." - err = msgTx.DeserializeNoWitness(bytes.NewReader(value)) + msgTx, err := readTransaction(value, true) if err != nil { return nil, err } @@ -326,9 +324,36 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { return nil, err } + if err := assertFullyConsumed(r); err != nil { + return nil, err + } + return &newPsbt, nil } +// decodeBase64Strict decodes an RFC4648 base64 stream without permitting +// whitespace and with '=' allowed only as final padding. +func decodeBase64Strict(r io.Reader) ([]byte, error) { + encoded, err := io.ReadAll(r) + if err != nil { + return nil, err + } + + // Go's strict base64 decoder still ignores CR/LF. Reject them before + // decoding so base64 PSBT parsing matches the RFC4648 alphabet exactly. + if bytes.ContainsAny(encoded, "\r\n") { + return nil, ErrInvalidPsbtFormat + } + + decoded := make([]byte, base64.StdEncoding.DecodedLen(len(encoded))) + n, err := base64.StdEncoding.Strict().Decode(decoded, encoded) + if err != nil { + return nil, ErrInvalidPsbtFormat + } + + return decoded[:n], nil +} + // Serialize creates a binary serialization of the referenced Packet struct // with lexicographical ordering (by key) of the subsections. func (p *Packet) Serialize(w io.Writer) error { diff --git a/psbt/strict_tx_values_test.go b/psbt/strict_tx_values_test.go new file mode 100644 index 00000000..83ed6bdd --- /dev/null +++ b/psbt/strict_tx_values_test.go @@ -0,0 +1,268 @@ +package psbt + +import ( + "bytes" + "encoding/base64" + "testing" + + "github.com/btcsuite/btcd/wire/v2" + "github.com/stretchr/testify/require" +) + +// strictnessTxPair returns a minimal unsigned transaction and the previous +// transaction provided as its non-witness UTXO. +func strictnessTxPair(t *testing.T) (*wire.MsgTx, *wire.MsgTx) { + t.Helper() + + prevTx := wire.NewMsgTx(2) + prevTx.AddTxIn(&wire.TxIn{ + PreviousOutPoint: wire.OutPoint{Index: wire.MaxPrevOutIndex}, + Sequence: wire.MaxTxInSequenceNum, + }) + prevTx.AddTxOut(&wire.TxOut{ + Value: 12345, + PkScript: []byte{0x51}, + }) + + unsignedTx := wire.NewMsgTx(2) + unsignedTx.AddTxIn(&wire.TxIn{ + PreviousOutPoint: wire.OutPoint{ + Hash: prevTx.TxHash(), + Index: 0, + }, + Sequence: wire.MaxTxInSequenceNum, + }) + unsignedTx.AddTxOut(&wire.TxOut{ + Value: 1000, + PkScript: []byte{0x51}, + }) + + return unsignedTx, prevTx +} + +// serializeTxForStrictness serializes tx in the PSBT form required by the +// field under test. +func serializeTxForStrictness(t *testing.T, tx *wire.MsgTx, + noWitness bool) []byte { + + t.Helper() + + var buf bytes.Buffer + var err error + if noWitness { + err = tx.SerializeNoWitness(&buf) + } else { + err = tx.Serialize(&buf) + } + require.NoError(t, err) + + return buf.Bytes() +} + +// strictnessPSBT builds a minimal PSBT using the supplied transaction-valued +// fields verbatim. +func strictnessPSBT(t *testing.T, unsignedTx, + nonWitnessUtxo []byte) []byte { + + t.Helper() + + var buf bytes.Buffer + _, err := buf.Write(psbtMagic[:]) + require.NoError(t, err) + + require.NoError(t, serializeKVPairWithType( + &buf, byte(UnsignedTxType), nil, unsignedTx, + )) + require.NoError(t, buf.WriteByte(0x00)) + + require.NoError(t, serializeKVPairWithType( + &buf, byte(NonWitnessUtxoType), nil, nonWitnessUtxo, + )) + require.NoError(t, buf.WriteByte(0x00)) + require.NoError(t, buf.WriteByte(0x00)) + + return buf.Bytes() +} + +// serializeTxOutForStrictness serializes txOut in the PSBT form required by +// the WitnessUtxo field. +func serializeTxOutForStrictness(t *testing.T, txOut *wire.TxOut) []byte { + t.Helper() + + var buf bytes.Buffer + require.NoError(t, wire.WriteTxOut(&buf, 0, 0, txOut)) + + return buf.Bytes() +} + +// strictnessPSBTWithWitnessUtxo builds a minimal PSBT using the supplied +// WitnessUtxo value verbatim. +func strictnessPSBTWithWitnessUtxo(t *testing.T, + witnessUtxo []byte) []byte { + + t.Helper() + + unsignedTx, _ := strictnessTxPair(t) + + var buf bytes.Buffer + _, err := buf.Write(psbtMagic[:]) + require.NoError(t, err) + + require.NoError(t, serializeKVPairWithType( + &buf, byte(UnsignedTxType), nil, + serializeTxForStrictness(t, unsignedTx, true), + )) + require.NoError(t, buf.WriteByte(0x00)) + + require.NoError(t, serializeKVPairWithType( + &buf, byte(WitnessUtxoType), nil, witnessUtxo, + )) + require.NoError(t, buf.WriteByte(0x00)) + require.NoError(t, buf.WriteByte(0x00)) + + return buf.Bytes() +} + +// TestRejectsTrailingDataInTransactionValues verifies that PSBT transaction +// values must contain exactly one serialized transaction. +func TestRejectsTrailingDataInTransactionValues(t *testing.T) { + unsignedTx, prevTx := strictnessTxPair(t) + unsignedTxBytes := serializeTxForStrictness(t, unsignedTx, true) + prevTxBytes := serializeTxForStrictness(t, prevTx, false) + + testCases := []struct { + name string + unsignedTx []byte + nonWitnessUtxo []byte + }{{ + name: "global unsigned tx", + unsignedTx: append(unsignedTxBytes, 0x00), + nonWitnessUtxo: prevTxBytes, + }, { + name: "input non-witness utxo", + unsignedTx: unsignedTxBytes, + nonWitnessUtxo: append(prevTxBytes, 0x00), + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + _, err := NewFromRawBytes(bytes.NewReader( + strictnessPSBT( + t, tc.unsignedTx, tc.nonWitnessUtxo, + ), + ), false) + require.ErrorIs(t, err, ErrInvalidPsbtFormat) + }) + } +} + +// TestRejectsTrailingDataAfterPacket verifies that extra bytes after a valid +// PSBT packet are rejected. +func TestRejectsTrailingDataAfterPacket(t *testing.T) { + unsignedTx, prevTx := strictnessTxPair(t) + rawPacket := strictnessPSBT( + t, + serializeTxForStrictness(t, unsignedTx, true), + serializeTxForStrictness(t, prevTx, false), + ) + + _, err := NewFromRawBytes( + bytes.NewReader(append(rawPacket, 0x00)), false, + ) + require.ErrorIs(t, err, ErrInvalidPsbtFormat) +} + +// TestRejectsNonCanonicalBase64Packet verifies that base64 PSBT input rejects +// whitespace, bad padding, and extra decoded packet bytes. +func TestRejectsNonCanonicalBase64Packet(t *testing.T) { + unsignedTx, prevTx := strictnessTxPair(t) + rawPacket := strictnessPSBT( + t, + serializeTxForStrictness(t, unsignedTx, true), + serializeTxForStrictness(t, prevTx, false), + ) + encoded := base64.StdEncoding.EncodeToString(rawPacket) + insert := func(idx int, s string) string { + return encoded[:idx] + s + encoded[idx:] + } + + testCases := []struct { + name string + encoded string + }{{ + name: "trailing LF", + encoded: encoded + "\n", + }, { + name: "trailing CRLF", + encoded: encoded + "\r\n", + }, { + name: "LF between groups", + encoded: insert(4, "\n"), + }, { + name: "LF inside group", + encoded: insert(5, "\n"), + }, { + name: "space between groups", + encoded: insert(4, " "), + }, { + name: "tab inside group", + encoded: insert(5, "\t"), + }, { + name: "padding in middle", + encoded: insert(len(encoded)-4, "="), + }, { + name: "extra decoded bytes", + encoded: base64.StdEncoding.EncodeToString( + append(append([]byte{}, rawPacket...), 0x00), + ), + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + _, err := NewFromRawBytes( + bytes.NewReader([]byte(tc.encoded)), true, + ) + require.ErrorIs(t, err, ErrInvalidPsbtFormat) + }) + } +} + +// TestParsesWitnessUtxoTxOutStrictly verifies that WitnessUtxo values are +// parsed as exact transaction outputs. +func TestParsesWitnessUtxoTxOutStrictly(t *testing.T) { + pkScript := bytes.Repeat([]byte{0x51}, 253) + txOutBytes := serializeTxOutForStrictness(t, &wire.TxOut{ + Value: 1234, + PkScript: pkScript, + }) + + packet, err := NewFromRawBytes(bytes.NewReader( + strictnessPSBTWithWitnessUtxo(t, txOutBytes), + ), false) + require.NoError(t, err) + require.Equal(t, pkScript, packet.Inputs[0].WitnessUtxo.PkScript) + + malformedTxOut := append(append([]byte{}, txOutBytes...), 0x00) + _, err = NewFromRawBytes(bytes.NewReader( + strictnessPSBTWithWitnessUtxo(t, malformedTxOut), + ), false) + require.ErrorIs(t, err, ErrInvalidPsbtFormat) +} + +// TestParsesWitnessUtxoTxOutCompactSizeScriptLength verifies that WitnessUtxo +// scripts with multi-byte CompactSize lengths are parsed without folding the +// length bytes into the script. +func TestParsesWitnessUtxoTxOutCompactSizeScriptLength(t *testing.T) { + pkScript := bytes.Repeat([]byte{0x51}, 300) + txOutBytes := serializeTxOutForStrictness(t, &wire.TxOut{ + Value: 1234, + PkScript: pkScript, + }) + require.Equal(t, byte(0xfd), txOutBytes[8]) + + packet, err := NewFromRawBytes(bytes.NewReader( + strictnessPSBTWithWitnessUtxo(t, txOutBytes), + ), false) + require.NoError(t, err) + require.Equal(t, pkScript, packet.Inputs[0].WitnessUtxo.PkScript) +} diff --git a/psbt/utils.go b/psbt/utils.go index 2c880e2b..baf75583 100644 --- a/psbt/utils.go +++ b/psbt/utils.go @@ -6,7 +6,6 @@ package psbt import ( "bytes" - "encoding/binary" "errors" "fmt" "io" @@ -278,17 +277,69 @@ func getKey(r io.Reader) (int, []byte, error) { return int(keyType), keyData, nil } -// readTxOut is a limited version of wire.ReadTxOut, because the latter is not -// exported. -func readTxOut(txout []byte) (*wire.TxOut, error) { - if len(txout) < 10 { - return nil, ErrInvalidPsbtFormat +// assertFullyConsumed returns ErrInvalidPsbtFormat if r still has bytes +// available after parsing. +func assertFullyConsumed(r io.Reader) error { + if lr, ok := r.(interface{ Len() int }); ok { + if lr.Len() > 0 { + return ErrInvalidPsbtFormat + } + + return nil } - valueSer := binary.LittleEndian.Uint64(txout[:8]) - scriptPubKey := txout[9:] + var trailing [1]byte + _, err := io.ReadFull(r, trailing[:]) + switch { + case err == nil: + return ErrInvalidPsbtFormat - return wire.NewTxOut(int64(valueSer), scriptPubKey), nil + case errors.Is(err, io.EOF): + return nil + + default: + return err + } +} + +// readTxOut parses a transaction output value and requires the full value to +// be consumed. +func readTxOut(txout []byte) (*wire.TxOut, error) { + txOut := &wire.TxOut{} + reader := bytes.NewReader(txout) + + if err := wire.ReadTxOut(reader, 0, 0, txOut); err != nil { + return nil, err + } + if err := assertFullyConsumed(reader); err != nil { + return nil, err + } + + return txOut, nil +} + +// readTransaction parses a transaction value and requires the full value to be +// consumed. PSBT transaction-valued fields contain exactly one network +// serialized transaction, not a transaction prefix with arbitrary trailing data. +func readTransaction(txBytes []byte, noWitness bool) (*wire.MsgTx, error) { + tx := wire.NewMsgTx(2) + reader := bytes.NewReader(txBytes) + + var err error + if noWitness { + err = tx.DeserializeNoWitness(reader) + } else { + err = tx.Deserialize(reader) + } + if err != nil { + return nil, err + } + + if err := assertFullyConsumed(reader); err != nil { + return nil, err + } + + return tx, nil } // SumUtxoInputValues tries to extract the sum of all inputs specified in the diff --git a/rpcserver.go b/rpcserver.go index 3a481aa4..fb5f0665 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -791,22 +791,22 @@ func handleDecodeRawTransaction(s *rpcServer, cmd interface{}, closeChan <-chan if err != nil { return nil, rpcDecodeHexError(hexStr) } - var mtx wire.MsgTx - err = mtx.Deserialize(bytes.NewReader(serializedTx)) + tx, err := btcutil.NewTxFromBytes(serializedTx) if err != nil { return nil, &btcjson.RPCError{ Code: btcjson.ErrRPCDeserialization, Message: "TX decode failed: " + err.Error(), } } + mtx := tx.MsgTx() // Create and return the result. txReply := btcjson.TxRawDecodeResult{ Txid: mtx.TxHash().String(), Version: mtx.Version, Locktime: mtx.LockTime, - Vin: createVinList(&mtx), - Vout: createVoutList(&mtx, s.cfg.ChainParams, nil), + Vin: createVinList(mtx), + Vout: createVoutList(mtx, s.cfg.ChainParams, nil), } return txReply, nil } @@ -2141,14 +2141,13 @@ func handleGetBlockTemplateProposal(s *rpcServer, request *btcjson.TemplateReque "hexadecimal string (not %q)", hexData), } } - var msgBlock wire.MsgBlock - if err := msgBlock.Deserialize(bytes.NewReader(dataBytes)); err != nil { + block, err := btcutil.NewBlockFromBytes(dataBytes) + if err != nil { return nil, &btcjson.RPCError{ Code: btcjson.ErrRPCDeserialization, Message: "Block decode failed: " + err.Error(), } } - block := btcutil.NewBlock(&msgBlock) // Ensure the block is building from the expected previous block. expectedPrevHash := s.cfg.Chain.BestSnapshot().Hash @@ -3441,8 +3440,7 @@ func handleSendRawTransaction(s *rpcServer, cmd interface{}, closeChan <-chan st if err != nil { return nil, rpcDecodeHexError(hexStr) } - var msgTx wire.MsgTx - err = msgTx.Deserialize(bytes.NewReader(serializedTx)) + tx, err := btcutil.NewTxFromBytes(serializedTx) if err != nil { return nil, &btcjson.RPCError{ Code: btcjson.ErrRPCDeserialization, @@ -3451,7 +3449,6 @@ func handleSendRawTransaction(s *rpcServer, cmd interface{}, closeChan <-chan st } // Use 0 for the tag to represent local node. - tx := btcutil.NewTx(&msgTx) acceptedTxs, err := s.cfg.TxMemPool.ProcessTransaction(tx, false, false, 0) if err != nil { // When the error is a rule error, it means the transaction was diff --git a/rpcserver_test.go b/rpcserver_test.go index 2e291da5..e4625642 100644 --- a/rpcserver_test.go +++ b/rpcserver_test.go @@ -1,15 +1,18 @@ package main import ( + "bytes" "encoding/hex" "errors" "testing" "github.com/btcsuite/btcd/btcjson" "github.com/btcsuite/btcd/btcutil/v2" + "github.com/btcsuite/btcd/chaincfg/v2" "github.com/btcsuite/btcd/chainhash/v2" "github.com/btcsuite/btcd/mempool" "github.com/btcsuite/btcd/wire/v2" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -67,6 +70,119 @@ func TestHandleTestMempoolAcceptFailDecode(t *testing.T) { } } +// TestHandleTestMempoolAcceptRejectsTrailingBytes ensures testmempoolaccept +// rejects byte strings that contain a valid transaction plus trailing data. +func TestHandleTestMempoolAcceptRejectsTrailingBytes(t *testing.T) { + t.Parallel() + + defer func() { + recovered := recover() + require.Nil(t, recovered, "handler reached mempool") + }() + + cmd := btcjson.NewTestMempoolAcceptCmd([]string{txHex1 + "00"}, 0) + result, err := handleTestMempoolAccept( + &rpcServer{}, cmd, make(chan struct{}), + ) + + requireRPCErrorCode(t, err, btcjson.ErrRPCDeserialization) + require.Nil(t, result) +} + +// requireRPCErrorCode asserts that the error is an RPC error with the expected +// error code. +func requireRPCErrorCode(t *testing.T, err error, code btcjson.RPCErrorCode) { + t.Helper() + + require.Error(t, err) + rpcErr, ok := err.(*btcjson.RPCError) + require.True(t, ok) + require.Equal(t, code, rpcErr.Code) +} + +// blockHexWithTrailingByte serializes a valid block and appends one extra byte. +func blockHexWithTrailingByte(t *testing.T) string { + t.Helper() + + var block bytes.Buffer + err := chaincfg.MainNetParams.GenesisBlock.Serialize(&block) + require.NoError(t, err) + + return hex.EncodeToString(append(block.Bytes(), 0x00)) +} + +// TestHandleSendRawTransactionRejectsTrailingBytes ensures sendrawtransaction +// rejects byte strings that contain a valid transaction plus trailing data. +func TestHandleSendRawTransactionRejectsTrailingBytes(t *testing.T) { + t.Parallel() + + mm := &mempool.MockTxMempool{} + mm.On( + "ProcessTransaction", mock.Anything, false, false, mempool.Tag(0), + ).Return(nil, errors.New("mempool should not be reached")).Maybe() + + s := &rpcServer{cfg: rpcserverConfig{ + TxMemPool: mm, + }} + cmd := btcjson.NewSendRawTransactionCmd(txHex1+"00", nil) + + result, err := handleSendRawTransaction(s, cmd, make(chan struct{})) + requireRPCErrorCode(t, err, btcjson.ErrRPCDeserialization) + require.Nil(t, result) +} + +// TestHandleDecodeRawTransactionRejectsTrailingBytes ensures +// decoderawtransaction rejects byte strings that contain a valid transaction +// plus trailing data. +func TestHandleDecodeRawTransactionRejectsTrailingBytes(t *testing.T) { + t.Parallel() + + cmd := btcjson.NewDecodeRawTransactionCmd(txHex1 + "00") + result, err := handleDecodeRawTransaction( + &rpcServer{}, cmd, make(chan struct{}), + ) + + requireRPCErrorCode(t, err, btcjson.ErrRPCDeserialization) + require.Nil(t, result) +} + +// TestHandleGetBlockTemplateProposalRejectsTrailingBytes ensures proposal mode +// rejects byte strings that contain a valid block plus trailing data. +func TestHandleGetBlockTemplateProposalRejectsTrailingBytes(t *testing.T) { + t.Parallel() + + defer func() { + recovered := recover() + require.Nil(t, recovered, "handler reached chain state") + }() + + request := &btcjson.TemplateRequest{ + Mode: "proposal", + Data: blockHexWithTrailingByte(t), + } + + result, err := handleGetBlockTemplateProposal(&rpcServer{}, request) + requireRPCErrorCode(t, err, btcjson.ErrRPCDeserialization) + require.Nil(t, result) +} + +// TestHandleSubmitBlockRejectsTrailingBytes ensures submitblock rejects byte +// strings that contain a valid block plus trailing data. +func TestHandleSubmitBlockRejectsTrailingBytes(t *testing.T) { + t.Parallel() + + defer func() { + recovered := recover() + require.Nil(t, recovered, "handler reached sync manager") + }() + + cmd := btcjson.NewSubmitBlockCmd(blockHexWithTrailingByte(t), nil) + result, err := handleSubmitBlock(&rpcServer{}, cmd, make(chan struct{})) + + requireRPCErrorCode(t, err, btcjson.ErrRPCDeserialization) + require.Nil(t, result) +} + var ( // TODO(yy): make a `btctest` package and move these testing txns there // so they be used in other tests. diff --git a/wire/message.go b/wire/message.go index 73bae42d..40edb966 100644 --- a/wire/message.go +++ b/wire/message.go @@ -570,6 +570,12 @@ func ReadV2MessageN(plaintext []byte, pver uint32, enc MessageEncoding) ( return nil, nil, err } + if buf.Len() > 0 { + str := fmt.Sprintf("message payload has %d extra bytes "+ + "after decode", buf.Len()) + return nil, nil, messageError("ReadV2MessageN", str) + } + return msg, plaintext, nil } diff --git a/wire/message_test.go b/wire/message_test.go index 278d8c24..dba4ac35 100644 --- a/wire/message_test.go +++ b/wire/message_test.go @@ -347,7 +347,6 @@ func TestReadMessageWireErrors(t *testing.T) { ErrUnknownMessage, 24, }, - } t.Logf("Running %d tests", len(tests)) @@ -428,6 +427,28 @@ func TestReadMessageTrailingBytes(t *testing.T) { } } +// TestReadV2MessageTrailingBytes verifies that a v2 message with unconsumed +// trailing bytes after BtcDecode is rejected with a MessageError. +func TestReadV2MessageTrailingBytes(t *testing.T) { + payload := []byte{ + v2Messages[CmdInv], + 0x00, // zero inventory vectors + 0xaa, // trailing data not consumed by MsgInv.BtcDecode + } + + _, _, err := ReadV2MessageN( + payload, ProtocolVersion, BaseEncoding, + ) + if err == nil { + t.Fatal("expected error for v2 message with trailing bytes") + } + + var msgErr *MessageError + if !errors.As(err, &msgErr) { + t.Fatalf("expected MessageError, got: %T (%v)", err, err) + } +} + // TestWriteMessageWireErrors performs negative tests against wire encoding from // concrete messages to confirm error paths work correctly. func TestWriteMessageWireErrors(t *testing.T) {