wallet: don't abort on crafted MuSig2 PSBT inputs#35154
Conversation
Attacker-controlled PSBT fields (PSBT_IN_TAP_BIP32_DERIVATION path and PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS) flow through a trivially-satisfied 4-byte fingerprint check into CPubKey::Derive() and a fatal Assert(). Two crash vectors exist: - A hardened derivation index (bit 31 set) in the path hits assert((nChild >> 31) == 0) in CPubKey::Derive (pubkey.cpp). - A mismatched (but unhardened) path derives to the wrong pubkey, hitting Assert(XOnlyPubKey(extpub.pubkey) == script_pubkey) in SignMuSig2 (sign.cpp). Both are reachable from any PSBT-processing RPC (finalizepsbt, analyzepsbt, descriptorprocesspsbt) without a wallet. Fix: reject hardened indices before entering the derivation loop, and replace the fatal Assert with a graceful continue that skips the mismatched aggregate pubkey.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
The release is already out, and this would not be a blocker for it. Stop calling RPC crashes vulnerabilities. |
| continue; | ||
| } | ||
| // Reject hardened derivation indices (public key derivation only supports unhardened) | ||
| if (std::any_of(agg_info.path.begin(), agg_info.path.end(), [](uint32_t idx) { return idx >> 31; })) { |
There was a problem hiding this comment.
Instead of looping through the path again, we can do this check below prior to derivation.
It would be nice if a PSBT co-signer service doesn't need to restart their node any time a PSBT from an untrusted source does something unexpected. Perhaps we can add fuzz coverage to enforce that |
|
Thanks, but LLM output is not accepted if the author can not properly explain, test or could have written the change themselves. The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on. Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR. I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output. Please reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested. I'll close this for now. |
|
Nowhere in the PR, other than the vestigial branch name, did we claim that this is a security vulnerability. As was stated in the PR, the bug was discovered by Anthropic, but was subsequently manually patched and validated by engineers at Trail of Bits. Would you rather us create a GitHub issue to track the bug? |
|
I've manually converted the two inputs to the fuzz input format and uploaded this to OSS-Fuzz for fun: |
Summary
SignMuSig2()insrc/script/sign.cppderives an attacker-supplied aggregate pubkey along an attacker-supplied BIP32 path (both from untrusted PSBT fieldsPSBT_IN_TAP_BIP32_DERIVATIONandPSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS) and thenAssert()s the result matches the script pubkey. A mismatched derivation or a hardened index abortsbitcoindviafinalizepsbt,analyzepsbt, ordescriptorprocesspsbt, with no wallet loaded.Two crash vectors:
assert((nChild >> 31) == 0)inCPubKey::Derive(pubkey.cpp).Assert(XOnlyPubKey(extpub.pubkey) == script_pubkey)atsign.cpp:313.Assert()usesstd::abort()in all builds (NDEBUGis#error'd incheck.h), so this terminates the process.Impact
finalizepsbt,analyzepsbt, anddescriptorprocesspsbtall reach the vulnerable code viaDUMMY_SIGNING_PROVIDER.analyzepsbtmakes even a read-only inspection a crash trigger.v31.0rc1,v31.0rc2, andmaster. Not present in any GA release. Introduced in commit4a273edda0(PR wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675). This is release-blocking forv31.0GA.Reproduction
On
master(2d5ab09f0dat the time of testing):Both PSBTs pass
decodepsbtcleanly and would not look suspicious in a MuSig2 / coinjoin workflow.bitcoindaborts on each call;bitcoin-clireports "EOF reached".analyzepsbtwith the same PSBTs also aborts.Fix
Two small changes in
SignMuSig2()atsrc/script/sign.cpp:CPubKey::Derive. Public-key derivation only supports non-hardened steps; a hardened index is attacker-controlled input that can never produce a valid derivation, so skipping the aggregate is correct.Assert(XOnlyPubKey(extpub.pubkey) == script_pubkey)withif (...) continue;. A pubkey mismatch means this aggregate is not the one that producedscript_pubkey. The correct behavior is to move on to the next candidate, not to abort the process.The diff is 6 added / 1 removed lines.
Credit
Found by Claude during an automated review conducted by Anthropic;
manually validated and patched by Trail of Bits. Reference:
ANT-2026-05771.