fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses #26859

vasild commented at 5:14 PM on January 9, 2023: contributor

In the process of doing so, refactor ConsumeNetAddr() to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in RandAddr().

DrahtBot commented at 5:14 PM on January 9, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, brunoerg, achow101
Concept ACK	dergoegge, jonatack, chinggg
Approach ACK	w0xlt
Stale ACK	stratospher

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Jan 9, 2023

in src/test/fuzz/addrman.cpp:69 in 0c8daaf752 outdated

  81 | -
  82 | -        s << net;
  83 | -        s << fast_random_context.randbytes(net_len_map.at(net));
  84 | -
  85 | -        s >> addr;
  86 | +        addr = ConsumeNetAddr(fuzzed_data_provider, &fast_random_context);

dergoegge commented at 6:00 PM on January 9, 2023:

I don't think consuming addresses from FastRandomContext has much upside in fuzzing. Especially after this PR since ConsumeNetAddr will now produce addrs of any kind. I think you were the author of RandAddr so maybe you can elaborate on why you added this back then.

vasild commented at 6:27 PM on January 9, 2023:

Yes, because the addrman tests (that use RandAddr()) need many many many addresses. If all are generated from the fuzzer input, they exhaust it very quickly.

vasild commented at 9:24 AM on January 10, 2023:

Here is the relevant discussion wrt generating addresses from FastRandomContext: #21129 (review)

vasild commented at 10:18 AM on January 10, 2023:

Added a comment to ConsumeNetAddr() to explain this.

dergoegge commented at 10:20 AM on January 10, 2023:

Ah I see, thanks for linking!

I'm still skeptical that consuming from FastRandomContext is useful in this context and I would speculate that leaving the addrs blank or providing some valid dummies is probably just as useful.

In any case, I guess this isn't really the concern of this PR. I was about to suggest that you add a comment to ConsumeNetAddr stating that for most targets not consuming from FastRandomContext is preferable but I see that you just did that on the latest push :)

vasild commented at 10:30 AM on January 10, 2023:

The test that fills addrman needs different addresses. This is why leaving them blank (e.g. 0.0.0.0) does not suffice. FastRandomContext is a convenient way to generate "unlimited" amount of (mostly) valid, different, dummies ;-)

mzumsande commented at 11:59 PM on January 31, 2023:

To add to this, the idea of addrman_serdeser is to be able to fill up a large fraction of AddrMan, for which we need a large variety of addresses because similar addresses will collide with each other.

The "(mostly) valid," is changed by this a little bit though: I think we'll now fill addrman with less addresses because 1 in 6 addresses will be of type NET_INTERNAL which is invalid and not accepted to AddrMan.

vasild commented at 2:11 PM on February 2, 2023:

RandAddr() could have returned NET_INTERNAL also before (if it called ConsumeNetAddr()), but you are right that this has little value because near the end RandAddr() will convert any invalid to always-the-same-5.5.5.5. I added a few retries to try to avoid that.

mzumsande commented at 8:35 PM on February 2, 2023:

oh, right, i missed that. Retrying a few times makes sense to me.

dergoegge commented at 6:02 PM on January 9, 2023: member

Concept ACK

vasild force-pushed on Jan 10, 2023

vasild commented at 10:17 AM on January 10, 2023: contributor

0c8daaf752...46a7c30871: fix the build on 32 bit archs and add a comment

w0xlt commented at 5:53 PM on January 10, 2023: contributor

Approach ACK

mzumsande commented at 11:59 PM on January 31, 2023: contributor

Concept ACK

It would be good to also update the netaddress fuzz target with the added networks. At the minimum, we could check / assert IsI2P() for Network::NET_I2P there, maybe we could also assert that addresses from Tor, I2P and CJDNS are always valid, which I believe is guaranteed by the creation process?

vasild force-pushed on Feb 2, 2023

vasild commented at 2:06 PM on February 2, 2023: contributor

46a7c30871...74fea1e088: expand fuzz/netaddress.cpp with I2P and CJDNS, suggested above. Retry a few times to get a valid address inside RandAddr(), discussion.

in src/test/fuzz/util/net.cpp:76 in 74fea1e088 outdated

  91 | +    } else {
  92 | +        data = rand->randbytes(aux.len);
  93 | +    }
  94 | +    s << data;
  95 | +
  96 | +    s >> addr;

mzumsande commented at 8:30 PM on February 2, 2023:

I tried to understand the CI fail: The process of unserializing can change the network into NET_UNROUTABLE, for example if a CJDNS address with the wrong prefix was generated. As a result of that, the fuzz/banman target can hit https://github.com/bitcoin/bitcoin/blob/7753efcbcf3ca2e1b46f167936e11cc580e28c7a/src/test/fuzz/banman.cpp#L112 because while converting the unroutable address to a CSubNet, no netmask is added as for IPv6, but after serializing / unserializing via banlist, it's no longer seen as unroutable but IPv6 and gets a /128 netmask.

mzumsande commented at 11:29 PM on February 2, 2023:

Slightly unrelated, but while looking into this I got the impression that banning CJDNS peers doesn't work at all currently (no MaybeFlipIPv6toCJDNS). I might try to add this possibility in another PR.

vasild commented at 2:41 PM on February 3, 2023:

Alright, this PR uncovered a problem elsewhere!

You are right in your analysis (except that NET_UNROUTABLE is not involved): the test bans CNetAddr{.m_net = NET_CJDNS, m_addr = 91b4:919d...} (that is !IsValid() because it does not start with fc). Then it gets serialized in fuzzed_banlist.json like:

        {   
            "version": 1,
            "ban_created": 2984688890,
            "banned_until": 3982933363,
            "address": "91b4:919d:1191:9140:0:c4:207f:7f00"
        }

and then of course, after unserialize ends up as an IPv6 address in banmap_read. This is something I have overlooked wrt CJDNS and banlist.json.

So, two issues:

How to handle an attempt to ban an address that is !IsValid()?
- Refuse and change BanMan::Ban() to return bool / failure, or
- Don't change anything, leave it as it is now.
How to survive CJDNS addresses through banman ser/unser aka banlist.json?
- Save the network explicitly in a new field in the JSON, or
- During unserialize maybe flip an IPv6 address that starts with fc to CJDNS type if -cjdnsreachable (but this would mean the contents of banlist.json could be interpreted differently based on the environment where it is loaded).

mzumsande commented at 9:31 PM on February 3, 2023:

I think that the fuzz test will not only fail on invalid addresses, it will also fail on valid CJDNS addresses for the same reason. The important thing is that IPv6 addresses will get the /128 netmask, other networks won't, and if the address, valid or not, is re-interpreted as IPv6 after unserialization of banlist.json, the assert will fail.

During unserialize maybe flip an IPv6 address that starts with fc to CJDNS type if -cjdnsreachable (but this would mean the contents of banlist.json could be interpreted differently based on the environment where it is loaded).

I had this in mind, and I verified that banning CJDNS peers will work that way. The different interpretation may not a huge problem, because regular IPv6 peers can't be from that reserved range, so there would be no risk of banning the wrong peer after disabling -cjdnsreachable. On the other hand, we would need some logic to deal with the fact that when interpreted as IPv6, the address gets a /128 mask and switching back to -cjdnsreachable won't work anymore, so maybe the cleanest solution would be to save the network explicitly?

vasild commented at 11:21 AM on February 10, 2023:

This turned out to be somewhat involved change. Opened a PR at #27071.

vasild commented at 9:29 AM on October 31, 2023:

Resolving this, given that #27071 has now been merged.

vasild commented at 11:23 AM on February 10, 2023: contributor

Don't merge this before the problems it uncovered are fixed by #27071 (the CI is failing anyway).

jonatack commented at 9:22 PM on February 14, 2023: member

Concept ACK. Reviewed prerequisite PR #27071.

chinggg commented at 4:44 AM on February 15, 2023: contributor

Concept ACK

achow101 marked this as a draft on Apr 25, 2023

DrahtBot added the label Needs rebase on Sep 7, 2023

vasild force-pushed on Sep 8, 2023

vasild commented at 11:42 AM on September 8, 2023: contributor

74fea1e088...8acdbb5a22: rebase due to conflicts

This PR extends the tests which uncovers a bug. The CI will likely fail due to that bug. The fix of the bug is pending review at #27071.

DrahtBot removed the label Needs rebase on Sep 8, 2023

DrahtBot added the label CI failed on Sep 10, 2023

DrahtBot added the label Needs rebase on Sep 15, 2023

achow101 referenced this in commit 0655e9dd92 on Oct 19, 2023

vasild force-pushed on Oct 31, 2023

vasild commented at 9:59 AM on October 31, 2023: contributor

8acdbb5a22...722fdf14a7: rebase and take out of draft because #27071 has been merged. Ready for review.

vasild marked this as ready for review on Oct 31, 2023

DrahtBot removed the label Needs rebase on Oct 31, 2023

vasild force-pushed on Oct 31, 2023

DrahtBot removed the label CI failed on Oct 31, 2023

in src/test/fuzz/util/net.cpp:37 in 790f0cb60d outdated

  33 | -    if (network == Network::NET_IPV4) {
  34 | -        in_addr v4_addr = {};
  35 | -        v4_addr.s_addr = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
  36 | -        net_addr = CNetAddr{v4_addr};
  37 | -    } else if (network == Network::NET_IPV6) {
  38 | -        if (fuzzed_data_provider.remaining_bytes() >= 16) {

stratospher commented at 6:09 AM on November 29, 2023:

Doesn't affect this PR, but wondering why previously an empty CNetAddr was returned when there weren't sufficient bytes in FuzzedDataProvider for ipv6 and not for other networks.

vasild commented at 8:39 AM on November 29, 2023:

Good observation! I guess there was no special reason for that inconsistency. Further, it would check if there are >=16 bytes available, but then would ConsumeBytes<uint8_t>(16) and ConsumeIntegral<uint32_t>() - that is actually 20 bytes. This was introduced in 815c7a67931.

stratospher commented at 6:12 AM on November 29, 2023: contributor

tested ACK 790f0cb.

DrahtBot requested review from mzumsande on Nov 29, 2023

DrahtBot requested review from dergoegge on Nov 29, 2023

DrahtBot requested review from jonatack on Nov 29, 2023

DrahtBot requested review from w0xlt on Nov 29, 2023

DrahtBot removed review request from w0xlt on Nov 29, 2023

DrahtBot requested review from w0xlt on Nov 29, 2023

DrahtBot removed review request from w0xlt on Nov 29, 2023

DrahtBot requested review from w0xlt on Nov 29, 2023

DrahtBot added the label CI failed on Jan 13, 2024

vasild commented at 3:57 PM on January 17, 2024: contributor

This tests extension was stalled because it uncovered a bug. Now the bug is fixed via #27071. @dergoegge, @mzumsande, @jonatack, @chinggg - you did Concept ACK, maybe you want to review it?

DrahtBot removed review request from w0xlt on Jan 17, 2024

DrahtBot requested review from w0xlt on Jan 17, 2024

mzumsande commented at 10:43 PM on January 19, 2024: contributor

This doesn't play well with recent changes made in #27935: That PR added the line

const std::optional<CNetAddr>& addr{LookupHost(net_addr.ToStringAddr(), /*fAllowLookup=*/false)};

to fuzz/banman.cpp , after which a randomly generated CJDNS address would be changed to IPv6, and then the fuzz target fails. It takes just a few seconds / minutes for the fuzzer to crash. So it would be good to rebase this branch and fix that issue (e.g. by just changing the addr back to CJDNS again).

DrahtBot removed review request from w0xlt on Jan 19, 2024

DrahtBot requested review from w0xlt on Jan 19, 2024

DrahtBot requested review from mzumsande on Jan 19, 2024

DrahtBot removed review request from w0xlt on Jan 19, 2024

DrahtBot requested review from w0xlt on Jan 19, 2024

vasild commented at 4:30 PM on January 22, 2024: contributor

To reproduce: Base64: IAEgdZ/fIAAAAAAaAAQAAUlJSUlZAAAAAALfAAAAGgAEAAH/+/5lAAAAgAAAIwCAKQ==. @mzumsande, right! What about this fix (git diff -w):

--- i/src/test/fuzz/banman.cpp
+++ w/src/test/fuzz/banman.cpp
@@ -67,18 +67,20 @@ FUZZ_TARGET(banman, .init = initialize_banman)
         LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
         {   
             CallOneOf(
                 fuzzed_data_provider,
                 [&] {
                     CNetAddr net_addr{ConsumeNetAddr(fuzzed_data_provider)};
+                    if (!net_addr.IsCJDNS()) {
                         const std::optional<CNetAddr>& addr{LookupHost(net_addr.ToStringAddr(), /*fAllowLookup=*/false
                         if (addr.has_value() && addr->IsValid()) {
                             net_addr = *addr;
                         } else {
                             contains_invalid = true;
                         }
+                    }
                     ban_man.Ban(net_addr, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool
                 },
                 [&] {
                     CSubNet subnet{ConsumeSubNet(fuzzed_data_provider)};

@brunoerg?

DrahtBot removed review request from w0xlt on Jan 22, 2024

DrahtBot requested review from w0xlt on Jan 22, 2024

brunoerg commented at 10:27 AM on January 23, 2024: contributor

Concept ACK

DrahtBot removed review request from w0xlt on Jan 23, 2024

DrahtBot requested review from w0xlt on Jan 23, 2024

brunoerg commented at 10:31 AM on January 23, 2024: contributor

@brunoerg?

Sounds good to me. I didn't check the code yet but is the CJDNS address from ConsumeNetAddr always valid?

DrahtBot removed review request from w0xlt on Jan 23, 2024

DrahtBot requested review from w0xlt on Jan 23, 2024

DrahtBot removed review request from w0xlt on Jan 23, 2024

DrahtBot requested review from w0xlt on Jan 23, 2024

fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses

In the process of doing so, refactor `ConsumeNetAddr()` to generate the
addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way -
by preparing some random stream and deserializing from it. Similar code
was already found in `RandAddr()`.

b851c5385d

vasild force-pushed on Jan 23, 2024

vasild commented at 10:54 AM on January 23, 2024: contributor

790f0cb60d...b851c5385d: rebase and adjust fuzz/banman.cpp due to #26859#pullrequestreview-1833881229.

is the CJDNS address from ConsumeNetAddr always valid

Yes, I think so. Anyway, for safety wrt future changes I added IsValid() to the check. That is - omit lookup for valid CJDNS addresses.

DrahtBot removed the label CI failed on Jan 23, 2024

mzumsande commented at 11:24 PM on January 25, 2024: contributor

ACK b851c5385d0a0acec4493be1561cea285065d5dc

I let the banman fuzzer run for a while, and it didn't crash anymore.

DrahtBot removed review request from w0xlt on Jan 25, 2024

DrahtBot requested review from w0xlt on Jan 25, 2024

DrahtBot requested review from stratospher on Jan 25, 2024

DrahtBot requested review from brunoerg on Jan 25, 2024

brunoerg commented at 1:02 AM on January 26, 2024: contributor

utACK b851c5385d0a0acec4493be1561cea285065d5dc

DrahtBot removed review request from w0xlt on Jan 26, 2024

DrahtBot requested review from w0xlt on Jan 26, 2024

achow101 commented at 9:35 PM on January 31, 2024: member

ACK b851c5385d0a0acec4493be1561cea285065d5dc

DrahtBot removed review request from w0xlt on Jan 31, 2024

DrahtBot requested review from w0xlt on Jan 31, 2024

achow101 merged this on Jan 31, 2024

achow101 closed this on Jan 31, 2024

vasild deleted the branch on Feb 1, 2024

in src/netaddress.h:267 in b851c5385d

 260 | @@ -261,6 +261,18 @@ class CNetAddr
 261 |          }
 262 |      }
 263 |  
 264 | +    /**
 265 | +     * BIP155 network ids recognized by this software.
 266 | +     */
 267 | +    enum BIP155Network : uint8_t {

jonatack commented at 5:02 PM on February 2, 2024:

Nice, making BIP155Network a public enum facilitates updating #27385.

jonatack commented at 5:02 PM on February 2, 2024: member

Post-merge ACK b851c5385d0a0acec4493be1561cea285065d5dc

bitcoin locked this on Feb 1, 2025