Merge #20852: net: allow CSubNet of non-IP networks

39b43298d9 test: add test for banning of non-IP addresses (Vasil Dimov)
94d335da7f 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 39b43298d9 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 39b43298d9

Tree-SHA512: 3239b26d0f2fa2d1388b4fdbc1d05ce4ac1980be699c6ec46049409baefcb2006b1e72b889871e2210e897f6725c48e873f68457eea7e6e4958ab4f959d20297

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();
+    switch (addr.m_net) {
-    if (!valid) {
+    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) {
+        uint8_t cidr = 0;
-        if (netmask[i] == 0x00) {
+
-            break;
+        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;

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;

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));

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()