net: fuzz: bypass network magic and checksum validation

[brunoerg]

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.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

No conflicts as of last run.

[brunoerg]

cc: @dergoegge

[dergoegge]

Concept ACK

[brunoerg]

Just moved it to draft to get some conceptual feedbacks first. I added a commit to bypass some asserts in 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?

[maflcko]

I added a commit to bypass some asserts in 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?

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?

[dergoegge]

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.

[brunoerg]

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:

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
    if ((hdr.pchMessageStart[0] & 1) != 0 && hdr.pchMessageStart != m_magic_bytes) {
#else

instead of just:

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
    if ((hdr.pchMessageStart[0] & 1) != 0) {
#else

I think this way we still facilitate it without invalidating valid magic/checksum.

[brunoerg]

Force-pushed addressing #31012 (comment) and #31012 (comment). Thanks, @dergoegge.

[marcofleon]

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.

[dergoegge]

Copy and pasting the command from the docs doesn't work:

$ git apply << "EOF" ...
error: corrupt patch at line 15

[brunoerg]

Copy and pasting the command from the docs doesn't work:

Just fixed it. Can you check it?

[dergoegge]

-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.

[brunoerg]

-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.

[DrahtBot]

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/31190077287}

Hints

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.

[brunoerg]

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.

[dergoegge]

ACK d439666

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.

[marcofleon]

Tested ACK d439666

[theuni]

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:

#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
exit(1);
#endif

Edit: I realize that bitcoind should be disabled in that case. This would be belt-and-suspenders to make sure that always holds.

[brunoerg]

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.

[fanquake]

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.

[maflcko]

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_serialization fuzz 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.

[brunoerg]

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.

[maflcko]

Ok, then I am nack-ish on this. Generally, test code should follow real code, not the other way round, unless there is a reason. Saving 15 lines of test-only code seems a weak reason to me, but I don't mind if others like this.

[brunoerg]

Since everyone agrees on removing Honggfuzz netdriver, I think we can change the approach to simply just remove it from the documentation.

[brunoerg]

Closing this in favor of #31092