sign: don’t assume we are parsing a sane TapMiniscript #29853

1. 
   darosior commented at 2:11 pm on April 11, 2024: member

   The script provided for signature might be externally provided, for instance by way of ‘finalizepsbt’. Therefore the script might be ill-crafted, so don’t assume pubkeys are always 32 bytes.

   Thanks to Niklas for finding this.

   FIxes #29851.

2. 
   DrahtBot commented at 2:11 pm on April 11, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, furszy
   Stale ACK	dergoegge

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. fanquake added the label Needs backport (27.x) on Apr 11, 2024
4. darosior referenced this in commit be9802b237 on Apr 11, 2024
5. dergoegge approved
6. 
   dergoegge commented at 9:05 am on April 12, 2024: member

   utACK bdf2ef2c94cae2b0e2cdac1866322b4f9f7c7a7b
7. achow101 requested review from achow101 on Apr 12, 2024
8. in src/test/script_tests.cpp:1278 in bdf2ef2c94 outdated

   1273+    prev.vout.emplace_back(0, GetScriptForDestination(builder.GetOutput()));
   1274+    curr.vin.emplace_back(COutPoint{prev.GetHash(), 0});
   1275+    sig_data.tr_spenddata = builder.GetSpendData();
   1276+
   1277+    // SignSignature can fail but it shouldn't raise an exception (nor crash).
   1278+    SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data);
   

   ---

   ---

   

   achow101 commented at 5:40 pm on April 15, 2024:

   I think it would be good to check that SignSignature fails here:

   0    BOOST_CHECK(!SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data));
   

   ---

   

   darosior commented at 4:24 pm on April 22, 2024:

   I think it emphases the wrong thing: we are not testing it’s failing, we are testing it’s not crashing. That’s why i didn’t do it. Anyways it’s not really important and there is already a comment highlighting this. Done.
9. sign: don't assume we are parsing a sane Miniscript

   The script provided for signature might be externally provided, for
   instance by way of 'finalizepsbt'. Therefore the script might be
   ill-crafted, so don't assume pubkeys are always 32 bytes.
   
   Thanks to Niklas for finding this.

   4d8d21320e
10. darosior force-pushed on Apr 22, 2024
11. DrahtBot added the label CI failed on Apr 22, 2024
12. DrahtBot removed the label CI failed on Apr 23, 2024
13. 
    achow101 commented at 7:41 pm on April 23, 2024: member

    ACK 4d8d21320eba54571ff63931509cd515c3e20339
14. DrahtBot requested review from dergoegge on Apr 23, 2024
15. 
    furszy commented at 8:56 pm on April 23, 2024: member

    ACK 4d8d21320eba54571ff63931509cd515c3e20339 with a small nuance that could be tackled in a follow-up by someone else (or never).

    As this was found by the RPC fuzzer, it would be nice to re-use those inputs and craft a functional test, rather than covering it on a unit test. This is because functional tests can be ported across releases with the bug fix, while unit tests might not be easy to port due to some internal changes. Plus, in the long term, unit tests require more maintenance work.

16. fanquake merged this on Apr 24, 2024
17. fanquake closed this on Apr 24, 2024

    ---

18. fanquake referenced this in commit ae9a2ed40a on Apr 24, 2024
19. fanquake removed the label Needs backport (27.x) on Apr 24, 2024
20. 
    fanquake commented at 1:18 pm on April 24, 2024: member

    Backported to 27.x in #29888.
21. luke-jr referenced this in commit 8a8dd1c444 on Apr 24, 2024
22. fanquake referenced this in commit c7885ecd77 on May 13, 2024
23. glozow referenced this in commit fe98e9c8de on May 13, 2024
24. 
    glozow commented at 9:43 am on May 14, 2024: member

    backported to 26.x in #29899
25. darosior deleted the branch on May 14, 2024

Contributors
darosior DrahtBot dergoegge achow101 furszy fanquake glozow

Review Requested
dergoegge