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 | + }
Control reaches end of non-void function.
Fixed.
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));
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Concept/direction ACK, will review the code later
06af169cf...6ab8b2b5d: minor tweaks, add comments to 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.
s/signle/single/ here and line 480
Fixed.
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
/**
* Construct from a given network start and number of bits (CIDR mask).
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
- * [@param](/bitcoin-bitcoin/contributor/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](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a single-host subnet is created.
+ * [@param](/bitcoin-bitcoin/contributor/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](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a signle-host subnet is created.
- * [@param](/bitcoin-bitcoin/contributor/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](/bitcoin-bitcoin/contributor/param/)[in] addr Network start. If not IPv4 or IPv6, then a single-host subnet is created.
+ * [@param](/bitcoin-bitcoin/contributor/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.
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
* [@param](/bitcoin-bitcoin/contributor/param/)[in] addr The sole address to be contained in the subnet.
Took only the capital, without the space :)
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)
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?
case NET_IPV4:
case NET_IPV6:
valid = true;
+ assert(addr.m_addr.size() <= sizeof(netmask));
memset(netmask, 0xFF, sizeof(netmask));
break;
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.
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)?
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.
826b8d9eb...293070adc: fix typos in comments
... 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;
We shouldn't just ignore mask... Might be best to only change the single-address form?
Done: #20852 (comment)
ACK 293070adc34f979bee1e9315cc70973a168c3af2 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
293070adc...1a54bd7d0: as per suggestion, reverted the two constructors that take also a "netmask" argument to create an invalid subnet, like in master without this PR.
1083 | + valid = true; 1084 | + break; 1085 | + case NET_INTERNAL: 1086 | + case NET_UNROUTABLE: 1087 | + case NET_MAX: 1088 | 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.
1a54bd7d0...5e95ce679: add a functional test that fails without this fix and passes with it
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.
[
{'address': '127.0.0.1/32', ...},
{'address': 'pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion', ...}
]
Here's a suggestion to do that, add logging, and also assert pre-add and post-remove (with the same number of lines):
+TOR_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
+
class SetBanTests(BitcoinTestFramework):
+ def banned_tor_addr(self, *, tor_addr=TOR_ADDR):
+ return list(filter(lambda ban_entry: ban_entry["address"] == tor_addr, self.nodes[1].listbanned()))
+
def run_test(self):
@@ -35,6 +40,13 @@ class SetBanTests(BitcoinTestFramework):
peerinfo = self.nodes[1].getpeerinfo()[0]
assert('noban' in peerinfo['permissions'])
+ self.log.info("Test that a Tor v3 address can be banned/unbanned")
+ assert(not self.banned_tor_addr())
+ self.nodes[1].setban(TOR_ADDR, "add")
+ assert(self.banned_tor_addr())
+ self.nodes[1].setban(TOR_ADDR, "remove")
+ assert(not self.banned_tor_addr())
+
# If we remove the ban, Node 0 should be able to reconnect even without noban permission
@@ -42,15 +54,5 @@ class SetBanTests(BitcoinTestFramework):
assert(not 'noban' in peerinfo['permissions'])
- tor_addr = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
- self.nodes[1].setban(tor_addr, "add")
- is_banned = False
- for ban_entry in self.nodes[1].listbanned():
- if ban_entry['address'] == tor_addr:
- is_banned = True
- break
- assert(is_banned)
- self.nodes[1].setban(tor_addr, "remove")
if __name__ == '__main__':
Added something like that in 39b43298d, thanks!
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>
5e95ce679...39b43298d: extend the functional test, #20852 (review)
Code review re-ACK 39b43298d9c54f9c18bef36f3d5934f57aefd088 per 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)
Moved to 0.21.1 milestone according to last weeks meeting. Let me know if this is not applicable.
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:
./src/bitcoin-cli -signet listbanned
[
{
"address": "::/0",
"banned_until": 1610451878,
"ban_created": 1610365478
}
]
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
<!-- export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin && git checkout master && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev libsqlite3-dev libdb++-dev -y && ./autogen.sh && ./configure --disable-wallet --with-gui=no --disable-tests --disable-bench && make -j 9 && apt install -y tor valgrind vim /etc/tor/torrc valgrind ./src/bitcoind -signet -printtoconsole=1 -debug=net -onlynet=onion
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:
# ./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
}
]
@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?
Looks like this was never backported? I guess not worth it at this point.
Milestone
0.21.1