diff --git a/psbt/psbt.go b/psbt/psbt.go index d8e39f8c..5a3f7783 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 83ed6bdd..9122aa2d 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 baf75583..289e596c 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)