[Server] Parse the rightmost address from forwarded header lists (#7586)

X-Forwarded-For carries a comma-separated list, and constructing a
QHostAddress from the raw header value returns a null address for it, so a
configured X-Forwarded-For or a multi-hop header never resolved and websocket
clients stayed on the proxy's address.

- Add parseForwardedAddress, returning the rightmost usable entry of a
  forwarded header list
- Use it for the websocket handshake instead of parsing the raw value
- Exempt loopback from per-address auth rate limits so a same-host proxy with a
  customized trusted_sources list cannot lock every user out
- Add forwarded_address_test

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

servatrice/CMakeLists.txt

@@ -7,6 +7,7 @@ project(Servatrice VERSION "${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}.${
 set(servatrice_SOURCES
     src/email_parser.cpp
     src/event_loop_watchdog.cpp
+    src/forwarded_address.cpp
     src/main.cpp
     src/metrics_registry.cpp
     src/ratelimiter.cpp

servatrice/servatrice.ini.example

@@ -42,9 +42,11 @@ websocket_host=any
 websocket_port=4748
 
 ; If websockets are served through a reverse proxy (e.g. nginx), the real client address can be
-; read from an HTTP header such as X-Forwarded-For. The header is ONLY trusted when the websocket
+; read from an HTTP header such as X-Real-IP or X-Forwarded-For. A comma-separated X-Forwarded-For
-; connection itself originates from an address listed in security/trusted_sources (default 127.0.0.1,::1),
+; list resolves to its rightmost entry, which is the one the proxy appended. The header is ONLY
-; so that clients cannot spoof their address to bypass bans or rate limits.
+; trusted when the websocket connection itself originates from an address listed in
+; security/trusted_sources (default 127.0.0.1,::1), so that clients cannot spoof their address to
+; bypass bans or rate limits.
 web_socket_ip_header=
 
 ; When database is enabled, servatrice writes the server status in the "update" database table; this

servatrice/src/forwarded_address.cpp

@@ -0,0 +1,21 @@
+#include "forwarded_address.h"
+
+#include <QString>
+
+QHostAddress parseForwardedAddress(const QByteArray &headerValue)
+{
+    const QList<QByteArray> entries = headerValue.split(',');
+    for (auto it = entries.crbegin(); it != entries.crend(); ++it) {
+        const QByteArray entry = it->trimmed();
+        if (entry.isEmpty()) {
+            continue;
+        }
+
+        const QHostAddress parsed(QString::fromLatin1(entry));
+        if (!parsed.isNull()) {
+            return parsed;
+        }
+    }
+
+    return {};
+}

servatrice/src/forwarded_address.h

@@ -0,0 +1,18 @@
+#ifndef FORWARDED_ADDRESS_H
+#define FORWARDED_ADDRESS_H
+
+#include <QByteArray>
+#include <QHostAddress>
+
+/**
+ * @brief Parses the real client address out of a forwarded header value (X-Real-IP, X-Forwarded-For).
+ *
+ * A comma-separated list resolves to its rightmost usable entry. That entry was appended by the
+ * proxy closest to the server and is the only one a direct client cannot choose, since the earlier
+ * entries are passed through verbatim from whatever the client sent.
+ *
+ * @return the parsed address, or a null QHostAddress when the value holds none.
+ */
+QHostAddress parseForwardedAddress(const QByteArray &headerValue);
+
+#endif

servatrice/src/servatrice.cpp

@@ -1551,8 +1551,11 @@ bool Servatrice::isRateLimitExempt(const QString &ipAddress) const
 {
     // Mirrors the max_users_per_address bypass in initSession: trusted sources
     // (e.g. 127.0.0.1 for Webatrice behind local nginx, where every websocket
-    // client shares one address) are not subject to per-address limits.
+    // client shares one address) are not subject to per-address limits. Loopback
-    return isTrustedSource(getTrustedSources(), ipAddress);
+    // is always exempt, so a same-host proxy whose forwarded address could not be
+    // resolved cannot lock every user out together when trusted_sources is
+    // customized.
+    return QHostAddress(ipAddress).isLoopback() || isTrustedSource(getTrustedSources(), ipAddress);
 }
 
 void Servatrice::clearFailedLogins(const QString &ipAddress, const QString &userName)

servatrice/src/serversocketinterface.cpp

@@ -22,6 +22,7 @@
 
 #include "deck_tag_serialization.h"
 #include "email_parser.h"
+#include "forwarded_address.h"
 #include "libcockatrice/protocol/pb/admin_commands.pb.h"
 #include "libcockatrice/protocol/pb/developer_commands.pb.h"
 #include "libcockatrice/protocol/pb/moderator_commands.pb.h"
@@ -4560,8 +4561,7 @@ void WebsocketServerSocketInterface::initConnection(void *_socket)
         // Only trust the forwarded header if the connection itself comes from a trusted proxy,
         // otherwise a client could spoof its address to bypass bans and rate limits.
         if (isTrustedSource(servatrice->getTrustedSources(), socket->peerAddress())) {
-            QString header(socket->request().rawHeader(websocketIPHeader));
+            const QHostAddress parsed = parseForwardedAddress(socket->request().rawHeader(websocketIPHeader));
-            QHostAddress parsed(header);
             if (!parsed.isNull()) {
                 address = parsed;
             }

tests/CMakeLists.txt

@@ -208,6 +208,17 @@ cockatrice_add_test(
   LABELS
   server
 )
+cockatrice_add_test(
+  NAME
+  forwarded_address_test
+  SOURCES
+  ${CMAKE_SOURCE_DIR}/servatrice/src/forwarded_address.cpp
+  forwarded_address_test.cpp
+  INCLUDE_DIRS
+  ${CMAKE_SOURCE_DIR}/servatrice/src
+  LABELS
+  server
+)
 cockatrice_add_test(
   NAME
   trusted_sources_test

tests/forwarded_address_test.cpp

@@ -0,0 +1,37 @@
+#include "forwarded_address.h"
+
+#include "gtest/gtest.h"
+
+namespace
+{
+
+TEST(ForwardedAddressTest, ParsesSingleAddress)
+{
+    EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("203.0.113.7"));
+    EXPECT_EQ(QHostAddress("::1"), parseForwardedAddress("::1"));
+    EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("  203.0.113.7  "));
+}
+
+TEST(ForwardedAddressTest, UsesRightmostEntryOfAList)
+{
+    // nginx's $proxy_add_x_forwarded_for appends its own view of the client to
+    // whatever the client sent, so only the trailing entry is trustworthy.
+    EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("198.51.100.9, 203.0.113.7"));
+    EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("spoofed.invalid, 203.0.113.7"));
+    EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("203.0.113.7, "));
+}
+
+TEST(ForwardedAddressTest, KeepsIpv4MappedEntries)
+{
+    EXPECT_EQ(QHostAddress("::ffff:203.0.113.7"), parseForwardedAddress("::ffff:203.0.113.7"));
+}
+
+TEST(ForwardedAddressTest, RejectsValuesWithoutAnAddress)
+{
+    EXPECT_TRUE(parseForwardedAddress("").isNull());
+    EXPECT_TRUE(parseForwardedAddress(",").isNull());
+    EXPECT_TRUE(parseForwardedAddress("unknown").isNull());
+    EXPECT_TRUE(parseForwardedAddress("garbage, more garbage").isNull());
+}
+
+} // namespace