fuzz: rule-out too deep derivation paths in descriptor parsing targets #28832

1. 
   darosior commented at 2:08 pm on November 9, 2023: member

   This fixes the mocked_descriptor_parse timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
2. 
   DrahtBot commented at 2:08 pm on November 9, 2023: 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	sipa, dergoegge, TheCharlatan, achow101

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

3. DrahtBot added the label Tests on Nov 9, 2023
4. in src/test/fuzz/descriptor_parse.cpp:116 in 77af2c8b8c outdated

   107@@ -108,6 +108,25 @@ class MockedDescriptorConverter {
   108 //! The converter of mocked descriptors, needs to be initialized when the target is.
   109 MockedDescriptorConverter MOCKED_DESC_CONVERTER;
   110 
   111+//! Maximum number of derivation indexes for an extended key's derivation path in a descriptor.
   112+static int MAX_DEPTH{2};
   113+
   114+/** Whether the buffer, if it represents a valid descriptor, contains a derivation path which depth
   115+ * is larger than the maximum authorized. Note this may also be hit for deriv paths in origins. */
   116+static bool HasDeepDerivPath(const FuzzBufferType& buff)
   

   ---

   ---

   

   brunoerg commented at 2:09 pm on November 15, 2023:

   In 77af2c8b8c68f3e971229bd43a7ca6ffe3d0bbe8: Correct me if I’m wrong, but wouldn’t HasDeepDerivPath avoid descriptors like pkh([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)? Is this intended?

   ---

   

   darosior commented at 10:28 am on November 27, 2023:

   Yes:

   Note this may also be hit for deriv paths in origins.

   ---

   

   brunoerg commented at 11:56 am on November 27, 2023:

   Thanks, didn’t notice that.
5. 
   maflcko commented at 10:15 am on November 27, 2023: member

   Another (related) one?

   clusterfuzz-testcase-miniscript_string-6556534783737856.bin.not.txt

   

6. 
   dergoegge commented at 2:16 pm on December 6, 2023: member

   Could you add the exception for the scriptpubkeyman harness as well?

   https://github.com/bitcoin/bitcoin/blob/d85491404352c8b20bd7c317c129dca0763aead2/src/wallet/test/fuzz/scriptpubkeyman.cpp#L54-L56

7. 
   darosior commented at 2:19 pm on December 31, 2023: member

   Another (related) one?

   clusterfuzz-testcase-miniscript_string-6556534783737856.bin.not.txt

   

   This one seems unrelated to the derivation paths parsing, which is part of the descriptor logic not Miniscript. Looks like it’s spending most of the time in miniscript::operator"" _mst, so it might be fixed by #28657?

8. darosior force-pushed on Dec 31, 2023
9. 
   darosior commented at 3:17 pm on December 31, 2023: member

   Could you add the exception for the scriptpubkeyman harness as well?

   Done by moving the introduced HasDeepDerivPath into src/test/fuzz/util/descriptor.h and also calling it in scriptpubkeyman target’s CreateWalletDescriptor right before parsing the mocked descriptor string.

10. fuzz: rule-out too deep derivation paths in descriptor parsing targets

    This fixes the reported timeouts and direct the target cycles toward what it's intended to fuzz: the descriptor syntax.

    a44808fb43
11. darosior force-pushed on Dec 31, 2023
12. 
    sipa commented at 6:15 pm on January 2, 2024: member

    utACK a44808fb437864878c2d9696b8a96193091446ee
13. dergoegge approved
14. 
    dergoegge commented at 3:42 pm on January 3, 2024: member

    ACK a44808fb437864878c2d9696b8a96193091446ee - Not running into timeouts anymore

    https://dergoegge.github.io/bitcoin-coverage/pr28832/fuzz.coverage/index.html

15. TheCharlatan approved
16. 
    TheCharlatan commented at 10:50 am on January 4, 2024: contributor

    ACK a44808fb437864878c2d9696b8a96193091446ee
17. 
    achow101 commented at 11:04 pm on January 4, 2024: member

    ACK a44808fb437864878c2d9696b8a96193091446ee
18. achow101 merged this on Jan 4, 2024
19. achow101 closed this on Jan 4, 2024

    ---

20. darosior deleted the branch on Nov 8, 2024

Contributors
darosior DrahtBot brunoerg maflcko dergoegge sipa TheCharlatan achow101

Labels
Tests