fuzz: Add process_messages harness #18521
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK Very nice fuzzing harness! Keep them coming! :) |
maflcko
force-pushed
the
2004-fuzzMsgs
branch
2 times, most recently
from
fac3812 to
fa6a008
Compare
|
Tested ACK fa6a008 Thanks for adding a very nice harness! |
|
Thanks for testing. Did you find the CVE after re-introducing it? |
|
Yes :) Live demo: |
|
Going to go ahead and merge this. If someone has feedback or questions, you are welcome to leave them here. |
robot-visions
left a comment
robot-visions
left a comment
There was a problem hiding this comment.
ACK fa6a008.
This looks extremely useful, thanks for adding! I was also able to reproduce the CVE:
bloom.cpp:45:69: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior bloom.cpp:45:69 in
AddressSanitizer:DEADLYSIGNAL
=================================================================
==56684==ERROR: AddressSanitizer: FPE on unknown address 0x00010a4a031e (pc 0x00010a4a031e bp 0x7ffee63a2d60 sp 0x7ffee63a2d30 T0)
#0 0x10a4a031e in CBloomFilter::Hash(unsigned int, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) const bloom.cpp:45
#1 0x10a49ffeb in CBloomFilter::insert(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) bloom.cpp:52
#2 0x109c1d4f8 in ProcessMessage(CNode*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, CDataStream&, long long, CChainParams const&, CTxMemPool&, CConnman*, BanMan*, std::__1::atomic<bool> const&) net_processing.cpp:3183
#3 0x109c5d928 in PeerLogicValidation::ProcessMessages(CNode*, std::__1::atomic<bool>&) net_processing.cpp:3359
#4 0x10985b7a9 in test_one_input(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) process_messages.cpp:70
| } | ||
|
|
||
| while (fuzzed_data_provider.ConsumeBool()) { | ||
| const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE).c_str()}; |
There was a problem hiding this comment.
Could the fuzzer check "interesting" cases more quickly by choosing a random but valid command, instead of generating a random string of length COMMAND_SIZE?
There was a problem hiding this comment.
Good question. I think there is a trade-off here between giving the fuzz engine as much freedom to explore as possible at a potentially slight cost of performance (citation needed, giving more freedom could also increase efficiency).
Your suggestion would involve enumerating all netmessage types in an array and then having the fuzz engine pick one from it. This is possible, but as soon as you add a new message type, the developed seeds (inputs) render invalid and need to be adjusted. Readjusting the seeds should not be an issue for the fuzz engine, but it has to be done.
Apart from the above slight inconvenience of having to readjust the seeds on every added (or removed) message type, there is also an issue that the enumeration could accidentally miss one message type. On top, Bitcoin Core ignores unknown message types, and I'd say we want to test this code path as well. (Have at least one unknown message type generated by the fuzzer). This can also be fixed by adding an unknown message type to the fixed array the fuzz engine can pick from. E.g. "foobarfoo".
Moreover, we might also want to test for slight modifications of the same message type and see if they affect behavior in any way. For example b"filteradd\x01add" vs b"filteradd\x02". Those two should never be treated as a valid command and obviously shouldn't crash Bitcoin Core. There should be unit and maybe other fuzz tests in place to check this, but I think allowing the fuzz engine in this test more freedom can't hurt.
Finally, while I don't know the internals of fuzz engines, I believe they are really smart and quick about figuring out "interesting" strings that appear in the source code. If I had to guess what the performance penalty was of allowing any message type as opposed to limiting it to a fixed set of types, I'd presume it is an unnoticeable difference, maybe slight slow down or possibly a slight speed up even. A benchmark might help here.
I tried a few other presumed performance improvements while writing the test and surprisingly I found that writing the least code, simplest code, while allowing the fuzz engine the most freedom was also the fastest way to find the CVE.
There was a problem hiding this comment.
Thanks for the really thorough explanation! That tradeoff / your decision makes sense to me, especially since I didn't know that fuzz engines might do better than "just generate random strings".
There was a problem hiding this comment.
I agree as well that the merged version makes sense, but just out of interest I ran both versions (PR and a version which samples an array with all NetMsgTypes currently defined in protocol.cpp ) in parallel with libfuzzer and it was the altered version which found the reintroduced CVE first after 2.3M iterations / 2 hours, while the PR version hadn't found it after 10M tries.
I found it interesting that while the cov metric was similar between the runs, the ft metric of the fuzzer was consistently higher for the array version throughout the run.
Although this was just sample size 1, so finding the CVE might have just been a lucky mutation in the fuzzer algorithm...
There was a problem hiding this comment.
How many workers did you run in parallel?
There was a problem hiding this comment.
Just 1 for each version.
There was a problem hiding this comment.
This sounds like a promising speed-up. I wouldn't object either having this one replaced or a new fuzz target with the array version.
There was a problem hiding this comment.
I'll do some more runs tomorrow, maybe I was just lucky. I don't have a good feeling for the variance of runtimes yet.
There was a problem hiding this comment.
I saw it vary by a factor or 5 or 10 in my tests last week, so it could very well be just noise
|
As a data point, I was able to reproduce the CVE. It took about ~5-6 hours running 16 jobs. |
Summary: Backport of core [[bitcoin/bitcoin#18521 | PR18521]]. Depends on D8374. Test Plan: ninja bitcoin-fuzzers ./src/test/fuzz/process_messages <path_to_corpus> Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8376
Summary: This reverts commit rABC22c85ce64493710b114ad598a594f9a9b25422c5. This is a backport of core [[bitcoin/bitcoin#18521 | PR18521]]. It is similar to D8376 rebased on top of D8525 which fixes the tsan race. Depends on D8634 for the test plan. Test Plan: ninja bitcoin-fuzzers ./src/test/fuzz/process_messages <path_to_corpus> ninja all check-all With TSAN: ninja check-bitcoin-radix_tests check-bitcoin-coinselector_tests This will likely trigger the TSAN issue on D8376 but not on this diff. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8635
| { | ||
| FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); | ||
|
|
||
| ConnmanTestMsg& connman = *(ConnmanTestMsg*)g_setup->m_node.connman.get(); |
There was a problem hiding this comment.
I'm pretty sure this is UB. You can't cast to a subclass of the actually created object, even if it adds no fields or member functions.
There was a problem hiding this comment.
Good find, further discussion in #24373
(Obviously I was testing the review process to see how long it would take to find out)
8 participants
No description provided.