psbt: detect invalid MuSig2 pubkeys in deserialization #34010

1. 
   rkrux commented at 6:49 am on December 5, 2025: contributor

   Throw error while deserializing PSBT if invalid pubkeys are passed as a MuSig2 aggregate or participant.

   Should fix #33999 by throwing error at the very start while decoding an invalid PSBT that should subsequently not allow the MuSig2 signing operation to take place, thereby avoiding the crash.

2. DrahtBot added the label Wallet on Dec 5, 2025
3. 
   DrahtBot commented at 6:49 am on December 5, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34010.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

4. 
   theStack commented at 2:00 pm on December 5, 2025: contributor

   The line leading to the crash involves the following two constructors https://github.com/bitcoin/bitcoin/blob/b8e66b901d563d7ba6042e6be54d7e48b9fffc83/src/pubkey.h#L256-L260 where cryptographic validity of the pubkey isn’t checked at any point, so I don’t think this PR fixes anything?
5. 
   rkrux commented at 2:08 pm on December 5, 2025: contributor

   Yes, the second constructor appears to be involved. This comment here suggests where the error lies because of an empty aggregate pubkey added in the fuzz input: #29675 (review)

   I believe that skipping the loop iteration before the following line hits should help in fixing IIUC? https://github.com/bitcoin/bitcoin/blob/b8e66b901d563d7ba6042e6be54d7e48b9fffc83/src/script/sign.cpp#L299

6. 
   theStack commented at 2:34 pm on December 5, 2025: contributor

   Oh right, the crash already happens in the std::span{pubkey}.subspan(1, 32) expression (if std::span{pubkey} is empty due to being invalid), before the first constructor is called. It seems we should detect invalid participant pubkeys as early as possible and not allow them to be added to the wallet state in the first place, I suspect a better place to fix this would be DeserializeMuSig2ParticipantPubkeys, where a validity check is currently missing.
7. 
   rkrux commented at 3:34 pm on December 5, 2025: contributor

   I did think about adding it there but decided against it to keep the change related to the crash site. But I don’t feel strongly about it and will update the PR to add the check in PSBT deserialization.
8. fanquake requested review from achow101 on Dec 5, 2025
9. 
   fjahr commented at 11:55 pm on December 5, 2025: contributor

   Could you add a simple test for this?
10. psbt: detect invalid MuSig2 pubkeys in deserialization

    Throw error while deserializing PSBT if invalid pubkeys are passed
    as a MuSig2 aggregate or participant.

    1818f7028e
11. rkrux force-pushed on Dec 8, 2025
12. rkrux renamed this:
    wallet: check for `agg_pub` validity in MuSig2 signing
    psbt: detect invalid MuSig2 pubkeys in deserialization
    on Dec 8, 2025
13. 
    rkrux commented at 2:32 pm on December 8, 2025: contributor

    I suspect a better place to fix this would be DeserializeMuSig2ParticipantPubkeys, where a validity check is currently missing.

    Updated PR to add the check there instead of in the MuSig2 signing flow that occurs later.

    Could you add a simple test for this?

    Added few tests from the fuzz inputs and created one more test so that the test coverage of an existing if condition that appears at the end of DeserializeMuSig2ParticipantPubkeys is not lost due to the newly added check for invalid participant pubkey.

Contributors
rkrux DrahtBot theStack fjahr

Review Requested
achow101

Labels
Wallet