fuzz-tests: Add a wire test for `struct wireaddr` #8324

Chand-ra · 2025-06-06T05:56:21Z

Wire functions in common/wireaddr.{c, h} consume untrusted input. Since no tests exist for these functions, add one for the wire operations of struct wireaddr.

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.

Chand-ra · 2025-06-06T06:31:15Z

This PR is draft because a corpus for the new test is yet to be added. This isn't possible for now because of the bug which is fixed in PR #8325 .

Chand-ra · 2025-07-23T11:07:29Z

The corpus for this test was created by applying the fix in #8325, so that PR probably needs to be merged before this one can.

morehouse · 2025-07-25T18:45:29Z

Do the wireaddress_internal functions actually consume untrusted input? I grepped for parse_wireaddr_internal and found three callers:

lightningd/connect_control.c - uses params passed via RPC
lightningd/options.c - uses params passed on the command line
wallet/wallet.c - uses params loaded from the DB

I don't see an obvious way for an attacker to influence any of these interfaces.

Chand-ra · 2025-07-28T13:47:27Z

Do the wireaddress_internal functions actually consume untrusted input?

Hmm, it doesn't look like any of the wireaddr_internal functions consume any type of untrusted input other than fmt_wireaddr_internal() which isn't currently tested here.

I modified this target to test fmt_wireaddr_internal() and it runs into an ASan heap-buffer-overflow error. I think this is happening because the address in parse_wireaddr() isn't formatted correctly (not NULL-terminated) in case of a DNS address of length 255, but I'm not too sure.

morehouse · 2025-07-29T13:43:41Z

Sounds like a potential bug. But it's unclear to me if it would be triggerable from the outside, since the fuzz target currently uses parse_wireaddr_internal to create the wireaddr, which always consumes trusted input.

Chand-ra · 2025-07-29T17:50:29Z

Sounds like a potential bug. But it's unclear to me if it would be triggerable from the outside, since the fuzz target currently uses parse_wireaddr_internal to create the wireaddr, which always consumes trusted input.

There seems to be a couple of instances where an attacker could potentially trigger this bug. websocket_connection_in() in connectd.c seems to be an easily reachable target when a new incoming websocket connection is established. try_connect_addr() and destroy_io_conn() in the same file appear to be reachable from external messages as well.

morehouse · 2025-07-29T18:35:49Z

If the buffer overflow is triggered from DNS addresses, it doesn't seem triggerable from the outside then.

websocket_connection_in uses get_remote_address, which doesn't generate DNS addresses.
try_connect_one_addr and destroy_io_conn operate on connect->addrs, which AFAICT never contain DNS addresses.

But it would still be good to investigate and make sure. And the buffer overflow should be fixed regardless.

Chand-ra · 2025-07-30T09:42:44Z

The crash actually occurs in fmt_wireaddr() which is called by fmt_wireaddr_internal(). The other wireaddr bug that we found some time ago was also a bug in fromwire_wireaddr() which also happened to get triggered here because the wireaddr_internal function uses the wireaddr function internally. Maybe it's better to drop this target altogether and merge a similar one for wireaddr instead?

morehouse · 2025-07-30T15:11:16Z

Yes, I'd prefer a target for the wireaddr functions since those definitely consume untrusted input.

Chand-ra · 2025-07-30T18:05:00Z

This target triggers this bug when run on its corpus, and as I suspected, it's due to parse_wireaddr() not appending a NULL terminator at the end of a DNS address of length 255. I think the best way to solve this issue would be to make this change:

diff --git a/common/wireaddr.h b/common/wireaddr.h
index efa0e1f16..780e0df82 100644
--- a/common/wireaddr.h
+++ b/common/wireaddr.h
@@ -45,7 +45,7 @@ enum wire_addr_type {
 struct wireaddr {
        enum wire_addr_type type;
        u8 addrlen;
-       u8 addr[LARGEST_ADDRLEN];
+       u8 addr[LARGEST_ADDRLEN + 1];
        u16 port;
 };

This would happen to fix the issue in #8325 as well, so maybe we should replace that fix with this one. What do you think @morehouse ?

morehouse · 2025-07-31T16:36:29Z

tests/fuzz/fuzz-wire-wireaddr.c

+    common_setup("fuzzer");
+	chainparams = chainparams_for_network("bitcoin");


Needs a clang-format.

morehouse · 2025-07-31T16:44:57Z

tests/fuzz/fuzz-wire-wireaddr.c

+    memcpy(addr, data, size);
+    addr[size] = '\0';
+
+    struct wireaddr *wa = tal(tmpctx, struct wireaddr);


I think wa can just be allocated on the stack.

morehouse · 2025-07-31T16:53:51Z

tests/fuzz/fuzz-wire-wireaddr.c

+        towire_wireaddr(&output_buffer, wa);
+        size_t len = tal_bytelen(output_buffer);
+
+        struct wireaddr *decoded_wa = tal(tmpctx, struct wireaddr);


decoded_wa can also live on the stack.

morehouse · 2025-07-31T17:04:01Z

tests/fuzz/fuzz-wire-wireaddr.c

@@ -0,0 +1,44 @@
+#include "config.h"


Nit: this target is quite different from the other fuzz-wire-wireaddr targets, which all fuzz code in wire/.

Let's just call this fuzz-wireaddr.

morehouse · 2025-07-31T17:07:39Z

This target triggers this bug when run on its corpus,

I tried to verify this, but it didn't trigger any bugs:

$ ../fuzz-wire-wireaddr -runs=0 ../corpora/fuzz-wire-wireaddr/
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1832893554
INFO: Loaded 1 modules   (25514 inline 8-bit counters): 25514 [0x8ce758, 0x8d4b02), 
INFO: Loaded 1 PC tables (25514 PCs): 25514 [0x8d4b08,0x9385a8), 
INFO:      177 files found in ../corpora/fuzz-wire-wireaddr/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 177 min: 1b max: 491b total: 11918b rss: 38Mb
#16     pulse  cov: 186 ft: 361 corp: 10/18b exec/s: 3 rss: 39Mb
#32     pulse  cov: 210 ft: 411 corp: 16/50b exec/s: 3 rss: 39Mb
#64     pulse  cov: 228 ft: 453 corp: 33/264b exec/s: 7 rss: 40Mb
#128    pulse  cov: 229 ft: 503 corp: 44/620b exec/s: 14 rss: 41Mb
#178    INITED cov: 243 ft: 520 corp: 53/2699b exec/s: 19 rss: 42Mb
#178    DONE   cov: 243 ft: 520 corp: 53/2699b lim: 491 exec/s: 19 rss: 42Mb
Done 178 runs in 9 second(s)

Changelog-None: `towire_wireaddr()` and `fromwire_wireaddr()` in `common/wireaddr.h` are responsible for marshalling/unmarshalling BOLT ElementsProject#7 address descriptors. Since these aren't tested by the existing wire fuzz tests, add a roundtrip test for them. This has the added benefit of testing `parse_wireaddr()` as well.

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-05T20:25:11Z

This target triggers this bug when run on its corpus,

Still can't reproduce the new ASan report you saw. After patching in #8325:

$ ../fuzz-wireaddr -runs=0 ../corpora/fuzz-wireaddr/
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 834839632
INFO: Loaded 1 modules   (25510 inline 8-bit counters): 25510 [0x8ce6f8, 0x8d4a9
e), 
INFO: Loaded 1 PC tables (25510 PCs): 25510 [0x8d4aa0,0x938500), 
INFO:      188 files found in ../corpora/fuzz-wireaddr/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4
096 bytes
INFO: seed corpus: files: 188 min: 1b max: 491b total: 13685b rss: 38Mb
#2      pulse  ft: 131 exec/s: 0 rss: 38Mb
#4      pulse  cov: 170 ft: 222 corp: 2/2b exec/s: 1 rss: 39Mb
#8      pulse  cov: 185 ft: 327 corp: 5/6b exec/s: 2 rss: 39Mb
#16     pulse  cov: 190 ft: 389 corp: 11/22b exec/s: 2 rss: 39Mb
#32     pulse  cov: 214 ft: 439 corp: 18/60b exec/s: 2 rss: 39Mb
#64     pulse  cov: 231 ft: 483 corp: 36/284b exec/s: 5 rss: 40Mb
#128    pulse  cov: 233 ft: 510 corp: 50/709b exec/s: 10 rss: 41Mb
#189    INITED cov: 247 ft: 545 corp: 60/2914b exec/s: 15 rss: 42Mb
#189    DONE   cov: 247 ft: 545 corp: 60/2914b lim: 489 exec/s: 15 rss: 42Mb
Done 189 runs in 12 second(s)

Chand-ra mentioned this pull request Jun 6, 2025

common/wireaddr: Fix an out-of-bounds bug in the address parser #8325

Open

4 tasks

Chand-ra marked this pull request as ready for review July 23, 2025 11:05

Chand-ra force-pushed the wireaddr_internal branch from 5114134 to 731d88e Compare July 28, 2025 13:48

Chand-ra force-pushed the wireaddr_internal branch from 731d88e to 1020631 Compare July 30, 2025 18:03

Chand-ra changed the title ~~fuzz-tests: Add a wire test for wireaddr_internal~~ fuzz-tests: Add a wire test for struct wireaddr Jul 30, 2025

morehouse suggested changes Jul 31, 2025

View reviewed changes

Chandra Pratap added 2 commits August 1, 2025 10:12

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

43cb03f

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 wireaddr_internal branch from 1020631 to 43cb03f Compare August 1, 2025 10:12

		common_setup("fuzzer");
		chainparams = chainparams_for_network("bitcoin");

fuzz-tests: Add a wire test for struct wireaddr #8324

Are you sure you want to change the base?

fuzz-tests: Add a wire test for struct wireaddr #8324

Conversation

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

Uh oh!

Checklist

Uh oh!

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

Uh oh!

Uh oh!

Chand-ra commented Jul 23, 2025

Uh oh!

morehouse commented Jul 25, 2025

Uh oh!

Chand-ra commented Jul 28, 2025

Uh oh!

morehouse commented Jul 29, 2025

Uh oh!

Chand-ra commented Jul 29, 2025

Uh oh!

morehouse commented Jul 29, 2025

Uh oh!

Chand-ra commented Jul 30, 2025

Uh oh!

morehouse commented Jul 30, 2025

Uh oh!

Chand-ra commented Jul 30, 2025

Uh oh!

morehouse Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

morehouse commented Jul 31, 2025

Uh oh!

morehouse commented Aug 5, 2025

Uh oh!

Uh oh!

fuzz-tests: Add a wire test for `struct wireaddr` #8324

fuzz-tests: Add a wire test for `struct wireaddr` #8324

Chand-ra commented Jun 6, 2025 •

edited

Loading

Chand-ra commented Jun 6, 2025 •

edited

Loading