From d1ed2a6e25d7e64943fbff5e3a7053c55c0617e4 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Sat, 23 May 2026 09:24:21 -0400 Subject: [PATCH] http: check rpcallowip immediately after accepting connection Instead of sending 403 Forbidden, disconnect as soon as possible. To facilitate unit testing, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of file-scope static scope. --- src/httpserver.cpp | 55 +++++++++++++---------- src/httpserver.h | 15 +++++++ src/test/httpserver_tests.cpp | 2 + src/test/util/setup_common.cpp | 4 ++ test/functional/interface_http.py | 10 +---- test/functional/rpc_bind.py | 18 +++++--- test/functional/test_framework/netutil.py | 10 +++++ 7 files changed, 75 insertions(+), 39 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index a05d19db1f2..ad8b5b2a904 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -67,8 +67,6 @@ struct HTTPPathHandler /** HTTP module state */ static std::unique_ptr g_http_server{nullptr}; -//! List of subnets to allow RPC connections from -static std::vector rpc_allow_subnets; //! Handlers for (sub)paths static GlobalMutex g_httppathhandlers_mutex; static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_mutex); @@ -77,23 +75,28 @@ static std::vector pathHandlers GUARDED_BY(g_httppathhandlers_m static ThreadPool g_threadpool_http("http"); static int g_max_queue_depth{100}; +namespace http_bitcoin { /** Check if a network address is allowed to access the HTTP server */ -static bool ClientAllowed(const CNetAddr& netaddr) +bool HTTPServer::ClientAllowed(const CNetAddr& netaddr) const { if (!netaddr.IsValid()) return false; - for(const CSubNet& subnet : rpc_allow_subnets) + for(const CSubNet& subnet : m_allow_subnets) if (subnet.Match(netaddr)) return true; return false; } /** Initialize ACL list for HTTP server */ -static bool InitHTTPAllowList() +bool HTTPServer::InitHTTPAllowList() { - rpc_allow_subnets.clear(); - rpc_allow_subnets.emplace_back(LookupHost("127.0.0.1", false).value(), 8); // always allow IPv4 local subnet - rpc_allow_subnets.emplace_back(LookupHost("::1", false).value()); // always allow IPv6 localhost + // Must be run before StartSocketThreads() because ThreadSocketHandler() + // will check m_allow_subnets from the I/O thread. + Assume(!m_thread_socket_handler.joinable()); + + m_allow_subnets.clear(); + m_allow_subnets.emplace_back(LookupHost("127.0.0.1", false).value(), 8); // always allow IPv4 local subnet + m_allow_subnets.emplace_back(LookupHost("::1", false).value()); // always allow IPv6 localhost for (const std::string& strAllow : gArgs.GetArgs("-rpcallowip")) { const CSubNet subnet{LookupSubNet(strAllow)}; if (!subnet.IsValid()) { @@ -102,14 +105,15 @@ static bool InitHTTPAllowList() CClientUIInterface::MSG_ERROR); return false; } - rpc_allow_subnets.push_back(subnet); + m_allow_subnets.push_back(subnet); } std::string strAllowed; - for (const CSubNet& subnet : rpc_allow_subnets) + for (const CSubNet& subnet : m_allow_subnets) strAllowed += subnet.ToString() + " "; LogDebug(BCLog::HTTP, "Allowing HTTP connections from: %s\n", strAllowed); return true; } +} // namespace http_bitcoin /** HTTP request method as string - use for logging only */ std::string_view RequestMethodString(HTTPRequestMethod m) @@ -127,14 +131,6 @@ std::string_view RequestMethodString(HTTPRequestMethod m) static void MaybeDispatchRequestToWorker(std::shared_ptr hreq) { - // Early address-based allow check - if (!ClientAllowed(hreq->GetPeer())) { - LogDebug(BCLog::HTTP, "HTTP request from %s rejected: Client network is not allowed RPC access\n", - hreq->GetPeer().ToStringAddrPort()); - hreq->WriteReply(HTTP_FORBIDDEN); - return; - } - // Early reject unknown HTTP methods if (hreq->GetRequestMethod() == HTTPRequestMethod::UNKNOWN) { LogDebug(BCLog::HTTP, "HTTP request from %s rejected: Unknown HTTP request method\n", @@ -758,6 +754,11 @@ void HTTPServer::StopListening() void HTTPServer::StartSocketsThreads() { + // The socket handler reads m_allow_subnets in ClientAllowed(). InitHTTPAllowList() + // must have populated it first; localhost entries are always added, so an empty + // list means it was never called and every connection is rejected. + Assume(!m_allow_subnets.empty()); + m_thread_socket_handler = std::thread(&util::TraceThread, "http", [this] { ThreadSocketHandler(); }); @@ -792,13 +793,19 @@ std::unique_ptr HTTPServer::AcceptConnection(const Sock& listen_sock, CSer } // The OS handed us a valid socket but we can't determine its source address. - // In the unlikely event this occurs, the invalid address will be rejected - // by the downstream ClientAllowed() check. if (!addr.SetSockAddr(sa, len)) { LogDebug(BCLog::HTTP, "Unknown socket family"); } + // Early address-based allow check + if (!ClientAllowed(addr)) { + LogDebug(BCLog::HTTP, "Connection from %s rejected: Client network is not allowed HTTP access\n", + addr.ToStringAddrPort()); + // Socket destroyed, connection aborted + return {}; + } + return sock; } @@ -1212,13 +1219,13 @@ bool HTTPRemoteClient::MaybeSendBytesFromBuffer() bool InitHTTPServer() { - if (!InitHTTPAllowList()) { - return false; - } - // Create HTTPServer g_http_server = std::make_unique(MaybeDispatchRequestToWorker); + if (!g_http_server->InitHTTPAllowList()) { + return false; + } + g_http_server->SetServerTimeout(std::chrono::seconds(gArgs.GetIntArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT))); // Bind HTTP server to specified addresses diff --git a/src/httpserver.h b/src/httpserver.h index 9031eb61e0a..295f193f1e9 100644 --- a/src/httpserver.h +++ b/src/httpserver.h @@ -215,6 +215,11 @@ public: Assume(m_listen.empty()); // Missing call to StopListening() } + /** + * Parse the user's -rpcallowip settings and populate m_allow_subnets + */ + bool InitHTTPAllowList(); + /** * Bind to a new address:port, start listening and add the listen socket to `m_listen`. * @param[in] to Where to bind. @@ -376,6 +381,16 @@ private: */ std::chrono::seconds m_rpcservertimeout{DEFAULT_HTTP_SERVER_TIMEOUT}; + /** + * List of subnets to allow HTTP connections from + */ + std::vector m_allow_subnets; + + /** + * Check an incoming connection's source IP against the allow list + */ + bool ClientAllowed(const CNetAddr& netaddr) const; + /** * Accept a connection. * @param[in] listen_sock Socket on which to accept the connection. diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp index aae899202d7..cd020fc60c1 100644 --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -522,6 +522,7 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests) }; HTTPServer server{StoreRequest}; + server.InitHTTPAllowList(); { // We can only bind to NET_IPV4 and NET_IPV6 @@ -647,6 +648,7 @@ BOOST_AUTO_TEST_CASE(http_socket_error_tests) // Can't call BOOST_REQUIRE from worker thread Assert(workers.Submit(std::move(item))); }}; + server.InitHTTPAllowList(); // All replies will be the same size static constexpr std::size_t reply_length = std::string_view{ diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index bcf637784d7..051035e80f3 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -640,6 +640,10 @@ std::vector TestChain100Setup::PopulateMempool(FastRandomContex SocketTestingSetup::SocketTestingSetup() { + // HTTPServer is not integrated into NodeContext yet and still pulls global args. + // This is the IP address DynSock claims to be from when connecting. + gArgs.ForceSetArg("-rpcallowip", "5.5.5.5"); + // "back up" the current CreateSock() so we can restore it after the test m_create_sock_orig = CreateSock; diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py index a71fc474a8f..b0c5243f9a8 100755 --- a/test/functional/interface_http.py +++ b/test/functional/interface_http.py @@ -5,6 +5,7 @@ """Test the HTTP server basics.""" from test_framework.test_framework import BitcoinTestFramework +from test_framework.netutil import NETWORK_ERRORS from test_framework.util import assert_equal, str_to_b64str import http.client @@ -17,15 +18,6 @@ RPCSERVERTIMEOUT = 2 MAX_HEADERS_SIZE = 8192 MAX_BODY_SIZE = 32 * 1024 * 1024 -# When a test expects a server disconnection, any of these errors are -# acceptable. The specific event is determined by race condition and platform OS. -NETWORK_ERRORS = ( - BrokenPipeError, # write to a closed socket/pipe - ConnectionResetError, # connection forcibly closed by peer - ConnectionAbortedError, # connection aborted locally or by network stack - http.client.ResponseNotReady, # server response not ready or connection out of sync -) - class BitcoinHTTPConnection: def __init__(self, node): self.url = urllib.parse.urlparse(node.url) diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py index 517df5d9c3f..4494dde5dcf 100755 --- a/test/functional/rpc_bind.py +++ b/test/functional/rpc_bind.py @@ -4,10 +4,10 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test running bitcoind with the -rpcbind and -rpcallowip options.""" -from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local +from test_framework.netutil import NETWORK_ERRORS, 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, rpc_port +from test_framework.util import assert_equal, rpc_port class RPCBindTest(BitcoinTestFramework): def set_test_params(self): @@ -62,6 +62,7 @@ class RPCBindTest(BitcoinTestFramework): Start a node with rpcallow IP, and request getnetworkinfo at a non-localhost IP. ''' + success = True self.log.info("Allow IP test for %s:%d" % (rpchost, rpcport)) node_args = \ ['-disablewallet', '-nolisten'] + \ @@ -72,8 +73,12 @@ class RPCBindTest(BitcoinTestFramework): self.nodes[0].rpchost = f"{rpchost}:{rpcport}" # connect to node through non-loopback interface node = self.nodes[0].create_new_rpc_connection() - node.getnetworkinfo() + try: + node.getnetworkinfo() + except NETWORK_ERRORS: + success = False self.stop_nodes() + return success def run_invalid_allowip_test(self): ''' @@ -162,12 +167,13 @@ class RPCBindTest(BitcoinTestFramework): self.run_bind_test([self.non_loopback_ip], self.non_loopback_ip, [self.non_loopback_ip], [(self.non_loopback_ip, self.defaultport)]) - # Check that with invalid rpcallowip, we are denied - self.run_allowip_test([self.non_loopback_ip], self.non_loopback_ip, self.defaultport) + # Check that connections from allowed IPs are allowed + assert self.run_allowip_test([self.non_loopback_ip], self.non_loopback_ip, self.defaultport) + # Otherwise we are denied if self.options.usecli: self.log.info("Skip negative IP test with CLI, because the CLI can not throw the tested exception type") return - assert_raises_rpc_error(-342, "non-JSON HTTP response with '403 Forbidden' from server", self.run_allowip_test, ['1.1.1.1'], self.non_loopback_ip, self.defaultport) + assert not self.run_allowip_test(['1.1.1.1'], self.non_loopback_ip, self.defaultport) if __name__ == '__main__': RPCBindTest(__file__).main() diff --git a/test/functional/test_framework/netutil.py b/test/functional/test_framework/netutil.py index 17043541f5a..a13ee61e59f 100644 --- a/test/functional/test_framework/netutil.py +++ b/test/functional/test_framework/netutil.py @@ -7,6 +7,7 @@ Roughly based on https://web.archive.org/web/20190424172231/http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/ by Ricardo Pascal """ +import http.client import sys import socket import struct @@ -34,6 +35,15 @@ ADDRMAN_NEW_BUCKET_COUNT = 1 << 10 ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8 ADDRMAN_BUCKET_SIZE = 1 << 6 +# When a test expects a server disconnection, any of these errors are +# acceptable. The specific event is determined by race condition and platform OS. +NETWORK_ERRORS = ( + BrokenPipeError, # write to a closed socket/pipe + ConnectionResetError, # connection forcibly closed by peer + ConnectionAbortedError, # connection aborted locally or by network stack + http.client.ResponseNotReady, # server response not ready or connection out of sync +) + def get_socket_inodes(pid): ''' Get list of socket inodes for process pid.