From 1018c93d064f3edcb3e45b83fb4057cd83fc667d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 4 Aug 2026 10:42:34 +0200 Subject: [PATCH] [Security] Harden network input handling and websocket proxying - TCP client connections: reject messages whose declared length is negative or exceeds a 1.5MB cap immediately instead of buffering indefinitely, preventing unbounded memory growth and the previously stuck state caused by negative lengths overflowing the int header. - ISL connections: apply the same length validation with a 4MB cap, disconnecting the peer on an implausible length. - Websockets: only trust the forwarded client-address header (e.g. X-Forwarded-For) when the websocket connection originates from an address in security/trusted_sources. Previously any client could spoof its address to bypass bans, user limits and rate limiting. Took 3 minutes --- servatrice/servatrice.ini.example | 6 ++++++ servatrice/src/isl_interface.cpp | 10 +++++++++ servatrice/src/serversocketinterface.cpp | 26 +++++++++++++++++++----- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index fac743c39..f822f10b2 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -41,6 +41,12 @@ websocket_host=any ; The TCP port number servatrice will listen on for websockets clients; default is 4748 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 +; 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 ; setting defines every how many milliseconds servatrice will update its status; default is 15000 (15 secs) statusupdate=15000 diff --git a/servatrice/src/isl_interface.cpp b/servatrice/src/isl_interface.cpp index b0ee201bf..0cbf7d3ad 100644 --- a/servatrice/src/isl_interface.cpp +++ b/servatrice/src/isl_interface.cpp @@ -22,6 +22,9 @@ #include #include +// Maximum size of a single ISL message. +static const int MAX_ISL_MESSAGE_SIZE = 4 * 1024 * 1024; + inline Q_LOGGING_CATEGORY(IslInterfaceLog, "isl_interface"); void IslInterface::sharedCtor(const QSslCertificate &cert, const QSslKey &privateKey) @@ -243,6 +246,13 @@ void IslInterface::readClient() (((quint32)(unsigned char)inputBuffer[1]) << 16) + (((quint32)(unsigned char)inputBuffer[2]) << 8) + ((quint32)(unsigned char)inputBuffer[3]); + // Reject implausible lengths instead of buffering until they arrive. + // This also handles lengths that overflow into negative int values. + if (messageLength < 0 || messageLength > MAX_ISL_MESSAGE_SIZE) { + qCWarning(IslInterfaceLog) << "Invalid message length" << messageLength << "from" << peerAddress; + socket->disconnectFromHost(); + return; + } inputBuffer.remove(0, 4); messageInProgress = true; } else { diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 6ceebfca9..03673548a 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -94,6 +94,9 @@ inline Q_LOGGING_CATEGORY(WebsocketServerSocketInterfaceLog, "websocket_server_s static const int protocolVersion = 14; +// Maximum size of a single TCP protocol message. Matches the websocket limit. +static const int MAX_TCP_MESSAGE_SIZE = 1500000; + AbstractServerSocketInterface::AbstractServerSocketInterface(Servatrice *_server, Servatrice_DatabaseInterface *_databaseInterface, QObject *parent) @@ -2306,13 +2309,21 @@ void TcpServerSocketInterface::readClient() (((quint32)(unsigned char)inputBuffer[1]) << 16) + (((quint32)(unsigned char)inputBuffer[2]) << 8) + ((quint32)(unsigned char)inputBuffer[3]); + // Reject implausible lengths instead of buffering until they arrive. + // This also handles lengths that overflow into negative int values. + if (messageLength < 0 || messageLength > MAX_TCP_MESSAGE_SIZE) { + qCWarning(TcpServerSocketInterfaceLog) + << "Invalid message length" << messageLength << "from" << getAddress(); + prepareDestroy(); + return; + } inputBuffer.remove(0, 4); messageInProgress = true; } else { return; } } - if (inputBuffer.size() < messageLength || messageLength < 0) { + if (inputBuffer.size() < messageLength) { return; } @@ -2418,10 +2429,15 @@ void WebsocketServerSocketInterface::initConnection(void *_socket) QByteArray websocketIPHeader = settingsCache->value("server/web_socket_ip_header", "").toByteArray(); if (websocketIPHeader.length() > 0 && socket->request().hasRawHeader(websocketIPHeader)) { - QString header(socket->request().rawHeader(websocketIPHeader)); - QHostAddress parsed(header); - if (!parsed.isNull()) { - address = parsed; + // 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. + QString trustedSources = settingsCache->value("security/trusted_sources", "127.0.0.1,::1").toString(); + if (trustedSources.contains(socket->peerAddress().toString(), Qt::CaseInsensitive)) { + QString header(socket->request().rawHeader(websocketIPHeader)); + QHostAddress parsed(header); + if (!parsed.isNull()) { + address = parsed; + } } }