Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
achow101 merged 1 commit into from
Jan 4, 2024
Merged

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

achow101 merged 1 commit into from
Jan 4, 2024

Conversation

@darosior
Copy link
Member

@darosior darosior commented Nov 9, 2023

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 9, 2023 •
edited
Loading

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.

@DrahtBot DrahtBot added the Tests label Nov 9, 2023
@dergoegge dergoegge mentioned this pull request Nov 13, 2023
Merged
brunoerg
brunoerg reviewed Nov 15, 2023
View reviewed changes
src/test/fuzz/descriptor_parse.cpp Outdated
Copy link
Contributor

@brunoerg brunoerg Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@darosior darosior Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes:

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

Copy link
Contributor

@brunoerg brunoerg Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, didn't notice that.

@maflcko
Copy link
Member

maflcko commented Nov 27, 2023

Another (related) one?

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

Screenshot from 2023-11-27 11-14-38

@dergoegge
Copy link
Member

dergoegge commented Dec 6, 2023

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

bitcoin/src/wallet/test/fuzz/scriptpubkeyman.cpp

Lines 54 to 56 in d854914

const std::string mocked_descriptor{fuzzed_data_provider.ConsumeRandomLengthString()};
const auto desc_str{MOCKED_DESC_CONVERTER.GetDescriptor(mocked_descriptor)};
if (!desc_str.has_value()) return std::nullopt;

@darosior
Copy link
Member Author

darosior commented Dec 31, 2023

Another (related) one?

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

Screenshot from 2023-11-27 11-14-38

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?

@darosior darosior force-pushed the 2311_fuzz_timeout_desc_parse branch from 77af2c8 to f5942c5 Compare December 31, 2023 15:14
@darosior
Copy link
Member Author

darosior commented Dec 31, 2023

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.

@darosior darosior mentioned this pull request Dec 31, 2023
Closed
@darosior
fuzz: rule-out too deep derivation paths in descriptor parsing targets
a44808f 
This fixes the reported timeouts and direct the target cycles toward what it's intended to fuzz: the descriptor syntax.
@darosior darosior force-pushed the 2311_fuzz_timeout_desc_parse branch from f5942c5 to a44808f Compare December 31, 2023 15:20
@maflcko maflcko mentioned this pull request Jan 2, 2024
Merged
@sipa
Copy link
Member

sipa commented Jan 2, 2024

utACK a44808f

dergoegge
dergoegge approved these changes Jan 3, 2024
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a44808f - Not running into timeouts anymore

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

sedited
sedited approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a44808f

@achow101
Copy link
Member

achow101 commented Jan 4, 2024

ACK a44808f

@achow101 achow101 merged commit d445545 into bitcoin:master Jan 4, 2024
@darosior darosior deleted the 2311_fuzz_timeout_desc_parse branch November 8, 2024 21:15
@bitcoin bitcoin locked and limited conversation to collaborators Nov 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

+2 more reviewers

@brunoerg brunoerg brunoerg left review comments

@dergoegge dergoegge dergoegge approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@darosior @DrahtBot @maflcko @dergoegge @sipa @achow101 @sedited @brunoerg