Resurrect pstratem's "Simple fuzzing framework"#9172
Conversation
|
Concept ACK. |
Piping in data is the part that the fuzzer should do :) |
|
I've added some bare-bones instructions on how to use AFL with Bitcoin Core. What I don't have are example inputs, so this doesn't do anything yet at the moment. @pstratem Would you mind sharing some of your example inputs? |
|
Concept ACK. Would prefer to see at least one MWE. |
|
What's an MWE? |
|
Minimal working example: Just a code bit that is of the smallest size, but still demonstrates how the framework is used. |
Agreed. I demonstrate how to use the framework in the documentation I added, but there need to be some example inputs to put in |
|
@laanwj suggestion for where examples should go? (which directory) |
|
Don't know. Probably a special repo with bitcoin core "manual testing" data
eventually. At this point though I'd be happy with a dropbox link as I'd
just like to start fuzzing.
|
|
FWIW, I have a 24 core host grinding on this in AFL for most of a day on this... I guess I'll leave it running all weekend and spin up some more cores. if someone wants the resulting vectors after I cmin them, I'd be glad to hand them out. (I just started from some particularly dumb /dev/urandom starting points... a future improvement I would suggest (not this PR!) might be a command-line argument that makes it serialize a dummy object or few for each of the modes and write out a set of files.) |
Yes that would be useful. Should ideally have realistic examples that come up in bitcoind itself (just grabbed from the wire randomly and saved) along with synthetic ones.
Yes please! |
| `AFLPATH` was set as above): | ||
| ``` | ||
| ./configure --disable-ccache --disable-shared --enable-tests CC=${AFLPATH}/afl-gcc CXX=${AFLPATH}/afl-g++ | ||
| export AFL_HARDEN=1 |
There was a problem hiding this comment.
For anyone wondering what AFL_HARDEN does. From the afl README.
Setting AFL_HARDEN=1 when calling 'make' will cause the CC wrapper to
automatically enable code hardening options that make it easier to detect
simple memory bugs. Libdislocator, a helper library included with AFL (see
libdislocator/README.dislocator) can help uncover heap corruption issues, too.
There was a problem hiding this comment.
It may actually be unnecessary with bitcoin core which already enables a lot of hardening options. Then again, it can't hurt either I think.
|
Started testing this on OS X 10.12. test_bitcoin_fuzzy compiles with one warning However, running gave: Same result even after upping the memory limit to 4GB (-m4096) which should be more than enough. Setting Also testing on Ubuntu 16.04 running in VirtualBox. |
|
Given that one was working alright, I've moved to running five in parallel. Will leave them running for a while. You can use the afl-whatsup tool to watch the progress, sample output below: |
| examples that it found. | ||
|
|
||
| ``` | ||
| mkdir inputs |
There was a problem hiding this comment.
µnit:
mkdir test/afl-inputs
AFLIN=$PWD/test/afl-inputs
...There was a problem hiding this comment.
nit: we could also add .gitignore entries for test/inputs, test/outputs and test/sync_dir
There was a problem hiding this comment.
Well, this is just an example. There's no strong reason to make these directories inside the bitcoin repository.
|
Concept ACK 18b4c6c |
doc/fuzzing.md
Outdated
|
|
||
| To start the actual fuzzing use: | ||
| ``` | ||
| $AFLPATH/afl-fuzz -i ${AFLIN} -o ${AFLOUT} -- test/test_bitcoin_fuzzy |
There was a problem hiding this comment.
I think the default memory needs to be increased to prevent OOM?
There was a problem hiding this comment.
Which OS did you see out of memory errors on?
What did you need to increase it to to make it run?
There was a problem hiding this comment.
I used the inputs by @laanwj (bitcoin_fuzzy_in.tar.xz) and the default of -m50 caused a failure in the dry run.
There was a problem hiding this comment.
Setting -m52 seems to be enough in this case. (OS: fedora 24)
|
I didn't have to change anything memory related on ubuntu at least
|
|
tested ACK. Works well on OSX with AFL_NO_FORKSRV set (probably worth documenting that it's necessary there). Also, feel free to take this as a simplification: theuni@88b8f92 |
|
@cfields thanks - though I'm not sure moving addrman to common is the right
thing to do. It is decidedly a server thing and not used by any of the
other (nontest) executables.
And later on we may want to fuzz more things in _server.
|
|
@fanquake if you completely remove the memory limit there are a few tests which can use nearly unbounded amounts of memory I suggest running with: export AFL_SKIP_CRASHES=1; afl-fuzz $AFL_PARAMETERS -i fuzzing/input -o fuzzing/output ./bitcoin/src/test/test_bitcoin_fuzzy |
|
@pstratem Thanks for the suggestions. I've restarted some of the fuzzers with the new vars. |
|
Wrapping up my testing of this. Results from individual fuzzers are below, and the sync_dir (outputs) as well as the inputs used are available for download. |
|
Concept ACK |
|
I think this ready for merge and we should select and add appropriate afl-inputs in another pull. (Maybe fix the doc nits before merge?) |
I don't think we should put the inputs in the repository. But I'll just add a link where they can be downloaded. Will fix the doc nits. |
|
My current test cases are here http://strateman.ninja/fuzzing.tar.xz |
0152939 to
8e0550f
Compare
src/test/test_bitcoin_fuzzy.cpp
Outdated
There was a problem hiding this comment.
error: ‘class CTransaction’ has no member named ‘Unserialize’There was a problem hiding this comment.
right, this should probably be converted to CMutableTransaction
8e0550f to
8b15434
Compare
|
Switched to the new way of transaction deserialization. This should be ready for merge now. |
d059544 [Build] fuzz target, change LIBBITCOIN_ZEROCOIN link order. (furszy) 2396e6b [fuzz] Add ContextualCheckTransaction call to transaction target. (furszy) f0887a0 Fuzzing documentation "PIVX-fication" (furszy) 9631f46 [doc] add sanitizers documentation in developer-notes.md (furszy) 70a0ace tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. Avoid code repetition. (practicalswift) e1b92b6 ignore new fuzz targets gitignore (furszy) d058d8c tests: Add deserialization fuzzing harnesses (furszy) e1f666c tests: Remove TRANSACTION_DESERIALIZE (replaced by transaction fuzzer) (practicalswift) b5f291c tests: Add fuzzing harness for CheckTransaction(...), IsStandardTx(...) and other CTransaction related functions (furszy) 3205871 fuzz: Remove option --export_coverage from test_runner (MarcoFalke) 52693ee fuzz: Add option to merge input dir to test runner (MarcoFalke) 2b4f8aa doc: Remove --disable-ccache from docs (MarcoFalke) b54b1d6 tests: Improve test runner output in case of target errors (practicalswift) cd6134f test: Log output even if fuzzer failed (MarcoFalke) 48cd0c8 doc: Improve fuzzing docs for macOS users (Fabian Jahr) d642b67 [Build] Do not disable wallet when fuzz is enabled. (furszy) c3447b5 Update doc and CI config (qmma) 1266d3e Disable other targets when enable-fuzz is set (qmma) f28ac9a build: Allow to configure --with-sanitizers=fuzzer (MarcoFalke) 425742c fuzz: test_runner: Better error message when built with afl (MarcoFalke) 541f442 qa: Add test/fuzz/test_runner.py (MarcoFalke) 89fe5b2 Add missing LIBBITCOIN_ZMQ to test target (furszy) 58dbe79 add fuzzing binaries to gitignore. (furszy) 393a126 fuzz: Move deserialize tests to test/fuzz/deserialize.cpp (MarcoFalke) a568df5 test: Build fuzz targets into separate executables (furszy) d5dddde [test] fuzz: make test_one_input return void (MarcoFalke) 2e4ec58 [fuzzing] initialize chain params by default. (furszy) 08d8ebe [tests] Add libFuzzer support. (practicalswift) 84f72da [test] Speed up fuzzing by ~200x when using afl-fuzz (practicalswift) faf2be6 Init ECC context for test_bitcoin_fuzzy. (Gregory Maxwell) 11150df Make fuzzer actually test CTxOutCompressor (Pieter Wuille) d6f6a85 doc: Add bare-bones documentation for fuzzing (Wladimir J. van der Laan) 5c3b550 Simple fuzzing framework (pstratem) Pull request description: As the title says, adding fuzzing framework support so we can start getting serious on this area as well. Adapted the following PRs: * bitcoin#9172. * bitcoin#9354. * bitcoin#9691. * bitcoin#10415. * bitcoin#10440. * bitcoin#15043. * bitcoin#15047. * bitcoin#15295. * bitcoin#15399 (fabcfa5 only). * bitcoin#16338. * bitcoin#17051. * bitcoin#17076. * bitcoin#17225. * bitcoin#17942. * bitcoin#16236 (only fa35c42). * bitcoin#18166 (only f2472f6). * bitcoin#18300. * And.. probably will go further and continue adapting more PRs.. ACKs for top commit: random-zebra: utACK d059544 and merging... Tree-SHA512: c0b05bca47bf99bafd8abf1453c5636fe05df75f16d0e9c750368ea2aed8142f0b28d28af1d23468b8829188412a80fd3b7bdbbda294b940d78aec80c1c7d03a









Because I feel strongly that we should have it, this brings back #7940, with some minor changes to make it ready for merge:
CDataStream::GetVersionsrc/tests, rename totest_bitcoin_fuzzyto make it clear it is part of the tests, not a user entry pointmake installnor shipped as part of the distribution (it is kind of useless without instrumentation). I opted to make it build by default with--enable-testsbecause if it is a specific option, no one will ever bother to enable it (which means it doesn't get built and runs out of date), and we don't want too many--enable-Xanyway.