Originally posted by @MarcoFalke in #22962 (comment)
It would be better to write a proper fuzz target for multiprocess (that ideally also covers serialization).
Originally posted by @MarcoFalke in #22962 (comment)
It would be better to write a proper fuzz target for multiprocess (that ideally also covers serialization).
Adding multiprocess fuzz coverage would be similar to adding fuzz coverage for the RPC server, which I don't think we have, so I don't know what would be ideal here.
You could approach it by opening IPC connection, writing arbitrary bytes to the socket, and make sure IPC implementation on the other end is memory safe (doesn't segfault or cause ASAN/MSAN) errors.
You could also call specific IPC methods and check for postconditions.
~A complication here is that IPC interface similar to the RPC interface, and pretty privileged, so it might be able to do things like read/write files on disk depending on how you call it.~ (EDIT: nvm)
A complication here is that IPC interface similar to the RPC interface, and pretty privileged, so it might be able to do things like read/write files on disk depending on how you call it.
Actually, this is not really a concern, because you could stub out all the server methods and just make sure they are being invoked correctly by the IPC and IPC serialization code.
While the server part of the RPC server isn't fuzzed, the RPC methods are fuzzed in the rpc target.
I guess fuzzing multiprocess is hard to fuzz because there is no logic to fuzz, it is just an interface for other logic.
While the server part of the RPC server isn't fuzzed, the RPC methods are fuzzed in the
rpctarget.
Wow, the RPC fuzz target is very interesting! It is just calling whitelisted RPC methods with random arguments. It wouldn't be hard to write the same test for a list of whitelisted IPC methods, since capnp does provide enough introspection to query method arguments and fill them with random values. So given a list of whitelisted methods,, the IPC fuzz test could randomly call whitelisted methods with random argument values, and even randomly fill recursive data structures.
Since a test like this would be calling capnp methods directly, it would not provide fuzz coverage for the C++ wrapper methods which call the corresponding capnp methods. These wrapper methods don't do very much, but are responsible for things like converting CFeeRate objects to and from streams of bytes, converting C++ vectors and maps to and from capnp lists. Separate fuzz tests could be written for this type conversion code, although from discussion #22962 (comment) it sounds like neither of us thinks this coverage would be very useful? There is no equivalent of this fuzzing for our RPC code just because we only provide an RPC server, not an RPC client (we don't have C++ methods that help call corresponding RPC methods remotely).
I guess fuzzing multiprocess is hard to fuzz because there is no logic to fuzz, it is just an interface for other logic.
The same thing is true for the RPC server. There should be little difference between IPC and RPC from a fuzzer's point of view. If you wanted to tweak the RPC fuzz test to only fuzz test the RPC server and skip actual execution of the RPC methods, you could do that. Equivalently, you can test the IPC server with IPC method execution, or the IPC server without IPC method execution.
I will say overall I don't have good intuition for what type of fuzzing is useful, and what type of fuzzing is not useful, and what the costs and benefits are for different kinds of fuzzing. I would love to read a simple guide that told me what fuzzing best practices are (and maybe told me how fuzzing works, because that is also largely a black box to me). But before that happens, I am happy to write whatever fuzzing test someone tells me to write.
One idea to to provide fuzz coverage of the IPC code could be to write an interface specifically meant for fuzz testing.
namespace interfaces {
class FuzzTest
{
virtual ~FuzzTest() = default;
virtual void fuzzInt32(int32_t&) = 0;
virtual void fuzzString(std::string&) = 0;
virtual void fuzzFeeRate(CFeeRate&) = 0;
virtual void fuzzArgsManager(ArgsManager&) = 0;
virtual void fuzzMapVectorFeeRates(std::map<std::string, std::vector<CFeeRate>>&) = 0;
virtual std::vector<int> fuzzInputArgumentsAndReturnValue(int a, int b, int& c, int& d) = 0;
virtual void fuzzCallbackFunction(std::function<int(int)> callback) = 0;
virtual void fuzzCallbackInterface(std::unique_ptr<FuzzTest> callback) = 0;
virtual std::unique_ptr<FuzzTest> fuzzReturnInterface() = 0;
};
} // namespace interfaces
Each method could transform arguments and return values (do modulo arithmetic on integer values, transform, reverse, or shift string values) in some way that the fuzz test code could check. The fuzz test code could call the methods randomly across a socket process boundary.
This type of test could probably provide more meaningful coverage of IPC code than the IPC executing test described previously above that actually executes real whitelisted IPC methods with random arguments. Both types of test could complement each other though.
Note that capnp has it's own detection for if it's being fuzzed, which was added as part of its oss-fuzz integration (https://github.com/capnproto/capnproto/pull/1188). This will likely need to be patched out/worked around at some point. See https://github.com/google/oss-fuzz/pull/14203.
I'd say fuzzamoto is well suited for this. I added a proof-of-concept scenario testing the mining interface, which just randomly calls the methods from the Mining and BlockTemplate interfaces for now. I'm sure we could come up with more sophisticated scenarios that involve the Wallet and Chain interfaces as well. The scenario includes a mining IPC client that connects to the bitcoin-node Unix socket. It's currently directly in the scenario file but could be extracted into a reusable multi-interface client for fuzzamoto to make it easier to write tests for the other interfaces and potentially interactions between them.
For setup, just had to configure fuzzamoto to build bitcoin-node with multiprocess enabled and add the capnp schema files. Here's the branch.
Essentially, we can fuzz the IPC code from the outside with fuzzamoto, covering serialization and interface behavior. This approach did already find a bug so seems promising!
Thanks! This looks really great and I am already dreading the crashes it may uncover, especially since I didn't fix the first one yet.
I was looking at what I think is the main implementation file: ipc_mining.rs and it appears pretty neat and fairly easy to extend and maintain.
I am wondering what next steps may be needed to take this from a "proof-of-concept" to something more final. Are there changes that would make it more maintainable? Are there gaps in coverage that would be good to fix?
dreading the crashes
While we are looking forward to them :)
Are there changes that would make it more maintainable? @dergoegge could speak more to this, but an initial step would probably be to enable fuzzamoto to more easily build the multiprocess binaries. Not having to manually change Dockerfiles and
Cargo.toml. And then also, we could have the various clients separated out from the scenarios themselves (or one general IPC client that wraps the others maybe).
Then it would just be a matter of writing the scenarios. And coming up with more clever approaches than just randomly calling the operations. I wouldn't mind adding some of these things to my to-do list if this is the route we'd like to take.
I'm working on making it easier to deploy custom testing workloads with fuzzamoto, so that something as simple as a different binary name isn't a struggle (i.e. doesn't require patches). See https://github.com/dergoegge/fuzzamoto/issues/48, which will hopefully also enable the use of python with our testing framework. I'll circle back here once that is done.
Opened #35118 as a first step. It adds a stub interface IpcFuzzInterface with round-trip assertions for value types (int32, COutPoint, CScript, std::vector<uint8_t>) across the capnp proxy layer. Callback and interface-passing coverage is a known gap for a follow-up.