Fixes #30957
We currently have some instructions on the documentation about how to fuzz the Bitcoin Core P2P layer using Honggfuzz NetDriver. It includes a patch to be applied to bypass network magic and checksum validation, that is outdated. However, we can change the code to bypass them using FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. It also makes the p2p_transport_serialization fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | dergoegge, marcofleon |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
265-+ if (false && memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) { // skip checksum checking
266- LogDebug(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
267- SanitizeString(msg.m_type), msg.m_message_size,
268- HexStr(Span{hash}.first(CMessageHeader::CHECKSUM_SIZE)),
269 EOF
270 $ cmake -B build_fuzz \
CPPFLAGS will have to include -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION when building bitcoind for fuzzing with the honggfuzz netdriver.
Isn’t FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION already set when using -DBUILD_FOR_FUZZING=ON?
0if(BUILD_FOR_FUZZING)
1 message(WARNING "BUILD_FOR_FUZZING=ON will disable all other targets and force BUILD_FUZZ_BINARY=ON.")
2 set(BUILD_DAEMON OFF)
3 set(BUILD_CLI OFF)
4 set(BUILD_TX OFF)
5 set(BUILD_UTIL OFF)
6 set(BUILD_UTIL_CHAINSTATE OFF)
7 set(BUILD_KERNEL_LIB OFF)
8 set(BUILD_WALLET_TOOL OFF)
9 set(BUILD_GUI OFF)
10 set(ENABLE_EXTERNAL_SIGNER OFF)
11 set(WITH_MINIUPNPC OFF)
12 set(WITH_ZMQ OFF)
13 set(BUILD_TESTS OFF)
14 set(BUILD_GUI_TESTS OFF)
15 set(BUILD_BENCH OFF)
16 set(BUILD_FUZZ_BINARY ON)
17
18 target_compile_definitions(core_interface INTERFACE
19 ABORT_ON_FAILED_ASSUME
20 FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
21 )
22endif()
DBUILD_FOR_FUZZING
This isn’t set for the hfuzz net driver. It uses the HFND_FUZZING_ENTRY_FUNCTION_CXX
ConnmanTestMsg that would fail since we’re bypassing network magic and checksum validation. Also, I think we will have to change p2p_transport_bidirectional target since there are some sanity checks that would fail with the bypasses, especially the assertions about ReceivedBytes/GetReceivedMessage. Any thoughts?
I added a commit to bypass some asserts in
ConnmanTestMsgthat would fail since we’re bypassing network magic and checksum validation. Also, I think we will have to changep2p_transport_bidirectionaltarget since there are some sanity checks that would fail with the bypasses, especially the assertions aboutReceivedBytes/GetReceivedMessage. Any thoughts?
Can you explain why the asserts and sanity checks would be failing? They are unrelated to the checksum, so if they are failing, it seems like this patch is incomplete and some part is still checking the magic or checksum, no?
They are unrelated to the checksum, so if they are failing, it seems like this patch is incomplete and some part is still checking the magic or checksum, no?
I’m guessing that we need to make sure that (if we are fuzzing) V1Transport::SetMessageToSend creates valid checksums/magic-bytes according to the new fuzzing mode check.
Can you explain why the asserts and sanity checks would be failing? They are unrelated to the checksum, so if they are failing, it seems like this patch is incomplete and some part is still checking the magic or checksum, no?
Some previously valid magic or checksum could be now be considered invalid. But I removed the commit that bypasses the asserts and changed the verification to:
0#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
1 if ((hdr.pchMessageStart[0] & 1) != 0 && hdr.pchMessageStart != m_magic_bytes) {
2#else
instead of just:
0#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
1 if ((hdr.pchMessageStart[0] & 1) != 0) {
2#else
I think this way we still facilitate it without invalidating valid magic/checksum.
727@@ -728,7 +728,11 @@ int V1Transport::readHeader(Span<const uint8_t> msg_bytes)
728 }
729
730 // Check start string, network magic
731+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
732+ if ((hdr.pchMessageStart[0] & 1) != 0 && hdr.pchMessageStart != m_magic_bytes) {
733+#else
734 if (hdr.pchMessageStart != m_magic_bytes) {
735+#endif
maybe I am missing something, but this looks still wrong. I am reading this as fuzzing adding additional constraints, which seems the inverse of what we want to achieve.
Also, this could be impossible to satisfy, if !(m_magic_bytes[0]&1) holds. What am I missing?
Edit: Nvm, this is about avoiding the error, so additional constraints are useful to avoid hitting it.
hdr.pchMessageStart != m_magic_bytes we would return an error. That is, fuzzing would have to spend time matching it. Now, we will only invalidate it if hdr.pchMessageStart != m_magic_bytes and (hdr.pchMessageStart[0] & 1) != 0. So, it will make fuzzing easier by just needing to match (hdr.pchMessageStart[0] & 1) == 0.
796@@ -793,7 +797,11 @@ CNetMessage V1Transport::GetReceivedMessage(const std::chrono::microseconds time
797 RandAddEvent(ReadLE32(hash.begin()));
798
799 // Check checksum and header message type string
800+#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
801+ if ((hdr.pchChecksum[0] & 1) != 0 && memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
802+#else
803 if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
804+#endif
I think this could be de-duplicated and made a little cleaner (same for the magic byte check):
0 bool checksum_ok{memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0};
1#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
2 checksum_ok = checksum_ok || (hdr.pchChecksum[0] & 1) != 0;
3#endif
4
5 if (!checksum_ok) {
6 ...
197@@ -198,6 +198,7 @@ $ cmake -B build_fuzz \
198 -DCMAKE_C_COMPILER="$(pwd)/honggfuzz/hfuzz_cc/hfuzz-clang" \
199 -DCMAKE_CXX_COMPILER="$(pwd)/honggfuzz/hfuzz_cc/hfuzz-clang++" \
200 -DBUILD_FOR_FUZZING=ON \
201+ -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION=ON \
This won’t define the c++ macro FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION, as this only sets a cmake build flag (cmake probably complains that this flag doesn’t exist).
You should be able to use -DAPPEND_CPPFLAGS="-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION" (I see where the confusion comes from, as -D is accepted by compilers to define macros and by cmake to set build flags but they are different things).
You will also need to add this to the correct section in the docs (i.e. the netdriver docs):
You should be able to use -DAPPEND_CPPFLAGS="-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION" (I see where the confusion comes from, as -D is accepted by compilers to define macros and by cmake to set build flags but they are different things).
Ah yes, thank you. Just saw some examples from the CI scripts.
Approach ACK. The use of FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in net.cpp looks good to me. Generated a coverage report to verify that both branches of both checks are hit.
I haven’t yet worked through the honggfuzz instructions to confirm that they work. I’ll circle back to this to give it a try.
Copy and pasting the command from the docs doesn’t work:
0$ git apply << "EOF" ...
1error: corrupt patch at line 15
Copy and pasting the command from the docs doesn’t work:
Just fixed it. Can you check it?
-v2transport=0 should be added to the bitcoind args used for netdriver fuzzing. Otherwise, it doesn’t seem like honggfuzz ever fuzzes anything besides the v2 handshake.
Also playing around with this and reading the netdriver post (i couldn’t find any other docs on it) I’m not sure if this will ever even get past the version handshake? So I’m wondering how useful this tool really is for us.
-v2transport=0 should be added to the bitcoind args used for netdriver fuzzing. Otherwise, it doesn’t seem like honggfuzz ever fuzzes anything besides the v2 handshake.
Well noticed, just added it.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31190077287
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Also playing around with this and reading the netdriver post (i couldn’t find any other docs on it) I’m not sure if this will ever even get past the version handshake? So I’m wondering how useful this tool really is for us.
Yes, I don’t think it will pass the version handshake. I can’t see what kind of bugs it would find that other fuzzers would not get.
ACK d43966603bdefb0402a2dc93ff990cb254cdf2df
I think we could consider removing the honggfuzz netdriver instructions in a follow up but the changes look good to me anyway. I tested this by running honggfuzz in netdriver mode.
If we’re going to be peppering the code with these, could we add a safety harness to make sure it’s never being used accidentally for a live node?
Maybe somewhere in init.cpp or bitcoind.cpp:
0#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
1exit(1);
2#endif
Edit: I realize that bitcoind should be disabled in that case. This would be belt-and-suspenders to make sure that always holds.
If we’re going to be peppering the code with these, could we add a safety harness to make sure it’s never being used accidentally for a live node?
Seems good. Note that we’re already use FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION to bypass CheckProofOfWork.
could we add a safety harness to make sure it’s never being used accidentally for a live node? Maybe somewhere in init.cpp or bitcoind.cpp: Edit: I realize that bitcoind should be disabled in that case. This would be belt-and-suspenders to make sure that always holds.
Just want to note that putting something in either of these files wouldn’t be enough to prevent someone producing a kernel lib with the fuzzing code.
I think we could consider removing the honggfuzz netdriver instructions in a follow
Not sure about touching real code with the motivation to make instructions happy that may be removed anyway.
If the changes are meaningful for the fuzz target, then it may be fine.
It also makes the
p2p_transport_serializationfuzz target simpler since we do not need to assist the network magic and checksum creation anymore.
Can you provide more details here? IIUC a hash will still be calculated, just not compared, so this may only drop one hash call, with a possible maximum speed-up of 2x. However, it would be good to actually measure the difference. Otherwise, I am not sure if this is worth it.
Can you provide more details here? IIUC a hash will still be calculated, just not compared, so this may only drop one hash call, with a possible maximum speed-up of 2x. However, it would be good to actually measure the difference. Otherwise, I am not sure if this is worth it.
This is not about performance. It makes the p2p_transport_serialization fuzz target simpler because we can remove the code that assists the checksum and magic bytes creation. So, we can achieve the same with less LoC.