net: allow CSubNet of non-IP networks #20852

vasild · 2021-01-04T17:50:17Z

Allow creation of valid CSubNet objects of non-IP networks and only
match the single address they were created from (like /32 for IPv4 or
/128 for IPv6).

This fixes a deficiency in CConnman::DisconnectNode(const CNetAddr& addr)
and in BanMan which assume that creating a subnet from any address
using the CSubNet(CNetAddr) constructor would later match that address
only. Before this change a non-IP subnet would be invalid and would not
match any address.

vasild · 2021-01-04T17:51:54Z

If this is merged, then #20849 will not be needed. That PR is valid with or without this one, but this one fixes the issue more broadly.

src/netaddress.cpp

sipa · 2021-01-05T00:50:03Z

I've given this label "Needs backport (0.21)", as either this or #20849 (or both) should go into rc5. If 20849 is merged and backported, we're good.

sipa · 2021-01-05T00:52:27Z

Test is broken, fix:

diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
index ac4db3a5b6..87d8bf6ef6 100644
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -442,8 +442,7 @@ BOOST_AUTO_TEST_CASE(netbase_dont_resolve_strings_with_embedded_nul_characters)
     BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0"s, ret));
     BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com"s, ret));
     BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com\0"s, ret));
-    // We only do subnetting for IPv4 and IPv6
-    BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion"s, ret));
+    BOOST_CHECK(LookupSubNet("5wyqrzbvrdsumnok.onion"s, ret));
     BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion\0"s, ret));
     BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion\0example.com"s, ret));
     BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion\0example.com\0"s, ret));

DrahtBot · 2021-01-05T05:42:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: simpler setban and new ban manipulation commands #19825 (rpc: simpler setban and new ban manipulation commands by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

laanwj · 2021-01-05T08:31:45Z

Concept/direction ACK, will review the code later

vasild · 2021-01-05T11:11:14Z

06af169...6ab8b2b: minor tweaks, add comments to CSubNet() constructors, fix and extend tests.

maflcko · 2021-01-05T11:41:05Z

 test  2021-01-05T11:36:34.542000Z TestFramework (ERROR): JSONRPC error 
                                   Traceback (most recent call last):
                                     File "/private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin18/test/functional/test_framework/test_framework.py", line 126, in main
                                       self.run_test()
                                     File "/private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin18/test/functional/rpc_setban.py", line 39, in run_test
                                       self.nodes[1].setban("127.0.0.1", "remove")
                                     File "/private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin18/test/functional/test_framework/coverage.py", line 47, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwh0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin18/test/functional/test_framework/authproxy.py", line 146, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: Error: Unban failed. Requested address/subnet was not previously manually banned. (-30)

maflcko · 2021-01-05T11:48:13Z

I am not familiar with this part of the codebase, but it seems safer to move this to 0.21.1. This claims to fix a bug how outbound onion addresses are handled by rpc and misbehavior-disconnect. However, it changes the code paths for all networks and all net handling. So it seems a bit risky to push out this late in the rc stage. (There haven't been any major issues since rc3 other than the macos code signing, so rc5 might very well be a good -final). It could be documented in the release notes, if needed, and a 0.21.1 can be pushed out once this has settled in the master branch (maybe after ~1-2 months)?

jonatack

Approach ACK; this works for me manually with disconnectnode on outbound onion peers (edit: but so does master, the issue isn't hit with it, will look at setban <onion> add) and the netbase_tests run locally, but seeing CI issues.

It would be good to have a failing test case that this patch fixes.

src/netaddress.h

jonatack · 2021-01-05T11:29:20Z

src/netaddress.h

+         * @param[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
+         * @param[in] mask Network mask, must be of the same type as `addr` and not contain 0-bits
+         * followed by 1-bits. Otherwise an invalid subnet is created. Ignored for non-IP `addr`.
+         */


readability suggestion

/** * Construct from a given network start and number of bits (CIDR mask). - * @param[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created. - * @param[in] mask CIDR mask, must be in [0, 32] for IPv4 addresses and in [0, 128] for IPv6 - * addresses. Otherwise an invalid subnet is created. Ignored for non-IP `addr`. + * @param[in] addr Network start. If not IPv4 or IPv6, then a single-host subnet is created. + * @param[in] mask CIDR mask, must be in [0, 32] for IPv4 addresses and in [0, 128] for IPv6 + * addresses. Otherwise an invalid subnet is created. Ignored for non-IP `addr`. */ CSubNet(const CNetAddr& addr, uint8_t mask); /** * Construct from a given network start and mask. - * @param[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created. - * @param[in] mask Network mask, must be of the same type as `addr` and not contain 0-bits - * followed by 1-bits. Otherwise an invalid subnet is created. Ignored for non-IP `addr`. + * @param[in] addr Network start. If not IPv4 or IPv6, then a single-host subnet is created. + * @param[in] mask Network mask, must be of the same type as `addr` and not contain 0-bits + * followed by 1-bits. Otherwise an invalid subnet is created. Ignored for non-IP `addr`. */ CSubNet(const CNetAddr& addr, const CNetAddr& mask);

This 3-column-layout has a maintainability problem when a parameter with longer name is added or existent one renamed.

Yes, though I think it's worth it. Anyway, just a suggestion.

src/netaddress.h

src/netaddress.cpp

vasild · 2021-01-06T16:10:13Z

6ab8b2b...826b8d9: fix failing rpc_setban.py by making sure in-memory representation of CSubNet::netmask[] for IPv4 subnets is the same when deserialized from disk and when created via the constructor.

For IPv4, deserialize only sets the first 4 bytes of the netmask, leaving the remaining 12 bytes to the default 0. operator== compares all the 16 bytes, so we want a deserialized subnet to equal the same subnet that is created via the CSubNet::CSubNet(CNetAddr) constructor. Thus, in that constructor only set the first 4 bytes and leave the remaining 12 ones to the default 0.

Yes, operator== need not compare all the 16 bytes for IPv4, it can/should compare just the first 4. Fixing this by modifying operator== I think goes beyond the purpose of this PR which is to allow non-IP subnets. So I restored the code to only set the first 4 bytes (+ the assert to ensure no overflow). This is the same as in master.

Eventually uint8_t CSubNet::netmask[16] should be changed to uint8_t CSubNet::m_cidr.

vasild · 2021-01-06T16:19:54Z

826b8d9...293070a: fix typos in comments

vasild · 2021-01-06T16:26:29Z

@MarcoFalke

... it changes the code paths for all networks and all net handling ...

Indeed. CSubNet is used in lots of places and changes to it can have subtle effects, as shown by CI failures on this PR.

luke-jr · 2021-01-06T17:14:47Z

src/netaddress.cpp

+    case NET_ONION:
+    case NET_I2P:
+    case NET_CJDNS:
+        valid = true;


We shouldn't just ignore mask... Might be best to only change the single-address form?

Done: #20852 (comment)

jonatack

ACK 293070a code review, manually verified that setban OUTBOUND_TOR_V3_PEER add succeeds with this patch and fails on master, cherry-picked the unit test change in 98140bc that fails on master, and it passes with this patch

vasild · 2021-01-07T12:13:05Z

293070a...1a54bd7: as per suggestion, reverted the two constructors that take also a "netmask" argument to create an invalid subnet, like in master without this PR.

laanwj · 2021-01-07T14:13:49Z

src/netaddress.cpp

+        break;
+    case NET_INTERNAL:
+    case NET_UNROUTABLE:
+    case NET_MAX:
        return;


I think valid is left uninitialized here. It's not initialized in class CSubNet declaration nor here.

It is set to false in the default constructor which is called by this constructor:

CSubNet::CSubNet(const CNetAddr& addr) : CSubNet()

netmask is also set there.

Thanks for illuminating. That's kinda sneaky, would prefer to do initialization in the class declaration itself, but no need to do so in this PR.

Maybe add valid = false; here as belt and suspenders.

vasild · 2021-01-07T14:59:35Z

1a54bd7...5e95ce6: add a functional test that fails without this fix and passes with it

Co-authored-by: Jon Atack <jon@atack.com>

vasild · 2021-01-10T14:55:02Z

5e95ce6...39b4329: extend the functional test, #20852 (comment)

jonatack · 2021-01-11T09:43:34Z

Code review re-ACK 39b4329 per git diff 5e95ce6 39b4329; only change since last review is improvements to the functional test; verified the test fails on master @ 616eace where expected (assert(self.is_banned(node, tor_addr)) fails and unban unfails)

maflcko · 2021-01-11T10:15:31Z

Moved to 0.21.1 milestone according to last weeks meeting. Let me know if this is not applicable.

laanwj · 2021-01-11T10:25:38Z

code review ACK 39b4329

Moved to 0.21.1 milestone according to last weeks meeting. Let me know if this is not applicable.

Agreed.

Allow creation of valid `CSubNet` objects of non-IP networks and only match the single address they were created from (like /32 for IPv4 or /128 for IPv6). This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)` and in `BanMan` which assume that creating a subnet from any address using the `CSubNet(CNetAddr)` constructor would later match that address only. Before this change a non-IP subnet would be invalid and would not match any address. Github-Pull: bitcoin#20852 Rebased-From: 94d335d

Co-authored-by: Jon Atack <jon@atack.com> Github-Pull: bitcoin#20852 Rebased-From: 39b4329

maflcko · 2021-01-11T11:49:09Z

Would be nice to summarize the changes that are expected here. Am I right that this ...

Breaks compatibility of the serialized banfile? I.e. opening a banfile written by this version with a previous version may drop some addresses?
Fixes banning of onion outbounds via rpc.
Fixes misbheavior disconnect of onion outbounds.

I tried testing the changes and restarting the node will corrupt the listbanned output:

./src/bitcoin-cli -signet  listbanned
[
  {
    "address": "::/0",
    "banned_until": 1610451878,
    "ban_created": 1610365478
  }
]

maflcko · 2021-01-11T11:58:54Z

Would probably be good to propose some release notes based on the changes. Added a draft here: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/%2320852-Release-notes-snippet

vasild · 2021-01-11T12:49:40Z

Would be nice to summarize the changes that are expected here. Am I right that this ...
* Breaks compatibility of the serialized banfile? I.e. opening a banfile written by this version with a previous version may drop some addresses?

The serialized format has not been changed. If this version writes a ban entry consisting of the newly allowed tor-one-host-subnet, an old version would deserialize that as an invalid subnet:

bitcoin/src/netaddress.h

Line 518 in 5cce607

SER_READ(obj, if (obj.valid) obj.valid = obj.SanityCheck());

old code, before this PR:

bitcoin/src/netaddress.cpp

Lines 1121 to 1123 in 616eace

    
           bool CSubNet::SanityCheck() const 
        
           { 
        
               if (!(network.IsIPv4() || network.IsIPv6())) return false;

* Fixes banning of onion outbounds via rpc.

Yes.

* Fixes misbheavior disconnect of onion outbounds.

Yes.

I tried testing the changes and restarting the node will corrupt the listbanned output:

This looks strange, how to reproduce?

maflcko · 2021-01-11T12:56:48Z

This looks strange, how to reproduce?

setban foo.onion add, listbanned (correct), restart, listbanned (corrupt)

maflcko · 2021-01-11T14:04:03Z

Steps to reproduce:

# ./src/bitcoind --version && ./src/bitcoind  -signet  -printtoconsole=0 -noconnect & 
Bitcoin Core version v21.99.0-5cce60710

# ./src/bitcoin-cli  -signet listbanned
[
  {
    "address": "::/0",
    "banned_until": 1610459838,
    "ban_created": 1610373438
  }
]

# ./src/bitcoin-cli  -signet setban v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion add
# ./src/bitcoin-cli  -signet setban fij5zbz4dui7tqolfmkvrj6uag7axoliif4dywhj5ghndnusxzlho2yd.onion add
# ./src/bitcoin-cli  -signet listbanned
[
  {
    "address": "::/0",
    "banned_until": 1610459838,
    "ban_created": 1610373438
  },
  {
    "address": "fij5zbz4dui7tqolfmkvrj6uag7axoliif4dywhj5ghndnusxzlho2yd.onion",
    "banned_until": 1610460063,
    "ban_created": 1610373663
  },
  {
    "address": "v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion",
    "banned_until": 1610460060,
    "ban_created": 1610373660
  }
]


# ./src/bitcoin-cli  -signet stop
Bitcoin Core stopping


# ./src/bitcoind  -signet  -printtoconsole=0 -noconnect &


# ./src/bitcoin-cli  -signet listbanned
[
  {
    "address": "::/0",
    "banned_until": 1610459838,
    "ban_created": 1610373438
  }
]

39b4329 test: add test for banning of non-IP addresses (Vasil Dimov) 94d335d net: allow CSubNet of non-IP networks (Vasil Dimov) Pull request description: Allow creation of valid `CSubNet` objects of non-IP networks and only match the single address they were created from (like /32 for IPv4 or /128 for IPv6). This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)` and in `BanMan` which assume that creating a subnet from any address using the `CSubNet(CNetAddr)` constructor would later match that address only. Before this change a non-IP subnet would be invalid and would not match any address. ACKs for top commit: jonatack: Code review re-ACK 39b4329 per `git diff 5e95ce6 39b4329`; only change since last review is improvements to the functional test; verified the test fails on master @ 616eace where expected (`assert(self.is_banned(node, tor_addr))` fails and unban unfails) laanwj: code review ACK 39b4329 Tree-SHA512: 3239b26d0f2fa2d1388b4fdbc1d05ce4ac1980be699c6ec46049409baefcb2006b1e72b889871e2210e897f6725c48e873f68457eea7e6e4958ab4f959d20297

This is a followup to bitcoin#20852 which allowed non-IP subnets, but some of them, e.g. torv3, cannot be serialized in 16 bytes (addrv1) and must use addrv2. This commit changes the format of `banlist.dat` in such a way that old versions (before this commit) will not be able to read a file written by new versions (after this commit).

vasild · 2021-01-11T16:43:44Z

@MarcoFalke the problem you describe above is fixed in #20904, but that requires a banlist.dat format change.

Maybe revert this PR and merge it together with #20904 as one PR?

The problem with bitcoin#20852 is that if a host is banned that cannot be serialized in addrv1 format (e.g. Torv3) it is serialized as a dummy IPv6 (`::`) in `banlist.dat`. So, upon restart, the ban entry is lost (is deserialized as an invalid subnet that matches nothing). This reverts commit 39b4329 from bitcoin#20852.

The problem with bitcoin#20852 is that if a host is banned that cannot be serialized in addrv1 format (e.g. Torv3) it is serialized as a dummy IPv6 (`::`) in `banlist.dat`. So, upon restart, the ban entry is lost (is deserialized as an invalid subnet that matches nothing). This reverts commit 94d335d from bitcoin#20852.

maflcko · 2021-06-23T07:45:14Z

Looks like this was never backported? I guess not worth it at this point.

… fixed in bitcoin#20852

39b4329 test: add test for banning of non-IP addresses (Vasil Dimov) 94d335d net: allow CSubNet of non-IP networks (Vasil Dimov) Pull request description: Allow creation of valid `CSubNet` objects of non-IP networks and only match the single address they were created from (like /32 for IPv4 or /128 for IPv6). This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)` and in `BanMan` which assume that creating a subnet from any address using the `CSubNet(CNetAddr)` constructor would later match that address only. Before this change a non-IP subnet would be invalid and would not match any address. ACKs for top commit: jonatack: Code review re-ACK 39b4329 per `git diff 5e95ce6 39b4329`; only change since last review is improvements to the functional test; verified the test fails on master @ 616eace where expected (`assert(self.is_banned(node, tor_addr))` fails and unban unfails) laanwj: code review ACK 39b4329 Tree-SHA512: 3239b26d0f2fa2d1388b4fdbc1d05ce4ac1980be699c6ec46049409baefcb2006b1e72b889871e2210e897f6725c48e873f68457eea7e6e4958ab4f959d20297

DrahtBot added the P2P label Jan 4, 2021

vasild mentioned this pull request Jan 4, 2021

net: disconnect peers by address without using a subnet #20849

Closed

sipa reviewed Jan 4, 2021

View reviewed changes

src/netaddress.cpp Show resolved Hide resolved

sipa added this to the 0.21.0rc5 milestone Jan 5, 2021

sipa added the Needs backport (0.21) label Jan 5, 2021

DrahtBot mentioned this pull request Jan 5, 2021

rpc: simpler setban and new ban manipulation commands #19825

Closed

vasild force-pushed the non_ip_subnets branch from 06af169 to 6ab8b2b Compare January 5, 2021 11:10

jonatack reviewed Jan 5, 2021

View reviewed changes

vasild force-pushed the non_ip_subnets branch from 6ab8b2b to 826b8d9 Compare January 6, 2021 15:44

vasild force-pushed the non_ip_subnets branch from 826b8d9 to 293070a Compare January 6, 2021 16:19

luke-jr reviewed Jan 6, 2021

View reviewed changes

laanwj modified the milestones: 0.21.0rc5, 0.21.0 Jan 7, 2021

jonatack reviewed Jan 7, 2021

View reviewed changes

vasild force-pushed the non_ip_subnets branch from 293070a to 1a54bd7 Compare January 7, 2021 12:08

laanwj reviewed Jan 7, 2021

View reviewed changes

vasild force-pushed the non_ip_subnets branch from 1a54bd7 to 5e95ce6 Compare January 7, 2021 14:59

test: add test for banning of non-IP addresses

39b4329

Co-authored-by: Jon Atack <jon@atack.com>

vasild force-pushed the non_ip_subnets branch from 5e95ce6 to 39b4329 Compare January 10, 2021 14:54

maflcko modified the milestones: 0.21.0, 0.21.1 Jan 11, 2021

laanwj merged commit 675af2a into bitcoin:master Jan 11, 2021

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jan 11, 2021

test: add test for banning of non-IP addresses

bdce029

Co-authored-by: Jon Atack <jon@atack.com> Github-Pull: bitcoin#20852 Rebased-From: 39b4329

maflcko removed the Needs backport (0.21) label Jan 11, 2021

vasild deleted the non_ip_subnets branch January 11, 2021 12:50

vasild mentioned this pull request Jan 11, 2021

net: (de)serialize CSubNet in addrv2 format #20904

Closed

vasild mentioned this pull request Jan 13, 2021

revert #20852 net: allow CSubNet of non-IP networks #20926

Closed

maflcko mentioned this pull request Jun 23, 2021

banman: save the banlist in a JSON format on disk #20966

Merged

knst added a commit to knst/dash that referenced this pull request Aug 6, 2022

Fix backport bitcoin#17754 due to missing functionality yet. Would be…

c6eeee3

… fixed in bitcoin#20852

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

net: allow CSubNet of non-IP networks #20852

net: allow CSubNet of non-IP networks #20852

Uh oh!

Conversation

vasild commented Jan 4, 2021 • edited by laanwj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jan 4, 2021

Uh oh!

Uh oh!

sipa commented Jan 5, 2021

Uh oh!

sipa commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 5, 2021

Conflicts

Uh oh!

laanwj commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jan 5, 2021

Uh oh!

maflcko commented Jan 5, 2021

Uh oh!

maflcko commented Jan 5, 2021

Uh oh!

jonatack left a comment • 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!

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!

Uh oh!

Uh oh!

vasild commented Jan 6, 2021

Uh oh!

vasild commented Jan 6, 2021

Uh oh!

vasild commented Jan 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 7, 2021

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!

vasild commented Jan 7, 2021

Uh oh!

vasild commented Jan 10, 2021

Uh oh!

jonatack commented Jan 11, 2021

Uh oh!

maflcko commented Jan 11, 2021

Uh oh!

laanwj commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 11, 2021

Uh oh!

maflcko commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jan 11, 2021

vasild commented Jan 4, 2021 •

edited by laanwj

Loading

sipa commented Jan 5, 2021 •

edited

Loading

laanwj commented Jan 5, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading

laanwj commented Jan 11, 2021 •

edited

Loading

maflcko commented Jan 11, 2021 •

edited

Loading