Skip to content

psbt: reject unsigned tx with trailing bytes#2541

Closed
Lrifton92 wants to merge 6 commits into
btcsuite:masterfrom
Lrifton92:fix/psbt-trailing-bytes
Closed

psbt: reject unsigned tx with trailing bytes#2541
Lrifton92 wants to merge 6 commits into
btcsuite:masterfrom
Lrifton92:fix/psbt-trailing-bytes

Conversation

@Lrifton92

@Lrifton92 Lrifton92 commented Jun 9, 2026

Copy link
Copy Markdown

Summary

While parsing the global unsigned transaction of a PSBT, btcd reads it from a reader over the value bytes but never verifies that the transaction consumed the entire value. A value whose length prefix (<valuelen>) declares more bytes than the transaction actually occupies is silently accepted.

This diverges from Bitcoin Core, which enforces that the deserialized data exactly matches the stated length in UnserializeFromVector:

if (remaining_after + expected_size != remaining_before) {
    throw std::ios_base::failure("Size of value was not the stated size");
}

This was originally surfaced by differential fuzzing between Bitcoin Core and btcd (#2424): btcd parsed a PSBT that Core rejected.

Fix

After decoding the unsigned transaction, assert that the value reader has no bytes left; otherwise return ErrInvalidPsbtFormat. The ldflags-free path is unchanged — well-formed PSBTs are unaffected. This also avoids the wasted allocations and trie lookups a malicious peer could trigger with oversized values.

-	err = msgTx.DeserializeNoWitness(bytes.NewReader(value))
-	if err != nil {
+	txReader := bytes.NewReader(value)
+	err = msgTx.DeserializeNoWitness(txReader)
+	if err != nil {
 		return nil, err
 	}
+	if txReader.Len() != 0 {
+		return nil, ErrInvalidPsbtFormat
+	}

Testing

Added TestNewFromRawBytesTrailingTxBytes, which:

  1. Builds a minimal valid PSBT (single input/output) and confirms it parses cleanly (so the error is attributable to the trailing byte alone).
  2. Injects one trailing byte into the global unsigned-tx value and grows the length prefix to cover it.
  3. Asserts NewFromRawBytes now returns ErrInvalidPsbtFormat.
$ go test ./psbt/
ok  	github.com/btcsuite/btcd/psbt/v2

gofmt, go vet ./psbt/... clean; full psbt package suite passes. Tested on go1.26.4.

Closes #2424

@Lrifton92

Lrifton92 commented Jun 16, 2026

Copy link
Copy Markdown
Author

Friendly ping — this is ready for review whenever a maintainer has bandwidth. Small fix: reject unsigned PSBT transactions with trailing bytes. Thanks!

@Lrifton92 Lrifton92 mentioned this pull request Jun 20, 2026
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Jun 25, 2026
Merged
@starius

starius commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for fixing this!

I found more instances of the same issue in PSBT code: https://gist.github.com/starius/0b74bc1f0842947e577ed20642d65f90

Could you integrate them into your PR, please? Ideally if you follow this commit structure for each bugfix: (1) fix commit, (2) regression test commit. The later commit should compile if the former commit is reverted, and the test should fail with a symptomatic error.

@starius starius mentioned this pull request Jun 25, 2026
Open
@Lrifton92 Lrifton92 force-pushed the fix/psbt-trailing-bytes branch from efbaf57 to cb9ccaa Compare June 25, 2026 19:28
@Lrifton92

Lrifton92 commented Jun 25, 2026

Copy link
Copy Markdown
Author

Thanks @starius — done. I integrated your full series and force-pushed the branch.

The PR now covers all the instances from your gist, each as a (1) fix + (2) regression test pair, in this order:

  1. psbt: reject trailing data in tx values — adds a readTransaction helper (full-consumption check via reader.Len() > 0) and routes both the global unsigned tx (NewFromRawBytes) and the input non-witness UTXO (PInput.deserialize) through it.
  2. psbt: add strict tx value regression test — TestRejectsTrailingDataInTransactionValues (global unsigned tx + input non-witness utxo cases).
  3. psbt: reject trailing packet data — rejects extra bytes after a valid packet in NewFromRawBytes.
  4. psbt: test trailing packet data rejection — TestRejectsTrailingDataAfterPacket.
  5. psbt: parse witness utxo txouts strictly — readTxOut now uses wire.ReadTxOut and rejects leftover bytes, replacing the manual length-assuming parse.
  6. psbt: test witness utxo txout strict parsing — TestParsesWitnessUtxoTxOutStrictly.

I kept the authorship on your commits since they're yours. Each test commit compiles if its preceding fix commit is reverted, and I verified that reverting each fix makes exactly its test fail with a symptomatic ErrInvalidPsbtFormat mismatch (the rest of the suite stays green). go build, go vet and the full psbt suite all pass locally.

Let me know if you'd like any wording or structure adjusted.

@starius starius mentioned this pull request Jun 26, 2026
Merged
7 tasks
@starius

starius commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Replacing this PR with #2558
I found another PSBT parsing issue - see Note 2 there.

guggero
guggero reviewed Jun 26, 2026
View reviewed changes

@guggero guggero left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion for an optimization/generalization, otherwise looks good to me. Thanks a lot for the fixes!

Comment thread psbt/psbt.go
return nil, err
}

var trailing [1]byte

@guggero guggero Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can introduce a helper function in the first commit and use it everywhere:

// assertFullyConsumed returns ErrInvalidPsbtFormat if the given reader is _not_
// fully exhausted, meaning there are bytes left to consume.
func assertFullyConsumed(r io.Reader) error {
	// Short-cut if it's already a bytes.Reader, which has a Len() function.
	if lr, ok := r.(interface{Len() int}); ok {
		if lr.Len() > 0 {
			return ErrInvalidPsbtFormat
		}

		return nil
	}

	// We have a generic reader, let's try reading a single byte. If that
	// succeeds, it means the reader wasn't fully exhausted/consumed.
	var trailing [1]byte
	_, err := io.ReadFull(r, trailing[:])
	switch {
	// Reading was a success, wich means the reader wasn't fully consumed.
	case err == nil:
		return ErrInvalidPsbtFormat

	// We couldn't read, which is what we wanted to assert.
	case errors.Is(err, io.EOF):
		return nil

	// Something unexpected happened.
	default:
		return err
	}
}

@starius starius Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! I addressed it here: #2558

@Roasbeef

Roasbeef commented Jul 1, 2026

Copy link
Copy Markdown
Member

Folded into #2558

@Roasbeef Roasbeef closed this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guggero guggero guggero left review comments
+1 more reviewer
@starius starius starius left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

psbt: Missing validation that deserialization consumes exact number of bytes specified in length prefix

4 participants

@Lrifton92 @starius @Roasbeef @guggero