Skip to content

fix crash in Descriptor::parse_desc found by fuzzer #809

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.

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

Merged
merged 2 commits into from
Apr 28, 2025

Conversation

apoelstra
Copy link
Member

@apoelstra apoelstra commented Apr 21, 2025

Thanks to Bruno Garcia and i-am-yuvi who independently found this crash and reported it to me.

Fixes #806

Needs backport.

@apoelstra
Copy link
Member Author

cc @i-am-yuvi @brunoerg

When parsing a descriptor with `Descriptor::parse_descriptor`, we first parse
as a string and then parse the keys. We fail to consider parsing errors
in the keys, resulting in a panic.

Also, clean up the panic message so it's clearer what's going on.
@apoelstra
Copy link
Member Author

Bug originates in #493

Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

On 6bff186 successfully ran local tests

@brunoerg
Copy link
Contributor

code review ACK 6bff186; haven't tested

Copy link

@yuvicc yuvicc left a comment

Choose a reason for hiding this comment

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

Tested and Code Review ACK 6bff186

cargo test regression_806


Compiling miniscript v13.0.0 (...rust-miniscript)
Finished `test` profile [unoptimized + debuginfo] target(s) in 3.99s
Running unittests src/lib.rs (target/debug/deps/miniscript-0203e4fe382223fb)

running 1 test
test descriptor::tests::regression_806 ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 119 filtered out; finished in 0.00s

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 6bff186

Thanks for the reports, Bruno and i-am-yuvi

@apoelstra apoelstra merged commit 6a3f0f7 into rust-bitcoin:master Apr 28, 2025
31 checks passed
@apoelstra apoelstra deleted the 2025-04--fuzz-crash branch April 28, 2025 21:52
apoelstra added a commit that referenced this pull request Apr 29, 2025
212d78a bump patch version of 12.3 (Andrew Poelstra)
4c6366a add regression test for #806 (Andrew Poelstra)
4823d86 descriptor: fix key parsing error handling in parse_desc (Andrew Poelstra)

Pull request description:

  Backports #809 and does another patch release.

ACKs for top commit:
  sanket1729:
    reACK 212d78a

Tree-SHA512: c95651f29e1bcb4e8c8036b2a74443c3fd818d39c259852d5c35f0f5ef88fbbe18b312eb7e4d29c39a4af8e9660871fbe998d86c9d21a3d502f1d9854f6f7a43
apoelstra added a commit that referenced this pull request May 3, 2025
06400b1 bump patch version of 10.2 (Andrew Poelstra)
1fed0ec add regression test for #806 (Andrew Poelstra)
2f21e23 descriptor: fix key parsing error handling in parse_desc (Andrew Poelstra)
0e40319 ci: update CI job to run all the fuzz tests (Andrew Poelstra)
6ff58af lib: remove some deny lints (Andrew Poelstra)

Pull request description:

  Backports #809 to 10.x. This is a direct rebase of #810 except that I added a commit with some lint fixes. (I had been avoiding this, but we've been having a lot of backports lately so I think I ought to just address it.)

  After this I will do 11.x.

ACKs for top commit:
  sanket1729:
    utACK 06400b1

Tree-SHA512: 6a30480becf20aa64e1f4528cf0d1df75bd2ddbaad95ae6bd4a3f5885624a791c7d7a77020b27675c348b2ff95467e0cb8bb42ca2c58112d7322f75480608dca
@apoelstra apoelstra mentioned this pull request May 4, 2025
apoelstra added a commit that referenced this pull request May 24, 2025
953e679 bump patch version of 11.2 (Andrew Poelstra)
3bf002c add regression test for #806 (Andrew Poelstra)
4f8b065 descriptor: fix key parsing error handling in parse_desc (Andrew Poelstra)
7c651f4 ci: update fuzz CI job to reorder tests (Andrew Poelstra)
aaf276c lib: remove some deny lints (Andrew Poelstra)

Pull request description:

  Backport of #809 to 11.x. Last backport for this PR.

ACKs for top commit:
  sanket1729:
    utACK 953e679

Tree-SHA512: 8f9ee0b49f2d2e1e4560a950bd84dad2bef285e72466b3ccf19170157958e5749bc73194581b40c7dc977c6938627ccb444d58a2161934e148281891c6b319de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fuzz: panic in Descriptor::parse_descriptor with Invalid pkh() Input in rust_miniscript
4 participants