Skip to content

Conversation

practicalswift
Copy link
Contributor

The fuzzers eval_script and script_flags require holding ECCVerifyHandle.

This is a follow-up to #17235 which accidentally broke those two fuzzers.

Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

@maflcko
Copy link
Member

maflcko commented Oct 30, 2019

What are the steps to reproduce? Please upload a seed to https://github.com/bitcoin-core/qa-assets

@practicalswift
Copy link
Contributor Author

This test case triggers the condition in script_flags:

020200000002020201006040001f84e6008d000000000000000000000000
04000000000000002751261a535353295b5352210300000000a7000000ac
acacacacacacacacacacac7573655f636d70acacacaca7ac000000000000
0000000000000000000000000000000010101000000000
$ mkdir script_flags-testcase/
$ xxd -p -r <<< "020200000002020201006040001f84e6008d00000000000000000000000004000000000000002751261a535353295b5352210300000000a7000000acacacacacacacacacacacac7573655f636d70acacacaca7ac0000000000000000000000000000000000000000000010101000000000" > script_flags-testcase/crash
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/fuzz/script_flags script_flags-testcase/
pubkey.cpp:174:36: runtime error: null pointer passed as argument 1, which is declared to never be null

This test case triggers the condition in eval_script:

1a3017021200f98d0a0f831000210000000000001a32170201008100ac6e
6e878c
$ mkdir script_flags-testcase/
$ xxd -p -r <<< "1a3017021200f98d0a0f831000210000000000001a32170201008100ac6e6e878c" > eval_script-testcase/crash
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/fuzz/eval_script eval_script-testcase/
pubkey.cpp:35:45: runtime error: null pointer passed as argument 1, which is declared to never be null

@practicalswift
Copy link
Contributor Author

Seeds submitted to qa-assets in bitcoin-core/qa-assets#1 :)

@practicalswift
Copy link
Contributor Author

@MarcoFalke The broken fuzzers are causing some pain in the form of false positives. Would it be possible to get those changes in? :)

maflcko pushed a commit that referenced this pull request Oct 31, 2019
…dding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to #17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
@maflcko maflcko merged commit 9cae3d5 into bitcoin:master Oct 31, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 31, 2019
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
maflcko pushed a commit that referenced this pull request Dec 6, 2019
d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR #17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 6, 2019
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 10, 2020
…rifyHandle only when needed.

Summary:
This diff squashes three Core PRs into one. The reason is that [[bitcoin/bitcoin#17235 | PR17235]] introduces a bug, and [[bitcoin/bitcoin#17274 | PR17274]] and [[bitcoin/bitcoin#17685 | PR17685]] both fix it, so our fuzzing test setup isn't broken at any point.

---

c2f964a6745be085f2891c909d6c998687de9080 tests: Remove Cygwin WinMain workaround (practicalswift)
db4bd32cc31789fc017f5db0b86a69ee43e41575 tests: Skip unnecessary fuzzer initialisation. Hold ECCVerifyHandle only when needed. (practicalswift)

Pull request description:

  Skip unnecessary fuzzer initialisation. Hold `ECCVerifyHandle` only when needed.

  As suggested by MarcoFalke in bitcoin/bitcoin#17018 (comment).

---

Merge #17274: tests: Fix fuzzers eval_script and script_flags by re-adding ECCVerifyHandle dependency

9cae3d5e94f4481e0d251c924314e57187a07a60 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to #17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

---

Merge #17685: tests: Fix bug in the descriptor parsing fuzzing harness (descriptor_parse)

6338c0203416a5f86e9422b6cd479da8af277f2f tests: Fix fuzzing harness for descriptor parsing (descriptor_parse) (practicalswift)

Pull request description:

  Fix bug in the descriptor parsing fuzzing harness (`descriptor_parse`) by making sure `secp256k1_context_verify` is properly initialized (via `ECCVerifyHandle`).

  Background:

  When fuzzing `Parse(…)` with `libFuzzer` I eventually reached the test case `combo(020000000000000000000000000000000000000000000000000000000000000000)`. That input triggers a call to `CPubKey::IsFullyValid()` which in turns requires an initialized `secp256k1_context_verify`.

  The fuzzing harness did not fulfil that pre-condition prior to this commit (sorry, my fault!) :)

---

Depends on D6881

Backport of Core [[bitcoin/bitcoin#17235 | PR17235]], [[bitcoin/bitcoin#17274 | PR17274]] and [[bitcoin/bitcoin#17685 | PR17685]]

Test Plan:
  cmake -GNinja .. -DENABLE_SANITIZERS="address;fuzzer" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
  ninja bitcoin-fuzzers link-fuzz-test_runner.py
  ./test/fuzz/test-runner.py -l DEBUG <path-to-corpus>

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6883
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 6, 2020
Summary:
> This makes it more clear for fuzzing harness writers and others that ECCVerifyHandle is expected to be held when interacting with CPubKey.
>
> Related [[bitcoin/bitcoin#17274 | PR17274]] (D6883).

This is a backport of Core [[bitcoin/bitcoin#17275 | PR17275]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8301
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
@practicalswift practicalswift deleted the fuzzer-initialization-follow-up branch April 10, 2021 19:39
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 14, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 14, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 26, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 26, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 1, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 3, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 3, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 5, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 7, 2022
…by re-adding ECCVerifyHandle dependency

9cae3d5 tests: Add fuzzer initialization (hold ECCVerifyHandle) (practicalswift)

Pull request description:

  The fuzzers `eval_script` and `script_flags` require holding `ECCVerifyHandle`.

  This is a follow-up to bitcoin#17235 which accidentally broke those two fuzzers.

  Sorry about the temporary breakage my fuzzing friends: it took a while to fuzz before reaching these code paths. That's why this wasn't immediately caught. Sorry.

Top commit has no ACKs.

Tree-SHA512: 67ebb155ba90894c07eac630e33f2f985c97bdf96dc751f312633414abeccdca20315d7d8f2ec4ee3ac810b666a1e44afb4ea8bc28165151cd51b623f816cac2
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants