Merge pull request #2558 from starius/trailing

multi: reject trailing characters when parsing inputs

Author: Roasbeef

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

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) {

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)

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()

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

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) {

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 {

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

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.

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 (