multi: reject trailing characters when parsing inputs#2558
Conversation
Lrifton92
left a comment
Lrifton92
left a comment
There was a problem hiding this comment.
Went through the full series (27 commits) @starius — happy to see all the trailing-byte cases consolidated in one place, and thanks for extending it well beyond the original three (wire v2 payloads, musig2 partial sigs, btcutil byte constructors, block proposals, DB block loading).
I built the root module + every touched submodule and ran their suites locally (go1.26.4):
psbt,wire,btcutil(+bloom),btcec/schnorr/musig2,blockchain, and root.(rpcserver) — all green.
I also validated the "revert the fix, keep the test → red" property on a few representative pairs, since that's the whole point of splitting each fix from its regression test:
- rpc: restoring
rpcserver.goto pre-59db835→TestHandleSendRawTransactionRejectsTrailingBytesandTestHandleDecodeRawTransactionRejectsTrailingBytesfail. ✅ - btcutil: restoring
tx.go/block.goto pre-41d537d→TestNewTxFromBytesRejectsTrailingDataandTestNewBlockFromBytesRejectsTrailingDatafail ("expected error for … with trailing data"). ✅ - psbt: restoring
psbt.goto pre-70e8ceb→TestRejectsNonCanonicalBase64Packetfails (rawillegal base64 datainstead ofErrInvalidPsbtFormat). ✅
One heads-up so it doesn't get blamed on this PR: netsync TestSyncStateMachine (handleHeadersMsg should update lastProgressTime) fails for me locally — but it fails identically on a clean master (280d4d2) and this PR doesn't touch netsync, so it looks like a pre-existing timing flake, unrelated here.
LGTM. This correctly supersedes #2541 / #2550 / #2556 — glad to close those in favor of this consolidated version whenever you'd like.
| // exported. | ||
| // assertFullyConsumed returns ErrInvalidPsbtFormat if r still has bytes | ||
| // available after parsing. | ||
| func assertFullyConsumed(r io.Reader) error { |
There was a problem hiding this comment.
👍
Was originally thinking we could just do it unilaterally at the wire level, but this is better as we can't assume too much about the context the reader/writer impls are used within.
|
|
||
| replace ( | ||
| github.com/btcsuite/btcd/btcutil/v2 => ./btcutil | ||
| github.com/btcsuite/btcd/wire/v2 => ./wire |
There was a problem hiding this comment.
Reminder to push a tag and then make a PR to update after.
| txOut := &wire.TxOut{} | ||
| reader := bytes.NewReader(txout) | ||
|
|
||
| if err := wire.ReadTxOut(reader, 0, 0, txOut); err != nil { |
There was a problem hiding this comment.
This introduces a memory-retention regression for parsed WitnessUtxo values. wire.ReadTxOut returns TxOut.PkScript as a slice into a 4 MiB wire.scriptSlab; psbt/partial_input.go then stores the returned TxOut directly as pi.WitnessUtxo, so each witness input can retain the whole slab even when the script is tiny.
Suggested fix: compact/copy the script after wire.ReadTxOut before returning it from readTxOut:
script := make([]byte, len(txOut.PkScript))
copy(script, txOut.PkScript)
txOut.PkScript = scriptA regression test can assert that parsed witness scripts do not retain the slab, e.g. cap(packet.Inputs[0].WitnessUtxo.PkScript) == len(packet.Inputs[0].WitnessUtxo.PkScript).
Fixed in #2564 |
|
Went back through the merged series once more — no concerns on correctness. Two non-blocking follow-ups worth tracking:
Also nice bonus catch on the |
4 participants
Change Description
Some functions accept
[]byteinputs with trailing characters. Many such cases were fixed and covered with regression tests.This PR replaces there PRs:
Fixes #2424
Fixes #2195
Note 1. In commit "multi: use local submodules in root" I replaced
btcutil/v2andwire/v2with local versions to make the fixed changes available inrpc: *commits.Note 2. PSBT parsing accepted
\nand\r\nin the end and in the middle, if base64 parsing is enabled. This is not aligned with BIP 174/RFC4648 and Bitcoin Core, which reject such inputs. I added this validation in a separate commit. Now base64 PSBT string must have only base64 commot charset +=characters in the end, but no whitespace, new line or tabs.Steps to Test
Run the tests. Each fix has a regression test which can be tried with the fix commit reverted. A regression test compiles, but fails with a symptomatic error.
Pull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.