fuzz-tests: Add a test for fundee_channel()
#8411
Conversation
tests/fuzz/Makefile
Outdated
| @@ -72,5 +72,17 @@ FUZZ_COMMON_OBJS := \ | |||
| $(FUZZ_TARGETS_OBJS): $(COMMON_HEADERS) $(WIRE_HEADERS) $(COMMON_SRC) | |||
| $(FUZZ_TARGETS_BIN): $(LIBFUZZ_OBJS) $(FUZZ_COMMON_OBJS) $(BITCOIN_OBJS) | |||
|
|
|||
| tests/fuzz/fuzz-open_channel: common/shutdown_scriptpubkey.o \ | |||
There was a problem hiding this comment.
Nit: let's put this rule up by the other target-specific ones
tests/fuzz/fuzz-open_channel.c
Outdated
| { | ||
| struct state *state = talz(ctx, struct state); | ||
|
|
||
| state->developer = fromwire_bool(cursor, max); |
There was a problem hiding this comment.
We should always set developer = false, since we're only interested in bugs that happen in production.
tests/fuzz/fuzz-open_channel.c
Outdated
| = state->upfront_shutdown_script[REMOTE] | ||
| = NULL; | ||
|
|
||
| fromwire_bitcoin_outpoint(cursor, max, &state->funding); |
There was a problem hiding this comment.
Why are we setting the funding outpoint here? Shouldn't this be set later by the funding_create message?
| &state->minimum_depth, | ||
| &state->min_feerate, &state->max_feerate, | ||
| &state->dev_force_tmp_channel_id, | ||
| &state->allowdustreserve, |
There was a problem hiding this comment.
This is probably one immediate cause of the crash -- this lets the fuzzer set allowdustreserve = true, which bypasses the checks needed to sanitize dust limits and channel reserves.
allowdustreserve is a developer option that we should probably always set to false for this test.
| u8 remaining_len = fromwire_u8(cursor, max); | ||
| if (remaining_len > *max) | ||
| remaining_len = *max; | ||
| towire_u8_array(&openingd_init_msg, *cursor, remaining_len); |
There was a problem hiding this comment.
I think we need to be more careful than to just populate state with random values. At the very least there are several developer options we should probably disable. And there might be other constraints we need to satisfy.
There was a problem hiding this comment.
I agree on the point that we should turn off the developer options, but I think letting the fuzzer decide as much of the state as it can helps discover more code paths than a fixed set of values would. It also doesn't result in a crash in the latest push, so I think it's worth constructing the message like this.
| } | ||
|
|
||
| static u8 *create_open_channel_msg(const tal_t *ctx, const u8 **cursor, size_t *max, struct state *state) | ||
| { |
There was a problem hiding this comment.
I don't think we should be doing any of the validation in this function. The open_channel message comes from the peer, which means it could contain anything. We should basically use raw fuzzer-generated data for the message.
There was a problem hiding this comment.
Right, that's what I did initially but that approach took too long for the fuzzer to reach deeper code paths in the target function. By doing it this way, the fuzzer can achieve new coverage quicker while still retaining the ability to tweak the message.
There was a problem hiding this comment.
Maybe we could use one input byte to decide whether we do the validation or not. Then we get the best of both worlds -- 50% of the time we do random values in case a crash can be found that way and 50% of the time we do validated values to fuzz deeper.
| /* These have the same definitions as the original peer_reed and peer_write. | ||
| * We reiterate these here because we want them to use test_sync_write and | ||
| * test_sync_read. | ||
| */ |
There was a problem hiding this comment.
Since we're overriding peer_read and peer_write, we can just call test_sync_read and test_sync_write directly here for clarity.
tests/fuzz/fuzz-open_channel.c
Outdated
| struct amount_sat reserve = amount_sat(100); | ||
| u32 mindepth = 10; | ||
| return towire_openingd_got_offer_reply(ctx, NULL, NULL, NULL, | ||
| &reserve, mindepth); |
There was a problem hiding this comment.
Maybe we should just use reserve=NULL for now, which will use the default of 1% of the channel capacity.
| assert(false && "Too many HSMD writes!"); | ||
| } | ||
| else if (fd == PEER_FD) | ||
| { |
There was a problem hiding this comment.
It looks like we could also get in the case from negotiation_failed, and in that scenario there wouldn't be an accept_channel message to send.
tests/fuzz/fuzz-open_channel.c
Outdated
| return towire_funding_created(ctx, &state->channel_id, | ||
| &state->funding.txid, state->funding.n, &sig.s); |
There was a problem hiding this comment.
funding_created is a message sent by the peer and could contain anything. Ideally we would use fuzzer-generated data for this message, so we can check if the peer can crash us from this message.
In fact it looks like the peer can send other non-funding-created messages to fundee_channel as well, which we should also try here.
Of course to fuzz deeper in the state machine we also want valid funding_created messages to be created sometimes. So there's an opportunity for some optimization here.
| static u8 *create_fuzz_msg(const tal_t *ctx) | ||
| { |
There was a problem hiding this comment.
handle_peer_in only ever calls fundee_channel with open_channel messages. So we should manually prefix with WIRE_OPEN_CHANNEL for that case.
tests/fuzz/fuzz-open_channel.c
Outdated
| static u8 *create_fuzz_msg(const tal_t *ctx) | ||
| { | ||
| u8 *msg = tal_arr(ctx, u8, 0); | ||
| u8 msg_len = fromwire_u8(cursor, max); |
There was a problem hiding this comment.
Reading one byte here gives a max len of 255. I don't think that's enough for any valid open_channel message: https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-open_channel-message.
Probably we need to read 2 bytes for the length.
tests/fuzz/fuzz-open_channel.c
Outdated
|
|
||
| u8 *open_channel_msg; | ||
| /* Choose between creating a valid message and a fuzzed one. */ | ||
| if (fromwire_bool(cursor, max)) |
There was a problem hiding this comment.
Looks like fromwire_bool reads one byte and then marks the cursor as failed if the byte isn't exactly 0 or 1. That means 99% of the time this will fail...
Probably we should write our own helper for fuzzing that reads one byte and then just checks the final bit to determine true or false.
| return towire_funding_created(ctx, &state->channel_id, | ||
| &state->funding.txid, state->funding.n, &sig.s); |
There was a problem hiding this comment.
Looks like state->funding is uninitialized here.
|
The latest push triggers the |
| open_channel_msg = tal_arr(tmpctx, u8, 0); | ||
| towire_u16(&open_channel_msg, WIRE_OPEN_CHANNEL); | ||
| towire_u8_array(&open_channel_msg, fuzz_msg, tal_bytelen(fuzz_msg)); |
There was a problem hiding this comment.
To avoid the extra copy here, we can add a msg type param to create_fuzz_msg, so it can directly prepend before copying the data bytes.
There was a problem hiding this comment.
create_fuzz_msg() is also used in test_sync_write() when fd == PEER_FD so I don't think it would be right to do this.
| &htlcs, | ||
| &feerate)); | ||
| } | ||
| else |
There was a problem hiding this comment.
We also need to handle HSM messages for signature validation in validate_initial_commitment_signature.
| u8 channel_flags; | ||
| u8 *shutdown_scriptpubkey; | ||
|
|
||
| assert(fromwire_openingd_got_offer(tmpctx, msg, |
There was a problem hiding this comment.
It is also possible that we get here from negotiation_aborted in opening_negotiate_msg, in which case this assertion will fail.
I looked into this. It seems that |
Changelog-None: `fundee_channel()` in `openingd/openingd.c` is responsible for handling incoming `open_channel` messages from a peer. Since it deals with external input, add a test for it.
Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.
This does seem to fix the issue at hand but while I was trying to fix the HSMD issues above, I ran into the following error: which seems to occur due to a malformed transaction by |
fundee_channel()inopeningd/openingd.cis responsible for handling incomingopen_channelmessages from a peer. Since it deals with external input, add a test for it.Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked: