psbt: avoid blocking reads and unbounded allocations in NewFromRawBytes
What the developer wrote
Authored by Olaoluwa Osuntokun
psbt: avoid blocking reads and unbounded allocations in NewFromRawBytes
In this commit, we address two issues with the strict parsing recently
added to NewFromRawBytes.
First, the trailing data check probed the caller supplied reader with a
blocking one byte read. A reader without a Len method (net.Conn,
io.Pipe) that stays open after delivering a complete packet would hang
the parser forever. We now only enforce the check when the reader can
report its remaining length without an additional read, which covers
in-memory readers along with the decoded base64 path. Plain streams are
left positioned directly after the packet, and the reader contract is
now documented on NewFromRawBytes.
Second, the base64 path read the entire input into memory before any
validation ran, so a very large input could force an arbitrarily large
allocation before the first validity check. We now bound the read to
wire.MaxMessagePayload expanded by the base64 encoding overhead.
Along the way, we simplify assertFullyConsumed down to the bytes.Reader
case that all remaining callers use.
What changed, and why it matters
This commit fixes two problems in the way btcd reads Partially Signed Bitcoin Transactions (PSBTs). First, the parser could hang forever when reading from a network connection or pipe because it tried to read one extra byte to check for leftover data. Second, when given a base64-encoded PSBT, it would read the entire input into memory before checking whether it was valid, so a maliciously huge input could force the program to allocate enormous amounts of RAM. The patch limits how much base64 data is read and only checks for trailing bytes when the reader can safely report its remaining length.
Treat this as a security hardening fix and include it in the next release. Users parsing PSBTs from untrusted network streams or base64 inputs should upgrade. Review other io.Reader consumers in the codebase for similar blocking trailing-data checks or unbounded io.ReadAll calls.
Security signals we found
Denial-of-service via blocking read on open stream (potential infinite hang)
Denial-of-service via unbounded memory allocation on base64 input before validation
Strict parsing hardening for PSBT base64 decoding
Reader contract documented for NewFromRawBytes
Evidence from the diff
In psbt.NewFromRawBytes, the previous trailing-data check called assertFullyConsumed, which performed a blocking io.ReadFull of one byte for readers without a Len() method. That caused indefinite blocking on open streams such as net.Conn or io.Pipe after a complete packet had been delivered. The patch removes that blocking probe and only enforces the trailing-data check for readers implementing Len() (e.g., bytes.Reader and the decoded base64 path). Separately, decodeBase64Strict previously called io.ReadAll(r) before any validation, allowing unbounded allocation. It now uses io.LimitReader(r, maxBase64PsbtSize+1), where maxBase64PsbtSize is derived from wire.MaxMessagePayload with base64 overhead, and rejects inputs exceeding the bound with ErrInvalidPsbtFormat. assertFullyConsumed is narrowed to *bytes.Reader to match its remaining callers.
Changed components
btcd/psbt/psbt.gobtcd/psbt/utils.gobtcd/psbt/strict_tx_values_test.goInspect captured patch +87 / −28
diff --git a/psbt/psbt.go b/psbt/psbt.go
index d8e39f8..5a3f778 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 83ed6bd..9122aa2 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 baf7558..289e596 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)
Why this scored 64/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.