Skip to content

tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...) #19379

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
merged 2 commits into from
Aug 31, 2020
Merged

tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...) #19379

merged 2 commits into from
Aug 31, 2020

Conversation

practicalswift
Copy link
Contributor

Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...).

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

@practicalswift practicalswift force-pushed the fuzzers-SigHashLowR branch 2 times, most recently from 6abcd62 to 07fad2c Compare June 25, 2020 14:38
@practicalswift
Copy link
Contributor Author

@Crypt-iQ Hello fuzzing friend! Would you mind reviewing? :)

@Crypt-iQ
Copy link
Contributor

@practicalswift Will do

@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 28, 2020 •
edited
Loading

@MarcoFalke Would you mind reviewing? FWIW this PR touches only src/test/fuzz/ (edit: not entirely correct -- it also makes the relevant functions visible outside of their translation unit to allow for testing) :)

@Crypt-iQ
Copy link
Contributor

ACK 46fcac1

Coverage from just these harnesses: https://crypt-iq.github.io/19379_fuzz_cov/

Ubuntu:

  • ./configure --enable-fuzz --with-sanitizers=address,undefined,integer,fuzzer reports no errors
  • valgrind reports no errors on either harness

macOS:

  • ./configure --enable-fuzz --with-sanitizers=address,fuzzer --disable-asm reports no errors
  • ./configure --enable-fuzz --with-sanitizers=undefined,integer,fuzzer --disable-asm complains for the secp256k1_ecdsa_signature_parse_der_lax harness:
/usr/local/opt/llvm/bin/../include/c++/v1/memory:1876:35: runtime error: implicit conversion from type 'char' of value -35 (8-bit, signed) to type 'unsigned char' changed the value to 221 (8-bit, unsigned)
    #0 0x1049734d1 in std::__1::enable_if<__is_cpp17_forward_iterator<std::__1::__wrap_iter<char const*> >::value, void>::type std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >::__construct_at_end<std::__1::__wrap_iter<char const*> >(std::__1::__wrap_iter<char const*>, std::__1::__wrap_iter<char const*>, unsigned long) vector:1076
    #1 0x104972b2a in ConsumeRandomLengthByteVector(FuzzedDataProvider&, unsigned long) util.h:38
    #2 0x104972482 in test_one_input(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) secp256k1_ecdsa_signature_parse_der_lax.cpp:20
    #3 0x1055b4ca6 in LLVMFuzzerTestOneInput fuzz.cpp:45
    #4 0x1057295a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:556
    #5 0x105728ce5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) FuzzerLoop.cpp:470
    #6 0x10572b387 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:765
    #7 0x10572b6e9 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:792
    #8 0x1057189dd in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:829
    #9 0x105744e12 in main FuzzerMain.cpp:19
    #10 0x7fff73778cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

maflcko
maflcko reviewed Aug 31, 2020
View reviewed changes
@maflcko maflcko merged commit 61b8c04 into bitcoin:master Aug 31, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 31, 2020
Merge bitcoin#19379: tests: Add fuzzing harness for SigHasLowR(...) a…
14bf1e5 
…nd ecdsa_signature_parse_der_lax(...)

46fcac1 tests: Add fuzzing harness for ec_seckey_import_der(...) and ec_seckey_export_der(...) (practicalswift)
b667a90 tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...) (practicalswift)

Pull request description:

  Add fuzzing harness for `SigHasLowR(...)` and `ecdsa_signature_parse_der_lax(...)`.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  Crypt-iQ:
    ACK 46fcac1

Tree-SHA512: 11a4856a1efd9a04030a8c8aee2413fd5be1ea248147e649a48a55bacdf732bb48a19ee1ce2761d47d4dd61c9598aec53061b961b319ad824d539dda11a8ccf4
@practicalswift practicalswift deleted the fuzzers-SigHashLowR branch April 10, 2021 19:42
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 17, 2022
@PastaPastaPasta
Merge bitcoin#19379: tests: Add fuzzing harness for SigHasLowR(...) a…
f058882 
…nd ecdsa_signature_parse_der_lax(...)

46fcac1 tests: Add fuzzing harness for ec_seckey_import_der(...) and ec_seckey_export_der(...) (practicalswift)
b667a90 tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...) (practicalswift)

Pull request description:

  Add fuzzing harness for `SigHasLowR(...)` and `ecdsa_signature_parse_der_lax(...)`.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  Crypt-iQ:
    ACK 46fcac1

Tree-SHA512: 11a4856a1efd9a04030a8c8aee2413fd5be1ea248147e649a48a55bacdf732bb48a19ee1ce2761d47d4dd61c9598aec53061b961b319ad824d539dda11a8ccf4
kwvg added a commit to kwvg/dash that referenced this pull request Aug 11, 2022
@kwvg
merge bitcoin#19379: Add fuzzing harness for SigHasLowR(...) and ecds…
278ed72 
…a_signature_parse_der_lax(...)
@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.
Reviewers
1 more reviewer

@maflcko maflcko maflcko left review comments

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.
4 participants
@practicalswift @Crypt-iQ @maflcko @DrahtBot