http: check rpcallowip immediately after accepting connection #35592

pinheadmz commented at 6:07 PM on June 23, 2026: member

This is a follow-up to #35182 addressing a review comment from that PR: #35182#pullrequestreview-4322490068

This update to HTTPServer checks the IP subnet allowlist as soon as possible (immediately after receiving a connection from a client) before any data is received. This does not entirely protect the server from the "slow loris" attack or CWE-400 but does restrict the attack surface to localhost and clients explicitly allowed by the user.

If a client is not allowed by the list, we disconnect as soon as possible. This is a behavior change from master branch (and previous release with libevent) where 403 Forbidden was returned (after a potentially large amount request data was written to memory by the server).

To facilitate existing unit tests, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of static file scope. This is needed because otherwise the allow list would be empty when the unit tests run.

There is still plenty of refactoring to do in order to modernize HTTPServer and de-globalize it, but since this specific issue has a resource allocation guard, I wanted to open it quickly on its own.

DrahtBot added the label RPC/REST/ZMQ on Jun 23, 2026

DrahtBot commented at 6:07 PM on June 23, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35592.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	janb84
Stale ACK	ViniciusCestarii

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

fanquake added this to the milestone 32.0 on Jun 23, 2026

ViniciusCestarii commented at 2:26 PM on June 24, 2026: contributor

tACK f21b2064c95a5dc5253dc7495101081a590c693f

Tested this locally and it works correctly. I liked that this also discloses less information to a potential attacker.

I noticed 2 non-blocking things:

The rejected client loses its diagnostic signal at the bitcoin-cli. Before this change, a blocked IP got a readable error:

error: server returned HTTP error 403

After this change, the same case surfaces as a generic connection failure:

error: Error while attempting to communicate with server 192.168.2.24:18443 (Read error: Connection reset by peer (104))

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.

I'm fine with the trade. But it might be worth listing -rpcallowip as a possible cause in the bitcoin-cli connection-failure hint. Could be a follow-up.

This is a user-visible behavior change so I believe this should have a release note.

pinheadmz commented at 3:08 PM on June 24, 2026: member

Thanks @ViniciusCestarii this was mentioned by @b-l-u-e as well. I pushed 429ad3bd29fef33ec5f1b70f386e3846bb18c9a3 which adds a release note.

in src/httpserver.h:392 in 429ad3bd29

 387 | +    std::vector<CSubNet> m_allow_subnets;
 388 | +
 389 | +    /**
 390 | +     * Check an incoming connection's source IP against the allow list
 391 | +     */
 392 | +    bool ClientAllowed(const CNetAddr& netaddr);

b-l-u-e commented at 8:29 PM on June 24, 2026:

nit i think maybe should be const since it only reads m_allow_subnets

-    bool ClientAllowed(const CNetAddr& netaddr);
+    bool ClientAllowed(const CNetAddr& netaddr) const;

pinheadmz commented at 2:37 PM on June 30, 2026:

Good catch! adding const

in src/httpserver.cpp:80 in 429ad3bd29

  74 | @@ -77,23 +75,24 @@ static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_m
  75 |  static ThreadPool g_threadpool_http("http");
  76 |  static int g_max_queue_depth{100};
  77 |  
  78 | +namespace http_bitcoin {
  79 |  /** Check if a network address is allowed to access the HTTP server */
  80 | -static bool ClientAllowed(const CNetAddr& netaddr)
  81 | +bool HTTPServer::ClientAllowed(const CNetAddr& netaddr)

b-l-u-e commented at 8:30 PM on June 24, 2026:

 namespace http_bitcoin {
 /** Check if a network address is allowed to access the HTTP server */
-bool HTTPServer::ClientAllowed(const CNetAddr& netaddr)
+bool HTTPServer::ClientAllowed(const CNetAddr& netaddr) const
 {
     if (!netaddr.IsValid())
         return false;

in src/httpserver.cpp:78 in 429ad3bd29 outdated

  74 | @@ -77,23 +75,24 @@ static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_m
  75 |  static ThreadPool g_threadpool_http("http");
  76 |  static int g_max_queue_depth{100};
  77 |  
  78 | +namespace http_bitcoin {

b-l-u-e commented at 9:04 PM on June 24, 2026:

i see duplicate namespace http_bitcoin introduced here maybe move both definitions to existing block namespace http_bitcoin

pinheadmz commented at 2:38 PM on June 30, 2026:

I'm going to leave this alone for now because it keeps the diff in this PR smaller. There are more followups to #35182 coming that move more of these static methods inside the HTTPServer class and I think that will be a better time for moving functions around.

pinheadmz force-pushed on Jun 30, 2026

pinheadmz commented at 2:39 PM on June 30, 2026: member

Push to 69a28fe60772f938ab94daff27980aa1b33858f5

Address feedback from @b-l-u-e adding const to one of the moved methods.

in test/functional/rpc_bind.py:7 in 69a28fe607

   6 | @@ -7,7 +7,8 @@
   7 |  from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local

janb84 commented at 8:32 AM on July 1, 2026:

from test_framework.netutil import NETWORK_ERRORS, all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local

NIT combine separate imports from netutil (remove line 10)

pinheadmz commented at 2:36 PM on July 8, 2026:

👍

in src/httpserver.cpp:782 in 69a28fe607

janb84 commented at 9:12 AM on July 1, 2026:

    // by the ClientAllowed() check.

NIT: Because of the change the comment "downstream" is not fitting anymore.

janb84 commented at 12:31 PM on July 1, 2026:

    // 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,

NIT: make sure InitHTTPAllowlist is called before starting the socket handler. (This can be a bit to defensive programming.)

pinheadmz commented at 2:32 PM on July 8, 2026:

Sure, added this defense the I/O thread initialization.

pinheadmz commented at 2:35 PM on July 8, 2026:

right! nice! removing the comment.

in src/httpserver.cpp:93 in 69a28fe607 outdated

  94 | +bool HTTPServer::InitHTTPAllowList()
  95 |  {
  96 | -    rpc_allow_subnets.clear();
  97 | -    rpc_allow_subnets.emplace_back(LookupHost("127.0.0.1", false).value(), 8);  // always allow IPv4 local subnet
  98 | -    rpc_allow_subnets.emplace_back(LookupHost("::1", false).value());  // always allow IPv6 localhost
  99 | +    m_allow_subnets.clear();

janb84 commented at 12:31 PM on July 1, 2026:

    Assume(!m_thread_socket_handler.joinable());
    m_allow_subnets.clear();

NIT: Make sure InitHTTPAllowList() can only be ran before StartSocketsThreads(). StartSocketsThreads calls ClientAllowed() and calling InitHttpAllowlist afterwards can cause a race condition.

pinheadmz commented at 2:26 PM on July 8, 2026:

Good call, adding the assumption with a comment here.

janb84 commented at 12:32 PM on July 1, 2026: contributor

Concept ACT 69a28fe60772f938ab94daff27980aa1b33858f5

Thanks for addressing this so fast ! The code changes in this PR will solve my review findings.

Did code review & security review. Some minor nit-picks and one possible race condition in the comments.

pinheadmz force-pushed on Jul 8, 2026

pinheadmz commented at 2:53 PM on July 8, 2026: member

push to a89fa837ad0985afd7f2382719c2e08ac393fa8d

Address feedback from @janb84, some nits, and extra Assume to prevent future regressions

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.

d1ed2a6e25

doc: add release note describing change for forbidden clients 55d3cd51a4

pinheadmz force-pushed on Jul 8, 2026

DrahtBot added the label CI failed on Jul 8, 2026

DrahtBot commented at 3:30 PM on July 8, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/28952193672/job/85901606969</sub> <sub>LLM reason (✨ experimental): CI failed because ctest aborted on httpserver_tests due to an assertion failure in HTTPServer::StartSocketsThreads() (!m_allow_subnets.empty()).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

pinheadmz commented at 3:30 PM on July 8, 2026: member

push to 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

rebase on master and fix silent conflict with #35614 which added another HTTPServer unit test, and therefore also needs a call to InitHTTPAllowList()

DrahtBot removed the label CI failed on Jul 8, 2026

janb84 commented at 5:51 PM on July 8, 2026: contributor

ACK 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

Thanks for incorporating my nit suggestions.