build: set ENABLE_IPC to OFF when fuzzing

[fanquake]

A BUILD_FOR_FUZZING build will currently failure to configure, with missing capnp.

[DrahtBot]

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33235.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Crypt-iQ, dergoegge, marcofleon, janb84

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[maflcko]

A BUILD_FOR_FUZZING build will currently failure to configure, with missing capnp.

IPC is by default on, and any build will fail with missing capnp? Whatever the user/dev is doing to fix those should be trivial to apply to a fuzz build to not have to special-case it?

[fanquake]

Whatever the user/dev is doing to fix those should be trivial to apply to a fuzz build to not have to special-case it?

It's already special cased though: BUILD_FOR_FUZZING=ON will disable all other targets and force BUILD_FUZZ_BINARY=ON., and that will no-longer be true if we leave IPC on.

[maflcko]

Yes, the multiprocess binary targets are correctly disabled already on current master https://cirrus-ci.com/task/4847132122808320?logs=ci#L2057:

[17:44:32.757] Configure summary
[17:44:32.757] =================
[17:44:32.757] Executables:
[17:44:32.757]   bitcoin ............................. OFF
[17:44:32.757]   bitcoind ............................ OFF
[17:44:32.757]   bitcoin-node (multiprocess) ......... OFF
[17:44:32.757]   bitcoin-qt (GUI) .................... OFF
[17:44:32.757]   bitcoin-gui (GUI, multiprocess) ..... OFF
[17:44:32.757]   bitcoin-cli ......................... OFF
[17:44:32.757]   bitcoin-tx .......................... OFF
[17:44:32.757]   bitcoin-util ........................ OFF
[17:44:32.757]   bitcoin-wallet ...................... OFF
[17:44:32.757]   bitcoin-chainstate (experimental) ... OFF
[17:44:32.757]   libbitcoinkernel (experimental) ..... OFF
[17:44:32.757] Optional features:
[17:44:32.757]   wallet support ...................... ON
[17:44:32.757]   external signer ..................... OFF
[17:44:32.757]   ZeroMQ .............................. OFF
[17:44:32.757]   IPC ................................. ON
[17:44:32.757]   USDT tracing ........................ OFF
[17:44:32.757]   QR code (GUI) ....................... OFF
[17:44:32.757]   DBus (GUI) .......................... OFF
[17:44:32.757] Tests:
[17:44:32.757]   test_bitcoin ........................ OFF
[17:44:32.757]   test_bitcoin-qt ..................... OFF
[17:44:32.757]   bench_bitcoin ....................... OFF
[17:44:32.757]   fuzz binary ......................... ON
[17:44:32.757] 
[17:44:32.757] Cross compiling ....................... FALSE
[17:44:32.757] C++ compiler .......................... Clang 20.1.8, /usr/bin/clang++
[17:44:32.757] CMAKE_BUILD_TYPE ...................... RelWithDebInfo
[17:44:32.757] Preprocessor defined macros ........... FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION

[fanquake]

Yea. So failing to configure because a dependency you don't even need, isn't installed, doesn't make any sense.

[ryanofsky]

IIUC, this change which toggles off the ENABLE_IPC option automatically when BUILD_FOR_FUZZING is set is safe for now because the fuzz test binary doesn't currently call any IPC code.

The change makes it more convenient to run the fuzz test without capnp installed since you don't have to turn off ENABLE_IPC manually.

But I think we would want to revert this change as soon as any fuzz test is written that does call IPC code, so it does seem like there may not much long term benefit in making it, if I understand correctly. It would be good to have some fuzz tests exercising IPC code.

[fanquake]

It would be good to have some fuzz tests exercising IPC code.

Is anyone working on that? I guess @Sjors?

[ryanofsky]

Is anyone working on that? I guess @Sjors?

I could put together something very basic like a fuzz test calling the echoipc RPC method. But there are a number of other loose ends since #31802 which should take priority so will probably not get to this very soon. @darosior has mentioned writing fuzz tests for IPC before which could be more meaningful, like fuzzing the IPC socket.

[Sjors]

Made a note to add a fuzz test if @darosior or @ryanofsky don't get around to it.

Would prefer to leave this ON unless such a PR doesn't land before branch-off. Maybe add a v30 milestone?

[maflcko]

I think marcofleon may pick this up next month?

[dergoegge]

Just leaving a few notes on fuzzing the IPC interfaces.

There are several approaches to fuzzing the IPC interface and not all of them make sense within our repo. If we add IPC fuzz tests in the repo they should not spawn separate processes and not do actual IPC over a socket. This is because the fuzz tests we have in our repo are designed to run in a single process.

We could add tests that call the c++ handler methods for the various interface functions either directly or through the capnp wrappers (only if possible without actual ipc). This should work and depending on the approach might not even require capnp as a dependency. This approach would likely suffer from instability due to state build up, just like the RPC fuzz tests.

Another approach could be to add IPC fuzz tests to fuzzamoto, where the test would essentially simulate an actual application connecting to the IPC interfaces (i.e. doing real IPC over a socket). The snapshot fuzzing approach would also avoid the state build up problem. This is the approach I would take, if I was gonna work on it.

There is also the question of what to target and what assurances it gives us. The IPC interface is trusted, so from a security perspective it's mostly uninteresting (e.g. a crash in this interface isn't considered a security issue). So really, we should mostly fuzz to gain confidence in stability and correctness, for which I'd suggest:

• API fuzzing: basically calling "random" sequences of the available IPC functions (similar to our RPC tests). This would mostly aim at stability, i.e. there are no random unexpected crashes caused by edge case usage of the interface. It might be possible to leverage something like https://github.com/dergoegge/libcapnp-mutator to infer a "grammar" for fuzzing the API, because our capnp files already encode all the type and structural information for how to use the interface (libcapnp-mutator would need to be built out further for this to really work, but it might serve as a starting point). This would make the maintenance of this kind of test very easy, as any time we change the capnp files, the fuzzer would automatically learn about it and know how to fuzz it.
• Correctness fuzzing: limit the scope of the above API fuzzing approach to a subset of interesting functions for which we can make assertions about expected behavior.

[ryanofsky]

Thanks @dergoegge this is really helpful. It's been hard for me to think about what would constitute a meaningful fuzz test, and it still may not be exactly clear, but this provides a great framework. I just have two question

they should not spawn separate processes and not do actual IPC over a socket

Not spawning separate processes makes sense, but I assumed it might be ok to communicate across an internal sockiet, e.g. with socketpair? That is what a lot of IPC unit tests are doing.

The IPC interface is trusted, so from a security perspective it's mostly uninteresting (e.g. a crash in this interface isn't considered a security issue)

This is true currently, but it would be possible to make IPC interfaces that are untrusted by checking inputs more rigorously and adding resource caps like maximum number of threads or block templates (in case of the mining interface) an IPC client could create. So I'd be curious if it would affect the recommended fuzzing strategy if we did offer untrusted interfaces or decide to make the mining interface less trusted in the future.

[dergoegge]

Not spawning separate processes makes sense, but I assumed it might be ok to communicate across an internal sockiet, e.g. with socketpair? That is what a lot of IPC unit tests are doing.

I'd advise against this (e.g. does this work for all OSs in CI?, might be slower) but it might also be fine.

So I'd be curious if it would affect the recommended fuzzing strategy if we did offer untrusted interfaces or decide to make the mining interface less trusted in the future.

In addition to what I suggested above, I'd fuzz any capnp code that would be exposed (e.g. deserialization) and check what kind of assurances they make w.r.t. security. https://capnproto.org/faq.html#is-capn-proto-secure looks reasonable, except for "the RPC layer is not robust against resource exhaustion attacks, possibly allowing denials of service".

[marcofleon]

we should mostly fuzz to gain confidence in stability and correctness, for which I'd suggest:

I'm happy to work on this. Still thinking about the best approach. It seems like using fuzzamoto is ultimately most effective, but it might be worth trying to get a baseline single-process fuzz test into the repo? This would probably be a more practical starting point for me too, but I'm open to suggestions.

[Sjors]

a baseline single-process fuzz test into the repo?

I think so yes, otherwise it might not make the branch-off.

[Crypt-iQ]

tACK af4156a

I ran into this while trying to measure fuzz coverage.

[dergoegge]

utACK af4156a

Seems reasonable to do for now while we don't have ipc fuzz tests and don't require the dependency.

[marcofleon]

ACK af4156a

I've decided to take the fuzzamoto approach, as I believe it'll be the best way to uncover bugs. In any case, disabling IPC for fuzz builds makes sense, at least until we have fuzz tests that need it.

[janb84]

ACK af4156a

PR sets the option ENABLE_IPC to OFF when building for fuzzing. This seems reasonable for now, because we do not have any fuzz testing on IPC code. Setting ENABLE_IPCtoOFF` will reduce a dependency (capnp).

• build and tested ✅

[ryanofsky]

It seems like there's not much downside to this change, other than that it may need to be reverted later if an IPC fuzz test is added. And I think if a fuzz test is added we would also want to revert google/oss-fuzz#13854?

Only Sjors above #33235 (comment) expressed a preference for not making this change and maflcko #33235 (comment) raised an early question about it. I think if there aren't renewed objections this would make sense to merge. Impact seems small either way.

[Sjors]

I'm fine with this change if no fuzz PR is merged before branch-off.

[maflcko]

lgtm

Should be fine either way and shouldn't matter much, because the set of devs without capnp doing fuzzing is limited