From 152a74d3f4fc7705298d89050db7494809d1fa72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 30 Aug 2026 23:32:32 +0200 Subject: [PATCH] [Server] Fix rate limiter: disable semantics, draining window, bounded keys, pre-work login gate - recordAttempt()/isBlocked() return false when maxAttempts <= 0 or windowSeconds <= 0, so the documented 'set to 0 to disable' actually disables instead of blocking every address permanently - Keys at their limit are rejected without recording, so repeated attempts at the attacker's pace cannot keep sliding the window and holding the lockout open; a block now clears once the recorded attempts age out of windowSeconds - Attempts map is bounded: an opportunistic prune (every 60s) erases keys whose attempts have fully aged out - Login is throttled before the work: isLoginRateLimited() (a new virtual on Server, backed by isBlocked()) gates loginUser(), so a locked-out address no longer burns a database round trip and password verification per attempt - recordAttemptAt()/isBlockedAt() time seams make the sliding-window behaviour deterministically testable; tests rewritten for the real semantics (no reliance on the maxAttempts=0 bug, no sleeps) --- .../network/server/remote/server.h | 10 +++ .../server/remote/server_protocolhandler.cpp | 5 ++ servatrice/src/ratelimiter.cpp | 62 +++++++++++-- servatrice/src/ratelimiter.h | 59 ++++++++++-- servatrice/src/servatrice.cpp | 5 ++ servatrice/src/servatrice.h | 1 + tests/rate_limiter_test.cpp | 89 +++++++++++++++---- 7 files changed, 202 insertions(+), 29 deletions(-) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server.h b/libcockatrice_network/libcockatrice/network/server/remote/server.h index f36e91598..82fb5601f 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server.h @@ -180,6 +180,16 @@ public: { return false; } + /** + * @brief True if the given address is already locked out of logging in. + * + * Consulted before any authentication work (database round trip, password + * verification) so a blocked address cannot burn server CPU per attempt. + */ + virtual bool isLoginRateLimited(const QString & /*ipAddress*/) + { + return false; + } /** @brief Clear any failed-login lockout for the given address, e.g. after a successful login. */ virtual void clearFailedLogins(const QString & /*ipAddress*/) { diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index 8fb47f8f7..7a519a239 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -514,6 +514,11 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd QString reasonStr; int banSecondsLeft = 0; QString connectionType = getConnectionType(); + // Throttle before doing any work: a locked-out address must not get a full + // database round trip and password verification on every attempt. + if (server->isLoginRateLimited(getAddress())) { + return Response::RespTooManyRequests; + } AuthenticationResult res = server->loginUser(this, userName, password, needsHash, reasonStr, banSecondsLeft, clientId, clientVersion, connectionType); switch (res) { diff --git a/servatrice/src/ratelimiter.cpp b/servatrice/src/ratelimiter.cpp index d22b851dd..ad2de69b4 100644 --- a/servatrice/src/ratelimiter.cpp +++ b/servatrice/src/ratelimiter.cpp @@ -4,22 +4,47 @@ bool RateLimiter::recordAttempt(const QString &key, int maxAttempts, int windowSeconds) { - QMutexLocker locker(&mutex); - const qint64 now = QDateTime::currentSecsSinceEpoch(); + return recordAttemptAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch()); +} - QList ×tamps = attempts[key]; - timestamps.append(now); +bool RateLimiter::recordAttemptAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) +{ + if (maxAttempts <= 0 || windowSeconds <= 0) { + return false; // rate limiting disabled for this endpoint + } + + QMutexLocker locker(&mutex); + pruneLocked(now); + + AttemptHistory &history = attempts[key]; + history.windowSeconds = windowSeconds; + QList ×tamps = history.timestamps; while (!timestamps.isEmpty() && timestamps.first() <= now - windowSeconds) { timestamps.removeFirst(); } + if (timestamps.size() >= maxAttempts) { + // Already at the limit: reject without appending so repeated attempts + // at the attacker's own pace cannot keep sliding the window forward + // and hold the lockout open forever. + return true; + } - return timestamps.size() > maxAttempts; + timestamps.append(now); + return false; } bool RateLimiter::isBlocked(const QString &key, int maxAttempts, int windowSeconds) const { + return isBlockedAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch()); +} + +bool RateLimiter::isBlockedAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const +{ + if (maxAttempts <= 0 || windowSeconds <= 0) { + return false; + } + QMutexLocker locker(&mutex); - const qint64 now = QDateTime::currentSecsSinceEpoch(); const auto it = attempts.constFind(key); if (it == attempts.constEnd()) { @@ -27,12 +52,12 @@ bool RateLimiter::isBlocked(const QString &key, int maxAttempts, int windowSecon } int count = 0; - for (const qint64 ×tamp : it.value()) { + for (const qint64 ×tamp : it.value().timestamps) { if (timestamp > now - windowSeconds) { ++count; } } - return count > maxAttempts; + return count >= maxAttempts; } void RateLimiter::clearAttempts(const QString &key) @@ -40,3 +65,24 @@ void RateLimiter::clearAttempts(const QString &key) QMutexLocker locker(&mutex); attempts.remove(key); } + +void RateLimiter::pruneLocked(qint64 now) +{ + if (lastPruneSecs + PruneIntervalSeconds > now) { + return; + } + lastPruneSecs = now; + + auto it = attempts.begin(); + while (it != attempts.end()) { + QList ×tamps = it.value().timestamps; + while (!timestamps.isEmpty() && timestamps.first() <= now - it.value().windowSeconds) { + timestamps.removeFirst(); + } + if (timestamps.isEmpty()) { + it = attempts.erase(it); + } else { + ++it; + } + } +} \ No newline at end of file diff --git a/servatrice/src/ratelimiter.h b/servatrice/src/ratelimiter.h index ba2998093..c93d8c1bf 100644 --- a/servatrice/src/ratelimiter.h +++ b/servatrice/src/ratelimiter.h @@ -6,20 +6,69 @@ #include #include -/** @brief Thread-safe per-key attempt counter used to throttle abusive requests. */ +/** @brief Thread-safe per-key attempt limiter used to throttle abusive requests. */ class RateLimiter { public: - /** @brief Record an attempt for the key and report whether the key is now over the limit. */ + /** + * @brief Record an attempt for @p key and report whether the key is now blocked. + * + * A key that is already at its limit is not extended: the attempt is + * rejected without appending, so the lockout lifts once the recorded + * attempts age out of the window. Returns false immediately when + * @p maxAttempts <= 0 or @p windowSeconds <= 0 (rate limiting disabled), + * recording nothing. + * + * Delegates to recordAttemptAt() with the current wall-clock time. + */ bool recordAttempt(const QString &key, int maxAttempts, int windowSeconds); - /** @brief True if the key already has more than maxAttempts attempts within windowSeconds. */ + + /** + * @brief Record an attempt as seen at epoch second @p now. + * + * Exposed for deterministic tests; production callers should use + * recordAttempt(). + */ + bool recordAttemptAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now); + + /** + * @brief True if @p key already has at least @p maxAttempts attempts within @p windowSeconds. + * + * Reads only; records nothing. Returns false when @p maxAttempts <= 0 or + * @p windowSeconds <= 0. Delegates to isBlockedAt(). + */ bool isBlocked(const QString &key, int maxAttempts, int windowSeconds) const; + + /** + * @brief Whether @p key is blocked as measured at epoch second @p now. + * + * Exposed for deterministic tests; production callers should use + * isBlocked(). + */ + bool isBlockedAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const; + /** @brief Drop all recorded attempts for the key, e.g. after a successful login. */ void clearAttempts(const QString &key); private: + struct AttemptHistory + { + QList timestamps; // epoch seconds, oldest first + int windowSeconds; + }; + + /** + * @brief Drops keys whose last attempts have fully aged out of their window. + * + * Runs at most every PruneIntervalSeconds to stop the map from growing + * without bound when attackers rotate source addresses. + */ + void pruneLocked(qint64 now); + mutable QMutex mutex; - QMap> attempts; // key -> attempt timestamps (epoch seconds) + QMap attempts; // key -> attempt history + qint64 lastPruneSecs = 0; + static constexpr int PruneIntervalSeconds = 60; }; -#endif +#endif \ No newline at end of file diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index 0557a9889..b896aee0d 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -1080,6 +1080,11 @@ bool Servatrice::recordFailedLogin(const QString &ipAddress) return rateLimiter.recordAttempt("login:" + ipAddress, getMaxLoginAttemptsPerIp(), getLoginAttemptWindowSeconds()); } +bool Servatrice::isLoginRateLimited(const QString &ipAddress) +{ + return rateLimiter.isBlocked("login:" + ipAddress, getMaxLoginAttemptsPerIp(), getLoginAttemptWindowSeconds()); +} + void Servatrice::clearFailedLogins(const QString &ipAddress) { rateLimiter.clearAttempts("login:" + ipAddress); diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index f968db4a9..ff961d255 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -284,6 +284,7 @@ public: return &rateLimiter; } bool recordFailedLogin(const QString &ipAddress) override; + bool isLoginRateLimited(const QString &ipAddress) override; void clearFailedLogins(const QString &ipAddress) override; int getMaxLoginAttemptsPerIp() const; int getLoginAttemptWindowSeconds() const; diff --git a/tests/rate_limiter_test.cpp b/tests/rate_limiter_test.cpp index ae495b122..270a2176b 100644 --- a/tests/rate_limiter_test.cpp +++ b/tests/rate_limiter_test.cpp @@ -8,36 +8,93 @@ namespace TEST(RateLimiterTest, AllowsAttemptsWithinLimit) { RateLimiter limiter; - ASSERT_FALSE(limiter.recordAttempt("ip", 3, 60)); - ASSERT_FALSE(limiter.recordAttempt("ip", 3, 60)); - ASSERT_FALSE(limiter.recordAttempt("ip", 3, 60)); - ASSERT_FALSE(limiter.isBlocked("ip", 3, 60)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1000)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1001)); + // Still under the limit after two attempts, so the key is not blocked... + ASSERT_FALSE(limiter.isBlockedAt("ip", 3, 60, 1002)); + // ...and the third (the limit) is allowed through. + ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1002)); + // The next attempt is blocked once three are on the books. + ASSERT_TRUE(limiter.isBlockedAt("ip", 3, 60, 1003)); + ASSERT_TRUE(limiter.recordAttemptAt("ip", 3, 60, 1003)); } -TEST(RateLimiterTest, BlocksAttemptsOverLimit) +TEST(RateLimiterTest, BlocksAttemptsAtTheLimit) { RateLimiter limiter; - ASSERT_FALSE(limiter.recordAttempt("ip", 2, 60)); - ASSERT_FALSE(limiter.recordAttempt("ip", 2, 60)); - ASSERT_TRUE(limiter.recordAttempt("ip", 2, 60)); - ASSERT_TRUE(limiter.isBlocked("ip", 2, 60)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, 1000)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, 1001)); + ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, 60, 1002)); + ASSERT_TRUE(limiter.isBlockedAt("ip", 2, 60, 1002)); +} + +TEST(RateLimiterTest, NonPositiveConfigDisablesLimiting) +{ + RateLimiter limiter; + // maxAttempts = 0 is the documented "set to 0 to disable"; negative or + // zero windows are config mistakes and must not lock everyone out either. + ASSERT_FALSE(limiter.recordAttempt("ip", 0, 60)); + ASSERT_FALSE(limiter.isBlocked("ip", 0, 60)); + ASSERT_FALSE(limiter.recordAttempt("ip", -1, 60)); + ASSERT_FALSE(limiter.isBlocked("ip", -1, 60)); + ASSERT_FALSE(limiter.recordAttempt("ip", 5, 0)); + ASSERT_FALSE(limiter.isBlocked("ip", 5, 0)); } TEST(RateLimiterTest, ClearAttempts) { RateLimiter limiter; - ASSERT_TRUE(limiter.recordAttempt("ip", 0, 60)); - ASSERT_TRUE(limiter.isBlocked("ip", 0, 60)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 1, 60, 1000)); + ASSERT_TRUE(limiter.recordAttemptAt("ip", 1, 60, 1001)); + ASSERT_TRUE(limiter.isBlockedAt("ip", 1, 60, 1001)); limiter.clearAttempts("ip"); - ASSERT_FALSE(limiter.isBlocked("ip", 0, 60)); + ASSERT_FALSE(limiter.isBlockedAt("ip", 1, 60, 1002)); + // The cleared key starts fresh and is allowed again. + ASSERT_FALSE(limiter.recordAttemptAt("ip", 1, 60, 1002)); } TEST(RateLimiterTest, KeysAreIndependent) { RateLimiter limiter; - ASSERT_TRUE(limiter.recordAttempt("a", 0, 60)); - ASSERT_FALSE(limiter.isBlocked("b", 0, 60)); - ASSERT_TRUE(limiter.isBlocked("a", 0, 60)); + ASSERT_FALSE(limiter.recordAttemptAt("a", 1, 60, 1000)); + ASSERT_TRUE(limiter.recordAttemptAt("a", 1, 60, 1001)); + ASSERT_FALSE(limiter.isBlockedAt("b", 1, 60, 1001)); + ASSERT_TRUE(limiter.isBlockedAt("a", 1, 60, 1001)); +} + +TEST(RateLimiterTest, AttemptsAgeOutOfTheWindow) +{ + RateLimiter limiter; + const qint64 t = 1000; + const int window = 60; + + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 1)); + ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, window, t + 2)); + ASSERT_TRUE(limiter.isBlockedAt("ip", 2, window, t + 2)); + + // A blocked attempt is rejected without recording, so it cannot extend + // the lockout either. + ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, window, t + 3)); + + // Once the recorded attempts fall outside the window the key clears by + // itself, with no manual unlock. + ASSERT_FALSE(limiter.isBlockedAt("ip", 2, window, t + 61)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 62)); +} + +TEST(RateLimiterTest, BlockedKeyIsUnlockedAfterSuccessfulClear) +{ + RateLimiter limiter; + const qint64 t = 5000; + + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t)); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t + 1)); + ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, 60, t + 2)); + ASSERT_TRUE(limiter.isBlockedAt("ip", 2, 60, t + 2)); + + limiter.clearAttempts("ip"); + ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t + 3)); } } // namespace @@ -46,4 +103,4 @@ int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); -} +} \ No newline at end of file