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.
1156+ case NET_CJDNS:
1157+ case NET_INTERNAL:
1158+ case NET_UNROUTABLE:
1159+ case NET_MAX:
1160+ return network.ToString();
1161+ }
Test is broken, fix:
0diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
1index ac4db3a5b6..87d8bf6ef6 100644
2--- a/src/test/netbase_tests.cpp
3+++ b/src/test/netbase_tests.cpp
4@@ -442,8 +442,7 @@ BOOST_AUTO_TEST_CASE(netbase_dont_resolve_strings_with_embedded_nul_characters)
5 BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0"s, ret));
6 BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com"s, ret));
7 BOOST_CHECK(!LookupSubNet("1.2.3.0/24\0example.com\0"s, ret));
8- // We only do subnetting for IPv4 and IPv6
9- BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion"s, ret));
10+ BOOST_CHECK(LookupSubNet("5wyqrzbvrdsumnok.onion"s, ret));
11 BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion\0"s, ret));
12 BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion\0example.com"s, ret));
13 BOOST_CHECK(!LookupSubNet("5wyqrzbvrdsumnok.onion\0example.com\0"s, ret));
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
CSubNet() constructors, fix and extend tests.
467+ */
468 CSubNet();
469+
470+ /**
471+ * Construct from a given network start and number of bits (CIDR mask).
472+ * @param[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
478+ /**
479+ * Construct from a given network start and mask.
480+ * @param[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
481+ * @param[in] mask Network mask, must be of the same type as `addr` and not contain 0-bits
482+ * followed by 1-bits. Otherwise an invalid subnet is created. Ignored for non-IP `addr`.
483+ */
readability suggestion
0 /**
1 * Construct from a given network start and number of bits (CIDR mask).
2- * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
3- * [@param](/bitcoin-bitcoin/contributor/param/)[in] mask CIDR mask, must be in [0, 32] for IPv4 addresses and in [0, 128] for IPv6
4- * addresses. Otherwise an invalid subnet is created. Ignored for non-IP `addr`.
5+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a single-host subnet is created.
6+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] mask CIDR mask, must be in [0, 32] for IPv4 addresses and in [0, 128] for IPv6
7+ * addresses. Otherwise an invalid subnet is created. Ignored for non-IP `addr`.
8 */
9 CSubNet(const CNetAddr& addr, uint8_t mask);
10
11 /**
12 * Construct from a given network start and mask.
13- * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
14- * [@param](/bitcoin-bitcoin/contributor/param/)[in] mask Network mask, must be of the same type as `addr` and not contain 0-bits
15- * followed by 1-bits. Otherwise an invalid subnet is created. Ignored for non-IP `addr`.
16+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a single-host subnet is created.
17+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] mask Network mask, must be of the same type as `addr` and not contain 0-bits
18+ * followed by 1-bits. Otherwise an invalid subnet is created. Ignored for non-IP `addr`.
19 */
20 CSubNet(const CNetAddr& addr, const CNetAddr& mask);
484 CSubNet(const CNetAddr& addr, const CNetAddr& mask);
485
486- //constructor for single ip subnet (<ipv4>/32 or <ipv6>/128)
487+ /**
488+ * Construct a single-host subnet.
489+ * @param[in] addr the sole address to be contained in the subnet.
readability suggestion
0 * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr The sole address to be contained in the subnet.
0 test 2021-01-05T11:36:34.542000Z TestFramework (ERROR): JSONRPC error
1 Traceback (most recent call last):
2 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
3 self.run_test()
4 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
5 self.nodes[1].setban("127.0.0.1", "remove")
6 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__
7 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
8 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__
9 raise JSONRPCException(response['error'], status)
10 test_framework.authproxy.JSONRPCException: Error: Unban failed. Requested address/subnet was not previously manually banned. (-30)
1118+ case NET_UNROUTABLE:
1119+ case NET_MAX:
1120 return;
1121 }
1122
1123- assert(addr.m_addr.size() <= sizeof(netmask));
Should we keep this assert?
0 case NET_IPV4:
1 case NET_IPV6:
2 valid = true;
3+ assert(addr.m_addr.size() <= sizeof(netmask));
4 memset(netmask, 0xFF, sizeof(netmask));
5 break;
6 case NET_ONION:
The assert was there because in master we wrote addr.m_addr.size() bytes and here it is changed to write sizeof(netmask) bytes so it can’t overflow and thus I removed the assert.
However, I changed/restored it to write addr.m_addr.size() bytes and also restored the assert, explanation.
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.
6ab8b2b5d…826b8d9eb: 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.
… 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.
1006+ valid = mask <= ADDR_IPV6_SIZE * 8;
1007+ break;
1008+ case NET_ONION:
1009+ case NET_I2P:
1010+ case NET_CJDNS:
1011+ valid = true;
mask… Might be best to only change the single-address form?
setban OUTBOUND_TOR_V3_PEER add succeeds with this patch and fails on master, cherry-picked the unit test change in 98140bc855b037d2c552153496e8e4e2c2efdad9 that fails on master, and it passes with this patch
master without this PR.
1083+ valid = true;
1084+ break;
1085+ case NET_INTERNAL:
1086+ case NET_UNROUTABLE:
1087+ case NET_MAX:
1088 return;
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.
valid = false; here as belt and suspenders.
48+ for ban_entry in self.nodes[1].listbanned():
49+ if ban_entry['address'] == tor_addr:
50+ is_banned = True
51+ break
52+ assert(is_banned)
53+ self.nodes[1].setban(tor_addr, "remove")
The test sees only one entry in listbanned, the added one. If it is moved above the previous test, there will also be a second entry in listbanned, e.g.
0[
1 {'address': '127.0.0.1/32', ...},
2 {'address': 'pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion', ...}
3]
Here’s a suggestion to do that, add logging, and also assert pre-add and post-remove (with the same number of lines):
0+TOR_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
1+
2 class SetBanTests(BitcoinTestFramework):
3
4+ def banned_tor_addr(self, *, tor_addr=TOR_ADDR):
5+ return list(filter(lambda ban_entry: ban_entry["address"] == tor_addr, self.nodes[1].listbanned()))
6+
7 def run_test(self):
8@@ -35,6 +40,13 @@ class SetBanTests(BitcoinTestFramework):
9 peerinfo = self.nodes[1].getpeerinfo()[0]
10 assert('noban' in peerinfo['permissions'])
11
12+ self.log.info("Test that a Tor v3 address can be banned/unbanned")
13+ assert(not self.banned_tor_addr())
14+ self.nodes[1].setban(TOR_ADDR, "add")
15+ assert(self.banned_tor_addr())
16+ self.nodes[1].setban(TOR_ADDR, "remove")
17+ assert(not self.banned_tor_addr())
18+
19 # If we remove the ban, Node 0 should be able to reconnect even without noban permission
20@@ -42,15 +54,5 @@ class SetBanTests(BitcoinTestFramework):
21 assert(not 'noban' in peerinfo['permissions'])
22
23- tor_addr = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
24- self.nodes[1].setban(tor_addr, "add")
25- is_banned = False
26- for ban_entry in self.nodes[1].listbanned():
27- if ban_entry['address'] == tor_addr:
28- is_banned = True
29- break
30- assert(is_banned)
31- self.nodes[1].setban(tor_addr, "remove")
32
33 if __name__ == '__main__':
ACK 5e95ce6797a286aa23022141188f9139315bff70 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 98140bc855b037d2c552153496e8e4e2c2efdad9 that fails on master, and it passes with this patch.
Happy to re-review if you update per comments above.
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.
Co-authored-by: Jon Atack <jon@atack.com>
git diff 5e95ce6 39b4329; only change since last review is improvements to the functional test; verified the test fails on master @ 616eace0 where expected (assert(self.is_banned(node, tor_addr)) fails and unban unfails)
code review ACK 39b43298d9c54f9c18bef36f3d5934f57aefd088
Moved to 0.21.1 milestone according to last weeks meeting. Let me know if this is not applicable.
Agreed.
Would be nice to summarize the changes that are expected here. Am I right that this …
I tried testing the changes and restarting the node will corrupt the listbanned output:
0./src/bitcoin-cli -signet listbanned
1[
2 {
3 "address": "::/0",
4 "banned_until": 1610451878,
5 "ban_created": 1610365478
6 }
7]
8
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
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:
old code, before this PR: https://github.com/bitcoin/bitcoin/blob/616eace02acdc5f9ce775f6bf82fc8964980162f/src/netaddress.cpp#L1121-L1123
* 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
listbannedoutput:
This looks strange, how to reproduce?
This looks strange, how to reproduce?
setban foo.onion add, listbanned (correct), restart, listbanned (corrupt)
Steps to reproduce:
0# ./src/bitcoind --version && ./src/bitcoind -signet -printtoconsole=0 -noconnect &
1Bitcoin Core version v21.99.0-5cce60710
2
3# ./src/bitcoin-cli -signet listbanned
4[
5 {
6 "address": "::/0",
7 "banned_until": 1610459838,
8 "ban_created": 1610373438
9 }
10]
11
12# ./src/bitcoin-cli -signet setban v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion add
13# ./src/bitcoin-cli -signet setban fij5zbz4dui7tqolfmkvrj6uag7axoliif4dywhj5ghndnusxzlho2yd.onion add
14# ./src/bitcoin-cli -signet listbanned
15[
16 {
17 "address": "::/0",
18 "banned_until": 1610459838,
19 "ban_created": 1610373438
20 },
21 {
22 "address": "fij5zbz4dui7tqolfmkvrj6uag7axoliif4dywhj5ghndnusxzlho2yd.onion",
23 "banned_until": 1610460063,
24 "ban_created": 1610373663
25 },
26 {
27 "address": "v7ajjeirttkbnt32wpy3c6w3emwnfr3fkla7hpxcfokr3ysd3kqtzmqd.onion",
28 "banned_until": 1610460060,
29 "ban_created": 1610373660
30 }
31]
32
33
34# ./src/bitcoin-cli -signet stop
35Bitcoin Core stopping
36
37
38# ./src/bitcoind -signet -printtoconsole=0 -noconnect &
39
40
41# ./src/bitcoin-cli -signet listbanned
42[
43 {
44 "address": "::/0",
45 "banned_until": 1610459838,
46 "ban_created": 1610373438
47 }
48]
@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?