Merge bitcoin/bitcoin#30679: fix: handle invalid -rpcbind port earlier

e6994ef fix: increase rpcbind check robustness (tdb3)
d38e3ae fix: handle invalid rpcbind port earlier (tdb3)
83b67f2 refactor: move host/port checking (tdb3)
73c2439 test: add tests for invalid rpcbind ports (tdb3)

Pull request description:

  Previously, when an invalid port was specified in `-rpcbind`, the `SplitHostPort()` return value in `HTTPBindAddresses()` was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host).

  This rearranges port checking code in `AppInitMain()` to handle the invalid port before reaching `HTTPBindAddresses()`.  Also adds a check in `HTTPBindAddresses()` as a defensive measure for future changes.

  Adds then updates associated functional tests as well.

ACKs for top commit:
  achow101:
    ACK e6994ef
  ryanofsky:
    Code review ACK e6994ef
  zaidmstrr:
    Code review ACK [e6994ef](bitcoin/bitcoin@e6994ef)

Tree-SHA512: bcc3e5ceef21963821cd16ce6ecb83d5c5657755882c05872a7cfe661a1492b1d631f54de22f41fdd173512d62dd15dc37e394fe1a7abe4de484b82cd2438b92

Author: achow101

src/httpserver.cpp

@@ -8,6 +8,7 @@
 
 #include <chainparamsbase.h>
 #include <common/args.h>
+#include <common/messages.h>
 #include <compat/compat.h>
 #include <logging.h>
 #include <netbase.h>
@@ -43,6 +44,8 @@
 
 #include <support/events.h>
 
+using common::InvalidPortErrMsg;
+
 /** Maximum size of http request (request line + headers) */
 static const size_t MAX_HEADERS_SIZE = 8192;
 
@@ -374,7 +377,10 @@ static bool HTTPBindAddresses(struct evhttp* http)
         for (const std::string& strRPCBind : gArgs.GetArgs("-rpcbind")) {
             uint16_t port{http_port};
             std::string host;
-            SplitHostPort(strRPCBind, port, host);
+            if (!SplitHostPort(strRPCBind, port, host)) {
+                LogError("%s\n", InvalidPortErrMsg("-rpcbind", strRPCBind).original);
+                return false;
+            }
             endpoints.emplace_back(host, port);
         }
     }

src/init.cpp

@@ -1145,6 +1145,53 @@ bool AppInitInterfaces(NodeContext& node)
     return true;
 }
 
+bool CheckHostPortOptions(const ArgsManager& args) {
+    for (const std::string port_option : {
+        "-port",
+        "-rpcport",
+    }) {
+        if (args.IsArgSet(port_option)) {
+            const std::string port = args.GetArg(port_option, "");
+            uint16_t n;
+            if (!ParseUInt16(port, &n) || n == 0) {
+                return InitError(InvalidPortErrMsg(port_option, port));
+            }
+        }
+    }
+
+    for ([[maybe_unused]] const auto& [arg, unix] : std::vector<std::pair<std::string, bool>>{
+        // arg name            UNIX socket support
+        {"-i2psam",                 false},
+        {"-onion",                  true},
+        {"-proxy",                  true},
+        {"-rpcbind",                false},
+        {"-torcontrol",             false},
+        {"-whitebind",              false},
+        {"-zmqpubhashblock",        true},
+        {"-zmqpubhashtx",           true},
+        {"-zmqpubrawblock",         true},
+        {"-zmqpubrawtx",            true},
+        {"-zmqpubsequence",         true},
+    }) {
+        for (const std::string& socket_addr : args.GetArgs(arg)) {
+            std::string host_out;
+            uint16_t port_out{0};
+            if (!SplitHostPort(socket_addr, port_out, host_out)) {
+#ifdef HAVE_SOCKADDR_UN
+                // Allow unix domain sockets for some options e.g. unix:/some/file/path
+                if (!unix || !socket_addr.starts_with(ADDR_PREFIX_UNIX)) {
+                    return InitError(InvalidPortErrMsg(arg, socket_addr));
+                }
+#else
+                return InitError(InvalidPortErrMsg(arg, socket_addr));
+#endif
+            }
+        }
+    }
+
+    return true;
+}
+
 bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 {
     const ArgsManager& args = *Assert(node.args);
@@ -1232,6 +1279,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     RegisterZMQRPCCommands(tableRPC);
 #endif
 
+    // Check port numbers
+    if (!CheckHostPortOptions(args)) return false;
+
     /* Start the RPC server already.  It will be started in "warmup" mode
      * and not really process calls already (but it will signify connections
      * that the server is there and will be ready later).  Warmup mode will
@@ -1322,50 +1372,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         validation_signals.RegisterValidationInterface(fee_estimator);
     }
 
-    // Check port numbers
-    for (const std::string port_option : {
-        "-port",
-        "-rpcport",
-    }) {
-        if (args.IsArgSet(port_option)) {
-            const std::string port = args.GetArg(port_option, "");
-            uint16_t n;
-            if (!ParseUInt16(port, &n) || n == 0) {
-                return InitError(InvalidPortErrMsg(port_option, port));
-            }
-        }
-    }
-
-    for ([[maybe_unused]] const auto& [arg, unix] : std::vector<std::pair<std::string, bool>>{
-        // arg name            UNIX socket support
-        {"-i2psam",                 false},
-        {"-onion",                  true},
-        {"-proxy",                  true},
-        {"-rpcbind",                false},
-        {"-torcontrol",             false},
-        {"-whitebind",              false},
-        {"-zmqpubhashblock",        true},
-        {"-zmqpubhashtx",           true},
-        {"-zmqpubrawblock",         true},
-        {"-zmqpubrawtx",            true},
-        {"-zmqpubsequence",         true},
-    }) {
-        for (const std::string& socket_addr : args.GetArgs(arg)) {
-            std::string host_out;
-            uint16_t port_out{0};
-            if (!SplitHostPort(socket_addr, port_out, host_out)) {
-#ifdef HAVE_SOCKADDR_UN
-                // Allow unix domain sockets for some options e.g. unix:/some/file/path
-                if (!unix || !socket_addr.starts_with(ADDR_PREFIX_UNIX)) {
-                    return InitError(InvalidPortErrMsg(arg, socket_addr));
-                }
-#else
-                return InitError(InvalidPortErrMsg(arg, socket_addr));
-#endif
-            }
-        }
-    }
-
     for (const std::string& socket_addr : args.GetArgs("-bind")) {
         std::string host_out;
         uint16_t port_out{0};

test/functional/rpc_bind.py

@@ -45,6 +45,19 @@ def run_bind_test(self, allow_ips, connect_to, addresses, expected):
         assert_equal(set(get_bind_addrs(pid)), set(expected))
         self.stop_nodes()
 
+    def run_invalid_bind_test(self, allow_ips, addresses):
+        '''
+        Attempt to start a node with requested rpcallowip and rpcbind
+        parameters, expecting that the node will fail.
+        '''
+        self.log.info(f'Invalid bind test for {addresses}')
+        base_args = ['-disablewallet', '-nolisten']
+        if allow_ips:
+            base_args += ['-rpcallowip=' + x for x in allow_ips]
+        init_error = 'Error: Invalid port specified in -rpcbind: '
+        for addr in addresses:
+            self.nodes[0].assert_start_raises_init_error(base_args + [f'-rpcbind={addr}'], init_error + f"'{addr}'")
+
     def run_allowip_test(self, allow_ips, rpchost, rpcport):
         '''
         Start a node with rpcallow IP, and request getnetworkinfo
@@ -84,6 +97,10 @@ def run_test(self):
 
         if not self.options.run_nonloopback:
             self._run_loopback_tests()
+            if self.options.run_ipv4:
+                self.run_invalid_bind_test(['127.0.0.1'], ['127.0.0.1:notaport', '127.0.0.1:-18443', '127.0.0.1:0', '127.0.0.1:65536'])
+            if self.options.run_ipv6:
+                self.run_invalid_bind_test(['[::1]'], ['[::1]:notaport', '[::1]:-18443', '[::1]:0', '[::1]:65536'])
         if not self.options.run_ipv4 and not self.options.run_ipv6:
             self._run_nonloopback_tests()