diff --git a/blockchain/chainio.go b/blockchain/chainio.go index 9ce58b12..be5c9a20 100644 --- a/blockchain/chainio.go +++ b/blockchain/chainio.go @@ -1168,6 +1168,35 @@ func (b *BlockChain) createChainState() error { return err } +// 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.Debugf("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 +} + // initChainState attempts to load and initialize the chain state from the // database. When the db does not yet contain any chain state, both it and the // chain state are initialized to the genesis block. @@ -1277,7 +1306,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 +1330,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, @@ -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 272bf61d..7fb961b4 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) } - if !strings.Contains(err.Error(), "trailing bytes") { - t.Fatalf("expected trailing byte error, 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()) + } + 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 8487b18a..59f95408 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 d8e39f8c..4f61858a 100644 --- a/psbt/psbt.go +++ b/psbt/psbt.go @@ -185,15 +185,19 @@ 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. For raw input, trailing data is only +// detected when the reader can report its remaining length without blocking +// (such as bytes.Reader); a plain raw stream is not probed past the packet, so +// the reader is left positioned directly after it. Base64 input is decoded +// incrementally and read through its canonical end. +// // 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 b64 { - decoded, err := decodeBase64Strict(r) - if err != nil { - return nil, err - } - r = bytes.NewReader(decoded) + r = newStrictBase64Decoder(r) } // The Packet struct does not store the fixed magic bytes, but they @@ -324,34 +328,91 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { return nil, err } - if err := assertFullyConsumed(r); err != nil { - return nil, err + if b64 { + if err := assertBase64FullyConsumed(r); err != nil { + return nil, err + } + } else { + // Reject any trailing data after the packet when a raw reader can + // report it without an additional read. Plain raw 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 } -// 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 +// canonicalBase64Reader rejects the CR and LF bytes that encoding/base64 +// otherwise ignores while decoding. +type canonicalBase64Reader struct { + io.Reader +} + +// Read returns encoded bytes only when they use the canonical RFC4648 +// alphabet. +func (r *canonicalBase64Reader) Read(p []byte) (int, error) { + n, err := r.Reader.Read(p) + if bytes.ContainsAny(p[:n], "\r\n") { + return 0, ErrInvalidPsbtFormat } - // 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 + return n, err +} + +// strictBase64Decoder maps base64 syntax and truncation errors to +// ErrInvalidPsbtFormat while preserving other errors returned by the +// caller-supplied reader. +type strictBase64Decoder struct { + io.Reader +} + +// Read returns incrementally decoded bytes. +func (d *strictBase64Decoder) Read(p []byte) (int, error) { + n, err := d.Reader.Read(p) + if err == nil || errors.Is(err, io.EOF) { + return n, err } - decoded := make([]byte, base64.StdEncoding.DecodedLen(len(encoded))) - n, err := base64.StdEncoding.Strict().Decode(decoded, encoded) - if err != nil { - return nil, ErrInvalidPsbtFormat + var corruptInput base64.CorruptInputError + if errors.As(err, &corruptInput) || + errors.Is(err, io.ErrUnexpectedEOF) || + errors.Is(err, ErrInvalidPsbtFormat) { + + return n, ErrInvalidPsbtFormat } - return decoded[:n], nil + return n, err +} + +// newStrictBase64Decoder returns a streaming RFC4648 base64 decoder that +// rejects whitespace and only accepts '=' as final padding. +func newStrictBase64Decoder(r io.Reader) io.Reader { + canonicalReader := &canonicalBase64Reader{Reader: r} + decoder := base64.NewDecoder( + base64.StdEncoding.Strict(), canonicalReader, + ) + + return &strictBase64Decoder{Reader: decoder} +} + +// assertBase64FullyConsumed verifies the end of the base64 envelope and +// rejects decoded data after the PSBT packet. +func assertBase64FullyConsumed(r io.Reader) error { + var trailing [1]byte + _, err := io.ReadFull(r, trailing[:]) + switch { + case err == nil: + return ErrInvalidPsbtFormat + + case errors.Is(err, io.EOF): + return nil + + default: + return err + } } // 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 e31b8be2..c68e5469 100644 --- a/psbt/strict_tx_values_test.go +++ b/psbt/strict_tx_values_test.go @@ -3,6 +3,7 @@ package psbt import ( "bytes" "encoding/base64" + "io" "testing" "github.com/btcsuite/btcd/wire/v2" @@ -172,6 +173,86 @@ 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) +} + +// TestAcceptsBase64PacketAboveWirePayloadLimit verifies that base64 parsing +// accepts valid PSBT packets larger than the peer-to-peer wire message payload +// limit. +func TestAcceptsBase64PacketAboveWirePayloadLimit(t *testing.T) { + unsignedTx, _ := strictnessTxPair(t) + packet, err := NewFromUnsignedTx(unsignedTx) + require.NoError(t, err) + + value := bytes.Repeat([]byte{0x01}, MaxPsbtValueLength) + for i := range 9 { + packet.Unknowns = append(packet.Unknowns, &Unknown{ + Key: []byte{0xfc, byte(i)}, + Value: value, + }) + } + + var rawPacket bytes.Buffer + require.NoError(t, packet.Serialize(&rawPacket)) + require.Greater(t, rawPacket.Len(), wire.MaxMessagePayload) + + rawParsed, err := NewFromRawBytes( + bytes.NewReader(rawPacket.Bytes()), false, + ) + require.NoError(t, err) + require.Len(t, rawParsed.Unknowns, 9) + + encodedReader, encodedWriter := io.Pipe() + encodeDone := make(chan error, 1) + go func() { + encoder := base64.NewEncoder( + base64.StdEncoding, encodedWriter, + ) + _, encodeErr := io.Copy(encoder, bytes.NewReader( + rawPacket.Bytes(), + )) + if closeErr := encoder.Close(); encodeErr == nil { + encodeErr = closeErr + } + _ = encodedWriter.CloseWithError(encodeErr) + encodeDone <- encodeErr + }() + + parsedPacket, parseErr := NewFromRawBytes(encodedReader, true) + _ = encodedReader.Close() + encodeErr := <-encodeDone + if parseErr == nil { + require.NoError(t, encodeErr) + } + require.NoError(t, parseErr) + require.Len(t, parsedPacket.Unknowns, 9) +} + // 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 758ffd2f..4e01641d 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 @@ -325,9 +310,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 fb5f0665..dcc97fc9 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1082,9 +1082,17 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i } var blkBytes []byte err = s.cfg.DB.View(func(dbTx database.Tx) error { - var err error - blkBytes, err = dbTx.FetchBlock(hash) - return err + dbBlockBytes, err := dbTx.FetchBlock(hash) + if err != nil { + return err + } + + // FetchBlock bytes are only valid for the lifetime of the + // transaction. Copy them before returning from the view so the + // lenient parser below owns its backing memory. + blkBytes = append([]byte(nil), dbBlockBytes...) + + return nil }) if err != nil { return nil, &btcjson.RPCError{ @@ -1092,6 +1100,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 +1122,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 { diff --git a/rpcserver_test.go b/rpcserver_test.go index e4625642..d3e9da06 100644 --- a/rpcserver_test.go +++ b/rpcserver_test.go @@ -10,6 +10,7 @@ import ( "github.com/btcsuite/btcd/btcutil/v2" "github.com/btcsuite/btcd/chaincfg/v2" "github.com/btcsuite/btcd/chainhash/v2" + "github.com/btcsuite/btcd/database" "github.com/btcsuite/btcd/mempool" "github.com/btcsuite/btcd/wire/v2" "github.com/stretchr/testify/mock" @@ -111,6 +112,60 @@ func blockHexWithTrailingByte(t *testing.T) string { return hex.EncodeToString(append(block.Bytes(), 0x00)) } +// invalidatingBlockDB clears fetched block bytes as soon as its managed view +// ends. This models database backends whose zero-copy buffers are only valid +// for the lifetime of a transaction. +type invalidatingBlockDB struct { + database.DB + blockBytes []byte +} + +// View runs the callback with a transaction backed by the configured block +// bytes, then invalidates those bytes before returning. +func (d *invalidatingBlockDB) View(fn func(database.Tx) error) error { + err := fn(&invalidatingBlockTx{blockBytes: d.blockBytes}) + clear(d.blockBytes) + + return err +} + +// invalidatingBlockTx returns the parent database's transaction-scoped block +// bytes. +type invalidatingBlockTx struct { + database.Tx + blockBytes []byte +} + +// FetchBlock returns bytes that are invalidated when the enclosing view ends. +func (t *invalidatingBlockTx) FetchBlock(*chainhash.Hash) ([]byte, error) { + return t.blockBytes, nil +} + +// TestHandleGetBlockCopiesTransactionBytes verifies getblock does not retain +// transaction-scoped database memory after its managed view ends. +func TestHandleGetBlockCopiesTransactionBytes(t *testing.T) { + t.Parallel() + + var serializedBlock bytes.Buffer + err := chaincfg.MainNetParams.GenesisBlock.Serialize(&serializedBlock) + require.NoError(t, err) + + wantBytes := serializedBlock.Bytes() + dbBytes := append([]byte(nil), wantBytes...) + dbBytes = append(dbBytes, 0x00) + db := &invalidatingBlockDB{blockBytes: dbBytes} + + verbosity := 0 + cmd := btcjson.NewGetBlockCmd( + chaincfg.MainNetParams.GenesisHash.String(), &verbosity, + ) + result, err := handleGetBlock( + &rpcServer{cfg: rpcserverConfig{DB: db}}, cmd, make(chan struct{}), + ) + require.NoError(t, err) + require.Equal(t, hex.EncodeToString(wantBytes), result) +} + // TestHandleSendRawTransactionRejectsTrailingBytes ensures sendrawtransaction // rejects byte strings that contain a valid transaction plus trailing data. func TestHandleSendRawTransactionRejectsTrailingBytes(t *testing.T) {