diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2021-01-11 11:27:42 +0100 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2021-01-11 11:27:47 +0100 |
commit | 675af2a515f25d96a6e3bfd4f824334f21dd6376 (patch) | |
tree | b804ae4ec0855b73f3e42a2b955c64ec86553f91 | |
parent | 616eace02acdc5f9ce775f6bf82fc8964980162f (diff) | |
parent | 39b43298d9c54f9c18bef36f3d5934f57aefd088 (diff) | |
download | bitcoin-675af2a515f25d96a6e3bfd4f824334f21dd6376.tar.xz |
Merge #20852: net: allow CSubNet of non-IP networks
39b43298d9c54f9c18bef36f3d5934f57aefd088 test: add test for banning of non-IP addresses (Vasil Dimov)
94d335da7f8232bc653c9b08b0a33b517b4c98ad 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 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)
laanwj:
code review ACK 39b43298d9c54f9c18bef36f3d5934f57aefd088
Tree-SHA512: 3239b26d0f2fa2d1388b4fdbc1d05ce4ac1980be699c6ec46049409baefcb2006b1e72b889871e2210e897f6725c48e873f68457eea7e6e4958ab4f959d20297
-rw-r--r-- | src/netaddress.cpp | 81 | ||||
-rw-r--r-- | src/netaddress.h | 24 | ||||
-rw-r--r-- | src/test/netbase_tests.cpp | 21 | ||||
-rwxr-xr-x | test/functional/rpc_setban.py | 16 |
4 files changed, 123 insertions, 19 deletions
diff --git a/src/netaddress.cpp b/src/netaddress.cpp index b1f9d32d34..85e46fd373 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -1068,15 +1068,24 @@ CSubNet::CSubNet(const CNetAddr& addr, const CNetAddr& mask) : CSubNet() CSubNet::CSubNet(const CNetAddr& addr) : CSubNet() { - valid = addr.IsIPv4() || addr.IsIPv6(); - if (!valid) { + switch (addr.m_net) { + case NET_IPV4: + case NET_IPV6: + valid = true; + assert(addr.m_addr.size() <= sizeof(netmask)); + memset(netmask, 0xFF, addr.m_addr.size()); + break; + case NET_ONION: + case NET_I2P: + case NET_CJDNS: + valid = true; + break; + case NET_INTERNAL: + case NET_UNROUTABLE: + case NET_MAX: return; } - assert(addr.m_addr.size() <= sizeof(netmask)); - - memset(netmask, 0xFF, addr.m_addr.size()); - network = addr; } @@ -1088,6 +1097,21 @@ bool CSubNet::Match(const CNetAddr &addr) const { if (!valid || !addr.IsValid() || network.m_net != addr.m_net) return false; + + switch (network.m_net) { + case NET_IPV4: + case NET_IPV6: + break; + case NET_ONION: + case NET_I2P: + case NET_CJDNS: + case NET_INTERNAL: + return addr == network; + case NET_UNROUTABLE: + case NET_MAX: + return false; + } + assert(network.m_addr.size() == addr.m_addr.size()); for (size_t x = 0; x < addr.m_addr.size(); ++x) { if ((addr.m_addr[x] & netmask[x]) != network.m_addr[x]) { @@ -1099,18 +1123,35 @@ bool CSubNet::Match(const CNetAddr &addr) const std::string CSubNet::ToString() const { - assert(network.m_addr.size() <= sizeof(netmask)); + std::string suffix; - uint8_t cidr = 0; + switch (network.m_net) { + case NET_IPV4: + case NET_IPV6: { + assert(network.m_addr.size() <= sizeof(netmask)); - for (size_t i = 0; i < network.m_addr.size(); ++i) { - if (netmask[i] == 0x00) { - break; + uint8_t cidr = 0; + + for (size_t i = 0; i < network.m_addr.size(); ++i) { + if (netmask[i] == 0x00) { + break; + } + cidr += NetmaskBits(netmask[i]); } - cidr += NetmaskBits(netmask[i]); + + suffix = strprintf("/%u", cidr); + break; + } + case NET_ONION: + case NET_I2P: + case NET_CJDNS: + case NET_INTERNAL: + case NET_UNROUTABLE: + case NET_MAX: + break; } - return network.ToString() + strprintf("/%u", cidr); + return network.ToString() + suffix; } bool CSubNet::IsValid() const @@ -1120,7 +1161,19 @@ bool CSubNet::IsValid() const bool CSubNet::SanityCheck() const { - if (!(network.IsIPv4() || network.IsIPv6())) return false; + switch (network.m_net) { + case NET_IPV4: + case NET_IPV6: + break; + case NET_ONION: + case NET_I2P: + case NET_CJDNS: + return true; + case NET_INTERNAL: + case NET_UNROUTABLE: + case NET_MAX: + return false; + } for (size_t x = 0; x < network.m_addr.size(); ++x) { if (network.m_addr[x] & ~netmask[x]) return false; diff --git a/src/netaddress.h b/src/netaddress.h index cf878fe374..b9eade7fd5 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -462,11 +462,33 @@ class CSubNet bool SanityCheck() const; public: + /** + * Construct an invalid subnet (empty, `Match()` always returns false). + */ CSubNet(); + + /** + * Construct from a given network start and number of bits (CIDR mask). + * @param[in] addr Network start. Must be IPv4 or IPv6, otherwise an invalid 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. + */ CSubNet(const CNetAddr& addr, uint8_t mask); + + /** + * Construct from a given network start and mask. + * @param[in] addr Network start. Must be IPv4 or IPv6, otherwise an invalid 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. + */ CSubNet(const CNetAddr& addr, const CNetAddr& mask); - //constructor for single ip subnet (<ipv4>/32 or <ipv6>/128) + /** + * Construct a single-host subnet. + * @param[in] addr The sole address to be contained in the subnet, can also be non-IPv[46]. + */ explicit CSubNet(const CNetAddr& addr); bool Match(const CNetAddr &addr) const; diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp index ac4db3a5b6..66ad7bb5ea 100644 --- a/src/test/netbase_tests.cpp +++ b/src/test/netbase_tests.cpp @@ -226,8 +226,22 @@ BOOST_AUTO_TEST_CASE(subnet_test) // IPv4 address with IPv6 netmask or the other way around. BOOST_CHECK(!CSubNet(ResolveIP("1.1.1.1"), ResolveIP("ffff::")).IsValid()); BOOST_CHECK(!CSubNet(ResolveIP("::1"), ResolveIP("255.0.0.0")).IsValid()); - // Can't subnet TOR (or any other non-IPv4 and non-IPv6 network). - BOOST_CHECK(!CSubNet(ResolveIP("5wyqrzbvrdsumnok.onion"), ResolveIP("255.0.0.0")).IsValid()); + + // Create Non-IP subnets. + + const CNetAddr tor_addr{ + ResolveIP("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion")}; + + subnet = CSubNet(tor_addr); + BOOST_CHECK(subnet.IsValid()); + BOOST_CHECK_EQUAL(subnet.ToString(), tor_addr.ToString()); + BOOST_CHECK(subnet.Match(tor_addr)); + BOOST_CHECK( + !subnet.Match(ResolveIP("kpgvmscirrdqpekbqjsvw5teanhatztpp2gl6eee4zkowvwfxwenqaid.onion"))); + BOOST_CHECK(!subnet.Match(ResolveIP("1.2.3.4"))); + + BOOST_CHECK(!CSubNet(tor_addr, 200).IsValid()); + BOOST_CHECK(!CSubNet(tor_addr, ResolveIP("255.0.0.0")).IsValid()); subnet = ResolveSubNet("1.2.3.4/255.255.255.255"); BOOST_CHECK_EQUAL(subnet.ToString(), "1.2.3.4/32"); @@ -442,8 +456,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)); diff --git a/test/functional/rpc_setban.py b/test/functional/rpc_setban.py index 523fdc9f63..fd5f8aa098 100755 --- a/test/functional/rpc_setban.py +++ b/test/functional/rpc_setban.py @@ -15,6 +15,9 @@ class SetBanTests(BitcoinTestFramework): self.setup_clean_chain = True self.extra_args = [[],[]] + def is_banned(self, node, addr): + return any(e['address'] == addr for e in node.listbanned()) + def run_test(self): # Node 0 connects to Node 1, check that the noban permission is not granted self.connect_nodes(0, 1) @@ -42,5 +45,18 @@ class SetBanTests(BitcoinTestFramework): peerinfo = self.nodes[1].getpeerinfo()[0] assert(not 'noban' in peerinfo['permissions']) + self.log.info("Test that a non-IP address can be banned/unbanned") + node = self.nodes[1] + tor_addr = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion" + ip_addr = "1.2.3.4" + assert(not self.is_banned(node, tor_addr)) + assert(not self.is_banned(node, ip_addr)) + node.setban(tor_addr, "add") + assert(self.is_banned(node, tor_addr)) + assert(not self.is_banned(node, ip_addr)) + node.setban(tor_addr, "remove") + assert(not self.is_banned(self.nodes[1], tor_addr)) + assert(not self.is_banned(node, ip_addr)) + if __name__ == '__main__': SetBanTests().main() |