Browse Source

Validate connecting peer before honoring X-Forwarded-For

Server::process_request only checked that trusted_proxies_ was
non-empty before deriving req.remote_addr from the X-Forwarded-For
header. It never verified that the actual TCP peer (remote_addr) was
itself one of the trusted proxies, so any client connecting directly
to the server could spoof remote_addr simply by sending an arbitrary
X-Forwarded-For header.

Now X-Forwarded-For is only honored when the connecting peer address
matches an entry in trusted_proxies_.
yhirose 2 weeks ago
parent
commit
fd0c18b1b5
2 changed files with 55 additions and 7 deletions
  1. 8 1
      httplib.h
  2. 47 6
      test/test.cc

+ 8 - 1
httplib.h

@@ -12558,7 +12558,14 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
     connection_closed = true;
   }
 
-  if (!trusted_proxies_.empty() && req.has_header("X-Forwarded-For")) {
+  // Only honor X-Forwarded-For if the peer on the actual TCP connection is
+  // itself a trusted proxy. Otherwise any direct client could spoof
+  // remote_addr simply by sending an arbitrary X-Forwarded-For header.
+  auto is_trusted_peer = std::any_of(
+      trusted_proxies_.begin(), trusted_proxies_.end(),
+      [&](const std::string &proxy) { return proxy == remote_addr; });
+
+  if (is_trusted_peer && req.has_header("X-Forwarded-For")) {
     auto x_forwarded_for = req.get_header_value("X-Forwarded-For");
     auto derived = get_client_ip(x_forwarded_for, trusted_proxies_);
     req.remote_addr = derived.empty() ? remote_addr : derived;

+ 47 - 6
test/test.cc

@@ -15365,6 +15365,45 @@ TEST(HeaderSmugglingTest, ChunkedTrailerHeadersMerged) {
   ASSERT_TRUE(send_request(1, req, &res));
 }
 
+// A direct client that is not listed in trusted_proxies must not be able to
+// spoof req.remote_addr by sending an arbitrary X-Forwarded-For header. Only
+// the peer address on the actual TCP connection determines whether the
+// header is honored.
+TEST(ForwardedHeadersTest, UntrustedDirectClientCannotSpoofXFF) {
+  Server svr;
+
+  // Deliberately does NOT include the loopback address the test client
+  // actually connects from, so the direct connection is not a trusted proxy.
+  svr.set_trusted_proxies({"203.0.113.66"});
+
+  std::string observed_remote_addr;
+
+  svr.Get("/ip", [&](const Request &req, Response &res) {
+    observed_remote_addr = req.remote_addr;
+    res.set_content("ok", "text/plain");
+  });
+
+  thread t = thread([&]() { svr.listen(HOST, PORT); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    t.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  Client cli(HOST, PORT);
+  auto res = cli.Get("/ip", Headers{{"X-Forwarded-For", "9.9.9.9"}});
+
+  ASSERT_TRUE(res);
+  EXPECT_EQ(StatusCode::OK_200, res->status);
+
+  // The connecting peer is not a trusted proxy, so the spoofed header must be
+  // ignored entirely and the real connection-level address retained.
+  EXPECT_TRUE(observed_remote_addr == "::1" ||
+              observed_remote_addr == "127.0.0.1");
+}
+
 TEST(ForwardedHeadersTest, NoProxiesSetting) {
   Server svr;
 
@@ -15434,7 +15473,9 @@ TEST(ForwardedHeadersTest, NoForwardedHeaders) {
 TEST(ForwardedHeadersTest, SingleTrustedProxy_UsesIPBeforeTrusted) {
   Server svr;
 
-  svr.set_trusted_proxies({"203.0.113.66"});
+  // Include the loopback address the test client actually connects from, so
+  // the direct connection itself is recognized as the trusted proxy hop.
+  svr.set_trusted_proxies({"203.0.113.66", "::1", "127.0.0.1"});
 
   std::string observed_remote_addr;
   std::string observed_xff;
@@ -15468,7 +15509,7 @@ TEST(ForwardedHeadersTest, SingleTrustedProxy_UsesIPBeforeTrusted) {
 TEST(ForwardedHeadersTest, MultipleTrustedProxies_UsesClientIP) {
   Server svr;
 
-  svr.set_trusted_proxies({"203.0.113.66", "192.0.2.45"});
+  svr.set_trusted_proxies({"203.0.113.66", "192.0.2.45", "::1", "127.0.0.1"});
 
   std::string observed_remote_addr;
   std::string observed_xff;
@@ -15502,7 +15543,7 @@ TEST(ForwardedHeadersTest, MultipleTrustedProxies_UsesClientIP) {
 TEST(ForwardedHeadersTest, TrustedProxyNotInHeader_UsesRightmostIP) {
   Server svr;
 
-  svr.set_trusted_proxies({"192.0.2.45"});
+  svr.set_trusted_proxies({"192.0.2.45", "::1", "127.0.0.1"});
 
   std::string observed_remote_addr;
   std::string observed_xff;
@@ -15539,7 +15580,7 @@ TEST(ForwardedHeadersTest, TrustedProxyNotInHeader_UsesRightmostIP) {
 TEST(ForwardedHeadersTest, SpoofedIPBeforeTrustedProxyIsIgnored) {
   Server svr;
 
-  svr.set_trusted_proxies({"10.0.0.1"});
+  svr.set_trusted_proxies({"10.0.0.1", "::1", "127.0.0.1"});
 
   std::string observed_remote_addr;
 
@@ -15572,7 +15613,7 @@ TEST(ForwardedHeadersTest, SpoofedIPBeforeTrustedProxyIsIgnored) {
 TEST(ForwardedHeadersTest, LastHopTrusted_SelectsImmediateLeftIP) {
   Server svr;
 
-  svr.set_trusted_proxies({"192.0.2.45"});
+  svr.set_trusted_proxies({"192.0.2.45", "::1", "127.0.0.1"});
 
   std::string observed_remote_addr;
   std::string observed_xff;
@@ -15607,7 +15648,7 @@ TEST(ForwardedHeadersTest, LastHopTrusted_SelectsImmediateLeftIP) {
 TEST(ForwardedHeadersTest, HandlesWhitespaceAroundIPs) {
   Server svr;
 
-  svr.set_trusted_proxies({"192.0.2.45"});
+  svr.set_trusted_proxies({"192.0.2.45", "::1", "127.0.0.1"});
 
   std::string observed_remote_addr;
   std::string observed_xff;