diff --git a/blockchain/chainio.go b/blockchain/chainio.go index 9ce58b1232..9d183a7238 100644 --- a/blockchain/chainio.go +++ b/blockchain/chainio.go @@ -1277,7 +1277,7 @@ func (b *BlockChain) initChainState() error { if err != nil { return err } - block, err := btcutil.NewBlockFromBytes(blockBytes) + block, err := DBBlockFromBytes(blockBytes, state.hash) if err != nil { return err } @@ -1301,8 +1301,15 @@ func (b *BlockChain) initChainState() error { } } - // Initialize the state related to the best block. - blockSize := uint64(len(blockBytes)) + // Initialize the state related to the best block. The block + // bytes are re-derived from the block itself so any trailing + // bytes ignored during deserialization are excluded from the + // recorded size. + serializedBlock, err := block.Bytes() + if err != nil { + return err + } + blockSize := uint64(len(serializedBlock)) blockWeight := uint64(GetBlockWeight(block)) numTxns := uint64(len(block.MsgBlock().Transactions)) b.stateSnapshot = newBestState(tip, blockSize, blockWeight, @@ -1367,6 +1374,35 @@ func dbFetchHeaderByHeight(dbTx database.Tx, height int32) (*wire.BlockHeader, e return dbFetchHeaderByHash(dbTx, hash) } +// DBBlockFromBytes deserializes a block fetched from the local database, +// tolerating trailing bytes rather than rejecting them outright. Databases +// written by older btcd versions may have persisted blocks with trailing +// bytes, and failing here would make such blocks permanently unreadable. +// Instead, any trailing bytes are logged, ignored, and excluded from the +// serialization cached in the returned block. +// +// This lenient parsing is only appropriate for blocks read back from the +// node's own database. Blocks from external sources (p2p, RPC) should be +// parsed with the strict btcutil.NewBlockFromBytes instead. +func DBBlockFromBytes(blockBytes []byte, hash chainhash.Hash) (*btcutil.Block, + error) { + + blockReader := bytes.NewReader(blockBytes) + var msgBlock wire.MsgBlock + if err := msgBlock.Deserialize(blockReader); err != nil { + return nil, err + } + if trailing := blockReader.Len(); trailing > 0 { + log.Warnf("Block %v has %d trailing bytes in the database; "+ + "ignoring them", hash, trailing) + blockBytes = blockBytes[:len(blockBytes)-trailing] + } + + // Cache the exact serialization on the block so downstream consumers + // of the raw bytes never observe the trailing bytes. + return btcutil.NewBlockFromBlockAndBytes(&msgBlock, blockBytes), nil +} + // dbFetchBlockByNode uses an existing database transaction to retrieve the // raw block for the provided node, deserialize it, and return a btcutil.Block // with the height set. @@ -1378,7 +1414,7 @@ func dbFetchBlockByNode(dbTx database.Tx, node *blockNode) (*btcutil.Block, erro } // Create the encapsulated block and set the height appropriately. - block, err := btcutil.NewBlockFromBytes(blockBytes) + block, err := DBBlockFromBytes(blockBytes, node.hash) if err != nil { return nil, err } diff --git a/blockchain/chainio_test.go b/blockchain/chainio_test.go index 272bf61d67..7fb961b49b 100644 --- a/blockchain/chainio_test.go +++ b/blockchain/chainio_test.go @@ -9,10 +9,10 @@ import ( "errors" "math/big" "reflect" - "strings" "testing" "github.com/btcsuite/btcd/btcutil/v2" + "github.com/btcsuite/btcd/chaincfg/v2" "github.com/btcsuite/btcd/database" "github.com/btcsuite/btcd/txscript/v2" "github.com/btcsuite/btcd/wire/v2" @@ -40,11 +40,13 @@ func TestErrNotInMainChain(t *testing.T) { } } -// TestInitChainStateRejectsTrailingBestBlockBytes ensures startup rejects a +// TestInitChainStateToleratesTrailingBestBlockBytes ensures startup loads a // stored best block whose bytes contain a valid block plus trailing data. -func TestInitChainStateRejectsTrailingBestBlockBytes(t *testing.T) { +// Databases written by older btcd versions may contain such blocks, so +// rejecting them would prevent the node from ever starting. +func TestInitChainStateToleratesTrailingBestBlockBytes(t *testing.T) { chain, params, teardown := utxoCacheTestChain( - "TestInitChainStateRejectsTrailingBestBlockBytes") + "TestInitChainStateToleratesTrailingBestBlockBytes") defer teardown() tip := btcutil.NewBlock(params.GenesisBlock) @@ -72,17 +74,83 @@ func TestInitChainStateRejectsTrailingBestBlockBytes(t *testing.T) { t.Fatalf("failed to process block: %v", err) } - _, err = New(&Config{ + restarted, 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 err != nil { + t.Fatalf("expected trailing best block bytes to be "+ + "tolerated at startup, got: %v", err) + } + + // The best state must reflect the stored block, with the trailing + // byte excluded from the recorded block size. + snapshot := restarted.BestSnapshot() + if snapshot.Hash != *block.Hash() { + t.Fatalf("unexpected best block hash - got %v, want %v", + snapshot.Hash, block.Hash()) } - if !strings.Contains(err.Error(), "trailing bytes") { - t.Fatalf("expected trailing byte error, got: %v", err) + wantSize := uint64(len(serialized.Bytes())) + if snapshot.BlockSize != wantSize { + t.Fatalf("unexpected best block size - got %d, want %d", + snapshot.BlockSize, wantSize) + } +} + +// TestDBBlockFromBytes ensures database block parsing strips trailing bytes +// from the cached serialization, passes exact serializations through +// untouched, and still rejects truncated blocks. +func TestDBBlockFromBytes(t *testing.T) { + t.Parallel() + + params := &chaincfg.MainNetParams + var serialized bytes.Buffer + err := params.GenesisBlock.Serialize(&serialized) + if err != nil { + t.Fatalf("failed to serialize block: %v", err) + } + cleanBytes := serialized.Bytes() + wantHash := params.GenesisBlock.BlockHash() + + // A block with trailing bytes must parse, and the cached + // serialization must exclude the trailing data. + block, err := DBBlockFromBytes( + append(append([]byte(nil), cleanBytes...), 0x00), wantHash, + ) + if err != nil { + t.Fatalf("failed to parse block with trailing bytes: %v", err) + } + gotBytes, err := block.Bytes() + if err != nil { + t.Fatalf("failed to serialize parsed block: %v", err) + } + if !bytes.Equal(gotBytes, cleanBytes) { + t.Fatal("cached serialization includes trailing bytes") + } + if *block.Hash() != wantHash { + t.Fatalf("unexpected block hash - got %v, want %v", + block.Hash(), wantHash) + } + + // An exact serialization must pass through untouched. + block, err = DBBlockFromBytes(cleanBytes, wantHash) + if err != nil { + t.Fatalf("failed to parse exact block: %v", err) + } + gotBytes, err = block.Bytes() + if err != nil { + t.Fatalf("failed to serialize parsed block: %v", err) + } + if !bytes.Equal(gotBytes, cleanBytes) { + t.Fatal("exact serialization was not preserved") + } + + // A truncated block must still fail to parse. + _, err = DBBlockFromBytes(cleanBytes[:len(cleanBytes)-1], wantHash) + if err == nil { + t.Fatal("expected truncated block to fail to parse") } } diff --git a/blockchain/indexers/manager.go b/blockchain/indexers/manager.go index 28f608fbe1..533f007396 100644 --- a/blockchain/indexers/manager.go +++ b/blockchain/indexers/manager.go @@ -309,7 +309,9 @@ func (m *Manager) Init(chain *blockchain.BlockChain, interrupt <-chan struct{}) if err != nil { return err } - block, err = btcutil.NewBlockFromBytes(blockBytes) + block, err = blockchain.DBBlockFromBytes( + blockBytes, *hash, + ) if err != nil { return err } diff --git a/psbt/psbt.go b/psbt/psbt.go index d8e39f8ce5..5a3f77831f 100644 --- a/psbt/psbt.go +++ b/psbt/psbt.go @@ -185,6 +185,13 @@ func NewFromUnsignedTx(tx *wire.MsgTx) (*Packet, error) { // argument b64 is true, the passed byte slice is decoded from base64 encoding // before processing. // +// The parsing is strict: base64 input must not contain whitespace or any +// characters outside the RFC4648 standard alphabet, and any data after the +// packet results in ErrInvalidPsbtFormat. Trailing data is only detected +// when the reader can report its remaining length without blocking (such as +// bytes.Reader, or the base64 path); a plain stream is not probed past the +// packet, so the reader is left positioned directly after it. +// // 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) { @@ -324,20 +331,37 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { return nil, err } - if err := assertFullyConsumed(r); err != nil { - return nil, err + // Reject any trailing data after the packet when the reader is able + // to report it without an additional read. This covers in-memory + // readers as well as the decoded base64 path above. Plain streams + // are not probed, as a read for EOF could block forever on an open + // connection that has already delivered a complete packet. + if lr, ok := r.(interface{ Len() int }); ok && lr.Len() > 0 { + return nil, ErrInvalidPsbtFormat } return &newPsbt, nil } +// maxBase64PsbtSize is the maximum number of base64 characters accepted when +// decoding a PSBT. It is wire.MaxMessagePayload, the largest payload the +// wire protocol will carry, expanded by the 4/3 base64 encoding overhead. It +// bounds the memory allocated for a caller-supplied reader before any +// validation runs. +const maxBase64PsbtSize = 4 * ((wire.MaxMessagePayload + 2) / 3) + // 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) + // Bound the read so an unbounded stream cannot force an arbitrarily + // large allocation before validation. + encoded, err := io.ReadAll(io.LimitReader(r, maxBase64PsbtSize+1)) if err != nil { return nil, err } + if len(encoded) > maxBase64PsbtSize { + return nil, ErrInvalidPsbtFormat + } // Go's strict base64 decoder still ignores CR/LF. Reject them before // decoding so base64 PSBT parsing matches the RFC4648 alphabet exactly. @@ -345,13 +369,12 @@ func decodeBase64Strict(r io.Reader) ([]byte, error) { return nil, ErrInvalidPsbtFormat } - decoded := make([]byte, base64.StdEncoding.DecodedLen(len(encoded))) - n, err := base64.StdEncoding.Strict().Decode(decoded, encoded) + decoded, err := base64.StdEncoding.Strict().AppendDecode(nil, encoded) if err != nil { return nil, ErrInvalidPsbtFormat } - return decoded[:n], nil + return decoded, nil } // Serialize creates a binary serialization of the referenced Packet struct diff --git a/psbt/strict_tx_values_test.go b/psbt/strict_tx_values_test.go index 83ed6bdd20..9122aa2d90 100644 --- a/psbt/strict_tx_values_test.go +++ b/psbt/strict_tx_values_test.go @@ -3,7 +3,10 @@ package psbt import ( "bytes" "encoding/base64" + "errors" + "io" "testing" + "testing/iotest" "github.com/btcsuite/btcd/wire/v2" "github.com/stretchr/testify/require" @@ -172,6 +175,53 @@ func TestRejectsTrailingDataAfterPacket(t *testing.T) { require.ErrorIs(t, err, ErrInvalidPsbtFormat) } +// TestStreamReaderNotProbedPastPacket verifies that a reader that cannot +// report its remaining length is not read past the end of the packet: the +// packet parses successfully and any subsequent data remains unread, so +// parsing never blocks on an open stream. +func TestStreamReaderNotProbedPastPacket(t *testing.T) { + unsignedTx, prevTx := strictnessTxPair(t) + rawPacket := strictnessPSBT( + t, + serializeTxForStrictness(t, unsignedTx, true), + serializeTxForStrictness(t, prevTx, false), + ) + + // io.MultiReader hides the Len method of the underlying bytes.Reader, + // mimicking a plain stream. + stream := io.MultiReader(bytes.NewReader( + append(append([]byte{}, rawPacket...), 0xde, 0xad), + )) + + _, err := NewFromRawBytes(stream, false) + require.NoError(t, err) + + // The bytes following the packet must still be readable from the + // stream. + trailing, err := io.ReadAll(stream) + require.NoError(t, err) + require.Equal(t, []byte{0xde, 0xad}, trailing) +} + +// TestRejectsOversizedBase64Packet verifies that base64 input larger than +// the maximum accepted size is rejected instead of being fully decoded. +func TestRejectsOversizedBase64Packet(t *testing.T) { + oversized := bytes.Repeat([]byte{'A'}, maxBase64PsbtSize+1) + + // The erroring sentinel after the oversized bytes pins the bound + // itself: with the size limit in place the reader is never read past + // maxBase64PsbtSize+1 bytes, so the sentinel stays untouched. Without + // the limit, the full read would surface the sentinel error instead + // of ErrInvalidPsbtFormat. + stream := io.MultiReader( + bytes.NewReader(oversized), + iotest.ErrReader(errors.New("read past size bound")), + ) + + _, err := NewFromRawBytes(stream, true) + 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) { diff --git a/psbt/utils.go b/psbt/utils.go index baf7558304..289e596c23 100644 --- a/psbt/utils.go +++ b/psbt/utils.go @@ -279,27 +279,12 @@ func getKey(r io.Reader) (int, []byte, error) { // 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 - } - - var trailing [1]byte - _, err := io.ReadFull(r, trailing[:]) - switch { - case err == nil: +func assertFullyConsumed(r *bytes.Reader) error { + if r.Len() > 0 { return ErrInvalidPsbtFormat - - case errors.Is(err, io.EOF): - return nil - - default: - return err } + + return nil } // readTxOut parses a transaction output value and requires the full value to @@ -318,9 +303,10 @@ func readTxOut(txout []byte) (*wire.TxOut, error) { 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. +// 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) diff --git a/rpcserver.go b/rpcserver.go index fb5f0665e3..4182549d5f 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1092,6 +1092,21 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i Message: "Block not found", } } + // Deserialize the block. Blocks read back from the node's own + // database are parsed leniently: trailing bytes persisted by older + // btcd versions are stripped rather than treated as an error, so the + // bytes served below are always the exact block serialization. + blk, err := blockchain.DBBlockFromBytes(blkBytes, *hash) + if err != nil { + context := "Failed to deserialize block" + return nil, internalRPCError(err.Error(), context) + } + blkBytes, err = blk.Bytes() + if err != nil { + context := "Failed to serialize block" + return nil, internalRPCError(err.Error(), context) + } + // If verbosity is 0, return the serialized block as a hex encoded string. if c.Verbosity != nil && *c.Verbosity == 0 { return hex.EncodeToString(blkBytes), nil @@ -1099,13 +1114,6 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i // Otherwise, generate the JSON object and return it. - // Deserialize the block. - blk, err := btcutil.NewBlockFromBytes(blkBytes) - if err != nil { - context := "Failed to deserialize block" - return nil, internalRPCError(err.Error(), context) - } - // Get the block height from chain. blockHeight, err := s.cfg.Chain.BlockHeightByHash(hash) if err != nil {