fuzz-tests: Add a test for the gossipd-connectd interface by Chand-ra · Pull Request #8423 · ElementsProject/lightning

Chand-ra · 2025-07-22T12:19:37Z

Changelog-None: connectd_req() in gossipd/gossipd.c is responsible for handling gossip messages from peers handed to it by connectd. Add a stateful test simulating its behaviour.

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:

The changelog has been updated in the relevant commit(s) according to the guidelines.
Tests have been added or modified to reflect the changes.
Documentation has been reviewed and updated as needed.
Related issues have been listed and linked, including any that this PR closes.

CC: @morehouse

Chand-ra · 2025-07-22T12:21:19Z

The test results in the following LeakSanitizer error when run on its corpus:

==116428==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x619eb51001f3 in malloc (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2da1f3) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #1 0x619eb54deb3d in split_node /home/chandra/lightning/ccan/ccan/intmap/intmap.c:67:9
    #2 0x619eb54de8c2 in intmap_add_ /home/chandra/lightning/ccan/ccan/intmap/intmap.c:113:9
    #3 0x619eb53122d1 in map_add /home/chandra/lightning/gossipd/gossmap_manage.c:176:6
    #4 0x619eb530f8c6 in gossmap_manage_channel_announcement /home/chandra/lightning/gossipd/gossmap_manage.c:640:8
    #5 0x619eb53b2101 in handle_recv_gossip /home/chandra/lightning/tests/fuzz/../../gossipd/gossipd.c:205:12
    #6 0x619eb53a93b2 in run /home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd.c:543:4
    #7 0x619eb513e8f8 in LLVMFuzzerTestOneInput /home/chandra/lightning/tests/fuzz/libfuzz.c:25:2
    #8 0x619eb504c0c4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2260c4) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #9 0x619eb504b7b9 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2257b9) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #10 0x619eb504d3d6 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2273d6) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #11 0x619eb504d8e7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2278e7) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #12 0x619eb503addf in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x214ddf) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #13 0x619eb5065466 in main (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x23f466) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)
    #14 0x7aed7c22a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #15 0x7aed7c22a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #16 0x619eb502fdc4 in _start (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x209dc4) (BuildId: 34398486c14ff41d79a297932e621cb4f6161418)

SUMMARY: AddressSanitizer: 40 byte(s) leaked in 1 allocation(s).

INFO: a leak has been found in the initial corpus.

morehouse

High level review: I like the target. Needs some rework so we consider exit to be a crash.

Haven't studied create_gossip_msg yet.

morehouse · 2025-07-28T18:06:03Z

tests/fuzz/fuzz-gossipd-connectd.c

+			struct node_id id = node_id(privkey_from_index(tal_count(peer_ids)));
+			tal_arr_expand(&peer_ids, id);
+
+			msg = towire_gossipd_new_peer(tmpctx, &id, false);


Setting gossip_queries_feature = false will limit some of the code paths that can be executed, especially all the seeker stuff. If there's a good reason to do this, please add a comment here that explains why.

morehouse · 2025-07-28T18:24:46Z

tests/fuzz/fuzz-gossipd-connectd.c

+			switch (fromwire_u8(&data, &size) % 5)
+			{
+				case 0:
+					gossip_msg = create_gossip_msg(tmpctx, &data, &size, WIRE_CHANNEL_ANNOUNCEMENT);
+					break;
+				case 1:
+					gossip_msg = create_gossip_msg(tmpctx, &data, &size, WIRE_CHANNEL_UPDATE);
+					break;
+				case 2:
+					gossip_msg = create_gossip_msg(tmpctx, &data, &size, WIRE_NODE_ANNOUNCEMENT);
+					break;
+				case 3:
+					gossip_msg = create_gossip_msg(tmpctx, &data, &size, WIRE_REPLY_CHANNEL_RANGE);
+					break;
+				case 4:
+					gossip_msg = create_gossip_msg(tmpctx, &data, &size, WIRE_REPLY_SHORT_CHANNEL_IDS_END);
+					break;
+				default:
+					break;
+			}


Nit: perhaps this switch could go inside create_gossip_msg, so there's one less parameter to pass.

morehouse · 2025-07-28T19:41:41Z

tests/fuzz/fuzz-gossipd-connectd.c

+
+cleanup:
+	if (daemon)
+		tal_free(daemon->connectd);


Is the explicit tal_free necessary? Naively it looks like clean_tmpctx should take care of this already.

This is to circumvent the dangling allocation error fixed in #8424 .

morehouse · 2025-07-28T19:43:36Z

tests/fuzz/fuzz-gossipd-connectd.c

+	if (setjmp(exit_jmp) != 0)
+		goto cleanup;


For gossipd, we actually shouldn't consider exit to be normal. If gossipd exits, the entire CLN node shuts down, which would be a DoS vulnerability.

This path can only be triggered by status_failed() and towire_warningfmt().

I can get rid of the mock for towire_warningfmt() by including common/wire_error.o for linking. As for status_failed(), I don't think triggering it would mean a vulnerability, it's used pretty liberally throughout gossipd/gossipd.c to report a bad message from the peer. Maybe exit()'s behavior manifests differently in a live node than it does in our fuzzer (where it aborts the entire process).

I don't see any reason for us to quit the fuzz target for towire_warning_fmt.

status_failed means "print error and exit", and should never happen in the wild. If it does, it's a DoS vulnerability.

I don't see any reason for us to quit the fuzz target for towire_warning_fmt.

Right, I think I didn't word that clearly. Right now we're mocking the behavior of towire_warning_fmt() which we don't need to, by simply including common/wire_error.o for linking along with the other artifacts. I've already done this in the latest push and it doesn't seem to result in any crash.

status_failed means "print error and exit", and should never happen in the wild. If it does, it's a DoS vulnerability.

Oh okay, makes sense.

morehouse · 2025-07-28T21:34:39Z

The test results in the following LeakSanitizer error when run on its corpus:

I looked closer at this. The accused line of code does a naked malloc (no tal use), which makes me suspect there could be a cleanup problem on each iteration of the fuzz target, since only tal-allocated memory gets freed.

We might need to manually delete all elements from the gossmap at the end of each iteration.

Chand-ra · 2025-07-29T10:06:47Z

We might need to manually delete all elements from the gossmap at the end of each iteration.

This makes the target a bit messier but does seem to manage to evade the issue at hand.

Chand-ra · 2025-07-30T10:46:38Z

I was fixing some of the other issues with this target when it ran into yet another memory leak:

==31969==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x57fcd5d431f3 in malloc (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2da1f3) (BuildId: 844798d4ffd40334b8c2f0d73bff69c289ab8025)
    #1 0x57fcd612359d in split_node /home/chandra/lightning/ccan/ccan/intmap/intmap.c:67:9
    #2 0x57fcd6123322 in intmap_add_ /home/chandra/lightning/ccan/ccan/intmap/intmap.c:113:9
    #3 0x57fcd5f52a16 in add_unknown_scid /home/chandra/lightning/gossipd/seeker.c:695:7
    #4 0x57fcd5f52830 in query_unknown_channel /home/chandra/lightning/gossipd/seeker.c:1159:2
    #5 0x57fcd5fd8f95 in process_channel_update /home/chandra/lightning/tests/fuzz/../../gossipd/gossmap_manage.c:806:3
    #6 0x57fcd5fd6e3d in gossmap_manage_channel_update /home/chandra/lightning/tests/fuzz/../../gossipd/gossmap_manage.c:983:9
    #7 0x57fcd5ff04be in handle_recv_gossip /home/chandra/lightning/tests/fuzz/../../gossipd/gossipd.c:210:12
    #8 0x57fcd5fe708c in run /home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd.c:515:4
    #9 0x57fcd5d818f8 in LLVMFuzzerTestOneInput /home/chandra/lightning/tests/fuzz/libfuzz.c:25:2
    #10 0x57fcd5c8f0c4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/chandra/lightning/tests/fuzz/fuzz-gossipd-connectd+0x2260c4) (BuildId: 844798d4ffd40334b8c2f0d73bff69c289ab8025)
...
...
...

This time, the leak occurs when we try to send a channel_update message for a channel that the peer hasn't already received a channel_announcement for. CLN asks the seeker to query the sending peer for a channel_announcement with query_unknown_channel() which then tried to add the unknown peer to seeker->unknown_scids using uintmap_add() which causes the leak.

morehouse · 2025-07-30T15:14:59Z

This time, the leak occurs when we try to send a channel_update message for a channel that the peer hasn't already received a channel_announcement for. CLN asks the seeker to query the sending peer for a channel_announcement with query_unknown_channel() which then tried to add the unknown peer to seeker->unknown_scids using uintmap_add() which causes the leak.

Then we also need to manually free that map at the end of each iteration.

morehouse

Have you been able to run this fuzz target? I tried and hit a bunch of crashes immediately.

Only reproduces when I use multiple workers, and seems multiple workers are trying to touch the same file (yikes).

fuzz-gossipd-connectd: gossip_store_compact: rename failed: No such file or dire
ctory (version v25.05-2-gda8afec-modded)                                        
==174417== ERROR: libFuzzer: fuzz target exited                                 
    #7 0x00000057ce3c in status_failed common/status.c:208:2
    #8 0x000000526e9c in gossip_store_compact gossipd/gossip_store.c:360:3 
    #9 0x000000526e9c in gossip_store_new gossipd/gossip_store.c:404:11
    #10 0x0000005ef290 in setup_gossmap tests/fuzz/../../gossipd/gossmap_manage.c:453:11
    #11 0x0000005eed07 in gossmap_manage_new tests/fuzz/../../gossipd/gossmap_manage.c:485:7
    #12 0x0000005f8381 in new_daemon tests/fuzz/fuzz-gossipd-connectd.c:128:15
    #13 0x0000005f8381 in run tests/fuzz/fuzz-gossipd-connectd.c:472:26

I'll review further once all shallow crashes are fixed.

morehouse · 2025-08-05T19:31:31Z

tests/fuzz/fuzz-gossipd-connectd.c

+	if (setjmp(exit_jmp) != 0)
+		goto cleanup;


The jmp_buf is now not needed. Please remove.

morehouse · 2025-08-05T19:36:51Z

tests/fuzz/fuzz-gossipd-connectd.c

+				channel_flags = 1;
+			}
+
+			timestamp = time_now().ts.tv_sec - 3600;


The peer could send any timestamp. It doesn't have to be recent. Perhaps we should let the fuzzer decide the timestamp.

Right, that's how I designed the fuzzer initially:

timestamp = fromwire_u32(cursor, max);

and how the NODE_ANNOUNCEMENT message is crafted, but the fuzzer was unable to get past this check so I swapped that out for the current setup. Maybe something like

timestamp = time_now().ts.tv_sec - fromwire_u16(cursor, max);

would be better?

There's an additional problem with using time_now -- it could make reproducibility difficult. Perhaps we should be using dev_gossip_time instead.

I would prefer all timestamps to be possible. I think it would be better to do something like 75% of the time choose a valid one but 25% of the time the fuzzer can choose whatever random values it wants.

Chand-ra · 2025-08-06T11:40:34Z

Have you been able to run this fuzz target? I tried and hit a bunch of crashes immediately.

Only reproduces when I use multiple workers, and seems multiple workers are trying to touch the same file (yikes).

Yeah, we discussed this over our meeting a while ago, and we decided upon creating a new file for each fuzz run. I tried to do that but the file name needs to be statically defined using #define GOSSIP_STORE_FILENAME otherwise the rename() here starts complaining.

The next best thing I could muster up was resetting the gossip_store file with unlink(GOSSIP_STORE_FILENAME) at the start of each run.

morehouse · 2025-08-06T21:33:49Z

tests/fuzz/fuzz-gossipd-connectd.c

+				channel_flags = 1;
+			}
+
+			timestamp = time_now().ts.tv_sec - - fromwire_u16(cursor, max);


Does this even compile? Please fix.

morehouse · 2025-08-06T21:38:46Z

tests/fuzz/fuzz-gossipd-connectd.c

+				channel_flags = 1;
+			}
+
+			timestamp = time_now().ts.tv_sec - 3600;


There's an additional problem with using time_now -- it could make reproducibility difficult. Perhaps we should be using dev_gossip_time instead.

I would prefer all timestamps to be possible. I think it would be better to do something like 75% of the time choose a valid one but 25% of the time the fuzzer can choose whatever random values it wants.

morehouse · 2025-08-06T21:54:35Z

Yeah, we discussed this over our meeting a while ago, and we decided upon creating a new file for each fuzz run. I tried to do that but the file name needs to be statically defined using #define GOSSIP_STORE_FILENAME otherwise the rename() here starts complaining.

Ugh, that's tricky. It kind of seems like this fuzz target would do better with an out-of-process fuzzing engine.

We can try to hack around the issue though. Maybe we could redefine GOSSIP_STORE_FILENAME to be some function call get_gossip_store_filename(), which then returns a thread-local temp file name.

Chand-ra · 2025-08-08T13:12:12Z

We can try to hack around the issue though. Maybe we could redefine GOSSIP_STORE_FILENAME to be some function call get_gossip_store_filename(), which then returns a thread-local temp file name.

In C, only adjacent string literals are concatenated at compile time, so the GOSSIP_STORE_FILENAME has to be a string literal. I tried using a function as a substitute but get a compile error at the expansion:

rename(get_gossip_store_filename(), get_gossip_store_filename() ".corrupt")

Changelon-None: `connectd_req()` in `gossipd/gossipd.c` is responsible for handling gossip messages from peers handed to it by `connectd`. Add a stateful test simulating its behaviour.

Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.

morehouse · 2025-08-08T21:20:29Z

In C, only adjacent string literals are concatenated at compile time, so the GOSSIP_STORE_FILENAME has to be a string literal. I tried using a function as a substitute but get a compile error at the expansion:

For now we could just change that location to use tal_strcat. This would allow the fuzz target to actually work, so we can run it and see if we find any bugs. If the target finds bugs, then it's probably worth changing that location permanently so this we can use this fuzz target can serve as a regression test.

Chand-ra · 2025-08-09T09:33:03Z

For now we could just change that location to use tal_strcat. This would allow the fuzz target to actually work, so we can run it and see if we find any bugs. If the target finds bugs, then it's probably worth changing that location permanently so this we can use this fuzz target can serve as a regression test.

I ran the target for quite some time and it doesn't seem to be able to discover any newer vulnerabilities.

rustyrussell · 2025-11-10T05:13:07Z

I'm deferring this one, as it will need significant rework for the rebase. In particular, we are going to have a way of overriding entropy, so this should use that.

Chand-ra mentioned this pull request Jul 22, 2025

common: fix dangling memory allocation in daemon_conn_new_() #8424

Merged

4 tasks

morehouse suggested changes Jul 28, 2025

View reviewed changes

Chand-ra force-pushed the gossipd-connectd branch from 448e689 to 4721eb2 Compare July 29, 2025 10:16

Chand-ra force-pushed the gossipd-connectd branch from 4721eb2 to da8afec Compare August 1, 2025 14:52

morehouse suggested changes Aug 5, 2025

View reviewed changes

Chand-ra force-pushed the gossipd-connectd branch from da8afec to ab7dc45 Compare August 6, 2025 11:45

morehouse reviewed Aug 6, 2025

View reviewed changes

Chandra Pratap added 2 commits August 8, 2025 13:12

fuzz-tests: Add a test for the gossipd-connectd interface

5e175bd

Changelon-None: `connectd_req()` in `gossipd/gossipd.c` is responsible for handling gossip messages from peers handed to it by `connectd`. Add a stateful test simulating its behaviour.

fuzz-tests: Add a seed corpus for the new test

8dd0ec8

Add a minimal input set as a seed corpus for the newly introduced test. This leads to discovery of interesting code paths faster.

Chand-ra force-pushed the gossipd-connectd branch from ab7dc45 to 8dd0ec8 Compare August 8, 2025 13:22

rustyrussell added this to the v25.12 milestone Nov 7, 2025

rustyrussell modified the milestones: v25.12, v26.03 Nov 10, 2025

Conversation

Chand-ra commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Chand-ra commented Jul 22, 2025

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse commented Jul 28, 2025

Uh oh!

Chand-ra commented Jul 29, 2025

Uh oh!

Chand-ra commented Jul 30, 2025

Uh oh!

morehouse commented Jul 30, 2025

Uh oh!

morehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chand-ra commented Aug 6, 2025

Uh oh!

morehouse Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morehouse commented Aug 6, 2025

Uh oh!

Chand-ra commented Aug 8, 2025

Uh oh!

morehouse commented Aug 8, 2025

Uh oh!

Chand-ra commented Aug 9, 2025

Uh oh!

rustyrussell commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Chand-ra commented Jul 22, 2025 •

edited

Loading

morehouse Aug 6, 2025 •

edited

Loading