From 73c243965ab256ece089d14173c2d285955e83d5 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Tue, 20 Aug 2024 22:16:33 -0400 Subject: [PATCH 1/4] test: add tests for invalid rpcbind ports --- test/functional/rpc_bind.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py index 8c76c1f5f59..0aabba2c462 100755 --- a/test/functional/rpc_bind.py +++ b/test/functional/rpc_bind.py @@ -6,6 +6,7 @@ from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local from test_framework.test_framework import BitcoinTestFramework, SkipTest +from test_framework.test_node import ErrorMatch from test_framework.util import assert_equal, assert_raises_rpc_error, get_rpc_proxy, rpc_port, rpc_url class RPCBindTest(BitcoinTestFramework): @@ -45,6 +46,19 @@ class RPCBindTest(BitcoinTestFramework): 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' # generic error will be adjusted in next commit + for addr in addresses: + self.nodes[0].assert_start_raises_init_error(base_args + [f'-rpcbind={addr}'], init_error, ErrorMatch.PARTIAL_REGEX) + def run_allowip_test(self, allow_ips, rpchost, rpcport): ''' Start a node with rpcallow IP, and request getnetworkinfo @@ -84,6 +98,10 @@ class RPCBindTest(BitcoinTestFramework): 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() From 83b67f2e6d59ea5de6573314ea4fe54ae52b7c12 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 21 Aug 2024 19:42:45 -0400 Subject: [PATCH 2/4] refactor: move host/port checking Reduces the size of AppInitMain() by moving checks to CheckHostPortOptions() Co-Authored-By: Ryan Ofsky --- src/init.cpp | 90 ++++++++++++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index d6c80d8f84d..c2e634b670f 100644 --- a/src/init.cpp +++ b/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>{ + // 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); @@ -1323,48 +1370,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) } // 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>{ - // 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 - } - } - } + if (!CheckHostPortOptions(args)) return false; for (const std::string& socket_addr : args.GetArgs("-bind")) { std::string host_out; From d38e3aed89eea665399eef07f5c3b64b14d4f586 Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Wed, 21 Aug 2024 20:19:13 -0400 Subject: [PATCH 3/4] fix: handle invalid rpcbind port earlier 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 adjusts associated functional tests. --- src/init.cpp | 6 +++--- test/functional/rpc_bind.py | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index c2e634b670f..8fbdb880bc9 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1279,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 @@ -1369,9 +1372,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) validation_signals.RegisterValidationInterface(fee_estimator); } - // Check port numbers - if (!CheckHostPortOptions(args)) return false; - for (const std::string& socket_addr : args.GetArgs("-bind")) { std::string host_out; uint16_t port_out{0}; diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py index 0aabba2c462..69afd45b9af 100755 --- a/test/functional/rpc_bind.py +++ b/test/functional/rpc_bind.py @@ -6,7 +6,6 @@ from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local from test_framework.test_framework import BitcoinTestFramework, SkipTest -from test_framework.test_node import ErrorMatch from test_framework.util import assert_equal, assert_raises_rpc_error, get_rpc_proxy, rpc_port, rpc_url class RPCBindTest(BitcoinTestFramework): @@ -55,9 +54,9 @@ class RPCBindTest(BitcoinTestFramework): base_args = ['-disablewallet', '-nolisten'] if allow_ips: base_args += ['-rpcallowip=' + x for x in allow_ips] - init_error = 'Error' # generic error will be adjusted in next commit + 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, ErrorMatch.PARTIAL_REGEX) + 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): ''' From e6994efe08b282dd9e46602bcbad69567fe91dcd Mon Sep 17 00:00:00 2001 From: tdb3 <106488469+tdb3@users.noreply.github.com> Date: Thu, 22 Aug 2024 12:13:55 -0400 Subject: [PATCH 4/4] fix: increase rpcbind check robustness Adds invalid rpcbind port checking to `HTTPBindAddresses()`. While movement of `CheckHostPortOptions()` in the previous commit handles rcpbind port errors, updating `HTTPBindAddresses()` port checking adds a defensive measure for potential future changes. --- src/httpserver.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index ebdab1043e8..ffc53fbd9c8 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -43,6 +44,8 @@ #include +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); } }