Fix assert failure in miniscript string parsing #26149

1. 
   sipa commented at 1:21 PM on September 21, 2022: member

   Fix a bug in the script_size sanity-check in the miniscript string parser, found by oss-fuzz in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51636, and introduced in e8cc2e4afc1142aa2b3da19cd5c17deea9963244 (#25540).

   This bug would cause an assertion failure when feeding a miniscript with a thresh(k,...) fragment, with k >= 128, to an RPC.

2. Correct sanity-checking script_size calculation 648f6950cd
3. fanquake added this to the milestone 24.0 on Sep 21, 2022
4. fanquake requested review from darosior on Sep 21, 2022
5. 
   sipa commented at 1:40 PM on September 21, 2022: member

   @darosior FWIW, my thinking with this code was incorrectly assuming that thresh's k couldn't be larger than 20, like multi(). That's obviously false.

   The 0x7fff and 0x7fffff conditions can't actually be hit (they'd immediately cause the script size.to exceed the limit) but I've included them here for consistency.

6. 
   darosior commented at 2:10 PM on September 21, 2022: member

   utACK 648f6950cd8d9ac767d76a1e302f37c611936a7a

   I checked it's the only place we missed it.

7. in src/script/miniscript.h:1224 in 648f6950cd

   1220 | @@ -1221,7 +1221,7 @@ inline NodeRef<Key> Parse(Span<const char> in, const Ctx& ctx)
   1221 |                  // n = 1 here because we read the first WRAPPED_EXPR before reaching THRESH
   1222 |                  to_parse.emplace_back(ParseContext::THRESH, 1, k);
   1223 |                  to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1);
   1224 | -                script_size += 2 + (k > 16);
   1225 | +                script_size += 2 + (k > 16) + (k > 0x7f) + (k > 0x7fff) + (k > 0x7fffff);
   

   ---

   ---

   

   amovfx commented at 3:35 PM on September 21, 2022:

   Reviewed and built locally. Checks all pass. This hex notation is new to me. I read that The 0x7FFF notation is much more clear about potential over/underflow than the decimal notation.

   Is this why it is used? A more safe representation of an int? Also, this might not be the right place to ask. Should I push these questions to stack exchange?

   ---

   

   sipa commented at 3:37 PM on September 21, 2022:

   I use the hex notation here because it's much more obvious. People might not recognize 32767 as the largest 16-bit signed integer, or 16777215 as the largest 24-bit signed integer; but in the hex notation this is immediately clear.

   It's just more readable to reviewers.

8. 
   achow101 commented at 4:50 PM on September 21, 2022: member

   ACK 648f6950cd8d9ac767d76a1e302f37c611936a7a

9. achow101 merged this on Sep 21, 2022
10. achow101 closed this on Sep 21, 2022

    ---

11. fanquake added the label Needs backport (24.x) on Sep 21, 2022
12. fanquake referenced this in commit 9dfccb40ac on Sep 22, 2022
13. 
    fanquake commented at 10:42 AM on September 22, 2022: member

    Added to #26133 for backporting to 24.x.

14. fanquake removed the label Needs backport (24.x) on Sep 22, 2022
15. sidhujag referenced this in commit 8d81a841f5 on Sep 23, 2022
16. fanquake referenced this in commit bcfd86a2bd on Sep 26, 2022
17. fanquake referenced this in commit be4338106b on Sep 29, 2022
18. fanquake referenced this in commit c97d924880 on Oct 11, 2022
19. achow101 referenced this in commit 885366c67a on Oct 13, 2022
20. bitcoin locked this on Oct 13, 2023

Contributors

sipadarosioramovfxachow101fanquake

Review Requested
darosior

Milestone
24.0