From b2f63255f006d61899f1668a04c9c64523d10472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 4 Aug 2026 10:38:20 +0200 Subject: [PATCH] [Security] Add per-address rate limiting for auth endpoints Introduce a thread-safe RateLimiter that tracks attempts per key (IP address) within a sliding time window, and wire it into the authentication endpoints: - Login: failed login attempts from an address are counted; once the configured maximum is exceeded within the window, further logins from that address are rejected with RespTooManyRequests. A successful login clears the failed attempts for that address. - Registration: implement the previously stubbed tooManyRegistrationAttempts, limiting how many accounts can be created per address per window. - Forgot-password: throttle both the email-request and the email-challenge paths per address. New [security] settings with defaults: max_login_attempts_per_ip=5 / login_attempt_window_seconds=900 max_registrations_per_ip=2 / registration_window_seconds=3600 max_forgot_password_requests_per_ip=3 / forgot_password_window_seconds=3600 Adds unit tests for the RateLimiter (window limit, over-limit blocking, clearing, per-key independence). Took 3 minutes --- .../network/server/remote/server.h | 9 ++++ .../server/remote/server_protocolhandler.cpp | 6 +++ servatrice/CMakeLists.txt | 1 + servatrice/servatrice.ini.example | 21 ++++++++ servatrice/src/ratelimiter.cpp | 42 ++++++++++++++++ servatrice/src/ratelimiter.h | 25 ++++++++++ servatrice/src/servatrice.cpp | 40 +++++++++++++++ servatrice/src/servatrice.h | 17 +++++++ servatrice/src/serversocketinterface.cpp | 26 ++++++++-- tests/CMakeLists.txt | 5 ++ tests/rate_limiter_test.cpp | 49 +++++++++++++++++++ 11 files changed, 238 insertions(+), 3 deletions(-) create mode 100644 servatrice/src/ratelimiter.cpp create mode 100644 servatrice/src/ratelimiter.h create mode 100644 tests/rate_limiter_test.cpp diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server.h b/libcockatrice_network/libcockatrice/network/server/remote/server.h index 2fca46593..f36e91598 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server.h @@ -175,6 +175,15 @@ public: { return false; } + /** @brief Record a failed login attempt from the given address; returns true if the address is now locked out. */ + virtual bool recordFailedLogin(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*/) + { + } Server_DatabaseInterface *getDatabaseInterface() const; int getNextLocalGameId() diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index c441da781..8fb47f8f7 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -527,10 +527,14 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd return Response::RespUserIsBanned; } case NotLoggedIn: + if (server->recordFailedLogin(getAddress())) { + return Response::RespTooManyRequests; + } return Response::RespWrongPassword; case WouldOverwriteOldSession: return Response::RespWouldOverwriteOldSession; case UsernameInvalid: { + server->recordFailedLogin(getAddress()); auto *re = new Response_Login; re->set_denied_reason_str(reasonStr.toStdString()); rc.setResponseExtension(re); @@ -541,8 +545,10 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd case ClientIdRequired: return Response::RespClientIdRequired; case UserIsInactive: + server->recordFailedLogin(getAddress()); return Response::RespAccountNotActivated; default: + server->clearFailedLogins(getAddress()); authState = res; usingRealPassword = needsHash; } diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index 6e4191beb..ac19a646a 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -7,6 +7,7 @@ project(Servatrice VERSION "${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}.${ set(servatrice_SOURCES src/email_parser.cpp src/main.cpp + src/ratelimiter.cpp src/servatrice.cpp src/servatrice_connection_pool.cpp src/servatrice_database_interface.cpp diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index fac743c39..7c5e5d930 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -348,6 +348,27 @@ max_users_websocket=500 ; Maximum number of users that can connect from the same IP address; useful to avoid bots, default is 4 max_users_per_address=4 +; Maximum number of failed login attempts from a single IP address before that +; address is temporarily locked out. Default is 5; set to 0 to disable. +max_login_attempts_per_ip=5 + +; Length in seconds of the sliding window used for the login lockout. Default is 900 (15 minutes). +login_attempt_window_seconds=900 + +; Maximum number of account registrations from a single IP address within the window below. +; Default is 2; set to 0 to disable. +max_registrations_per_ip=2 + +; Length in seconds of the registration window. Default is 3600 (1 hour). +registration_window_seconds=3600 + +; Maximum number of forgot-password requests from a single IP address within the window below. +; Default is 3; set to 0 to disable. +max_forgot_password_requests_per_ip=3 + +; Length in seconds of the forgot-password window. Default is 3600 (1 hour). +forgot_password_window_seconds=3600 + ; You may want to allow an unlimited number of users from a trusted source. This setting can contain a ; comma-separed list of IP addresses which will allow an unlimited number of connections from each of the ; IP addresses listed (ignoring the max_users_per_address). Default is "127.0.0.1,::1"; example: "192.73.233.244,81.4.100.74" diff --git a/servatrice/src/ratelimiter.cpp b/servatrice/src/ratelimiter.cpp new file mode 100644 index 000000000..d22b851dd --- /dev/null +++ b/servatrice/src/ratelimiter.cpp @@ -0,0 +1,42 @@ +#include "ratelimiter.h" + +#include + +bool RateLimiter::recordAttempt(const QString &key, int maxAttempts, int windowSeconds) +{ + QMutexLocker locker(&mutex); + const qint64 now = QDateTime::currentSecsSinceEpoch(); + + QList ×tamps = attempts[key]; + timestamps.append(now); + while (!timestamps.isEmpty() && timestamps.first() <= now - windowSeconds) { + timestamps.removeFirst(); + } + + return timestamps.size() > maxAttempts; +} + +bool RateLimiter::isBlocked(const QString &key, int maxAttempts, int windowSeconds) const +{ + QMutexLocker locker(&mutex); + const qint64 now = QDateTime::currentSecsSinceEpoch(); + + const auto it = attempts.constFind(key); + if (it == attempts.constEnd()) { + return false; + } + + int count = 0; + for (const qint64 ×tamp : it.value()) { + if (timestamp > now - windowSeconds) { + ++count; + } + } + return count > maxAttempts; +} + +void RateLimiter::clearAttempts(const QString &key) +{ + QMutexLocker locker(&mutex); + attempts.remove(key); +} diff --git a/servatrice/src/ratelimiter.h b/servatrice/src/ratelimiter.h new file mode 100644 index 000000000..ba2998093 --- /dev/null +++ b/servatrice/src/ratelimiter.h @@ -0,0 +1,25 @@ +#ifndef RATELIMITER_H +#define RATELIMITER_H + +#include +#include +#include +#include + +/** @brief Thread-safe per-key attempt counter 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. */ + bool recordAttempt(const QString &key, int maxAttempts, int windowSeconds); + /** @brief True if the key already has more than maxAttempts attempts within windowSeconds. */ + bool isBlocked(const QString &key, int maxAttempts, int windowSeconds) const; + /** @brief Drop all recorded attempts for the key, e.g. after a successful login. */ + void clearAttempts(const QString &key); + +private: + mutable QMutex mutex; + QMap> attempts; // key -> attempt timestamps (epoch seconds) +}; + +#endif diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index aa50e068a..0557a9889 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -1075,6 +1075,46 @@ int Servatrice::getForgotPasswordTokenLife() const return settingsCache->value("forgotpassword/tokenlife", 60).toInt(); } +bool Servatrice::recordFailedLogin(const QString &ipAddress) +{ + return rateLimiter.recordAttempt("login:" + ipAddress, getMaxLoginAttemptsPerIp(), getLoginAttemptWindowSeconds()); +} + +void Servatrice::clearFailedLogins(const QString &ipAddress) +{ + rateLimiter.clearAttempts("login:" + ipAddress); +} + +int Servatrice::getMaxLoginAttemptsPerIp() const +{ + return settingsCache->value("security/max_login_attempts_per_ip", 5).toInt(); +} + +int Servatrice::getLoginAttemptWindowSeconds() const +{ + return settingsCache->value("security/login_attempt_window_seconds", 900).toInt(); +} + +int Servatrice::getMaxRegistrationsPerIp() const +{ + return settingsCache->value("security/max_registrations_per_ip", 2).toInt(); +} + +int Servatrice::getRegistrationWindowSeconds() const +{ + return settingsCache->value("security/registration_window_seconds", 3600).toInt(); +} + +int Servatrice::getMaxForgotPasswordRequestsPerIp() const +{ + return settingsCache->value("security/max_forgot_password_requests_per_ip", 3).toInt(); +} + +int Servatrice::getForgotPasswordWindowSeconds() const +{ + return settingsCache->value("security/forgot_password_window_seconds", 3600).toInt(); +} + bool Servatrice::getEnableForgotPasswordChallenge() const { return settingsCache->value("forgotpassword/enablechallenge", false).toBool(); diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 62fb382cb..f968db4a9 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -20,6 +20,8 @@ #ifndef SERVATRICE_H #define SERVATRICE_H +#include "ratelimiter.h" + #include #include #include @@ -172,6 +174,8 @@ private: int nextShutdownMessageMinutes; QTimer *shutdownTimer; + RateLimiter rateLimiter; + mutable QMutex serverListMutex; QList serverList; void updateServerList(); @@ -275,6 +279,19 @@ public: void incRxBytes(quint64 num); void addDatabaseInterface(QThread *thread, Servatrice_DatabaseInterface *databaseInterface); + RateLimiter *getRateLimiter() + { + return &rateLimiter; + } + bool recordFailedLogin(const QString &ipAddress) override; + void clearFailedLogins(const QString &ipAddress) override; + int getMaxLoginAttemptsPerIp() const; + int getLoginAttemptWindowSeconds() const; + int getMaxRegistrationsPerIp() const; + int getRegistrationWindowSeconds() const; + int getMaxForgotPasswordRequestsPerIp() const; + int getForgotPasswordWindowSeconds() const; + bool islConnectionExists(int _serverId) const; void addIslInterface(int _serverId, IslInterface *interface); void removeIslInterface(int _serverId); diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 6ceebfca9..5f75a60cf 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -1433,9 +1433,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRegisterAccount(const C bool AbstractServerSocketInterface::tooManyRegistrationAttempts(const QString &ipAddress) { - //! \todo Implement registration attempt limiting. - Q_UNUSED(ipAddress); - return false; + return servatrice->getRateLimiter()->recordAttempt("register:" + ipAddress, servatrice->getMaxRegistrationsPerIp(), + servatrice->getRegistrationWindowSeconds()); } Response::ResponseCode AbstractServerSocketInterface::cmdActivateAccount(const Command_Activate &cmd, @@ -1788,6 +1787,17 @@ Response::ResponseCode AbstractServerSocketInterface::cmdForgotPasswordRequest(c qCDebug(AbstractServerSocketInterfaceLog) << "Received reset password request from user:" << userName; + if (servatrice->getRateLimiter()->recordAttempt("forgot:" + this->getAddress(), + servatrice->getMaxForgotPasswordRequestsPerIp(), + servatrice->getForgotPasswordWindowSeconds())) { + if (servatrice->getEnableForgotPasswordAudit()) { + sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), + "PASSWORD_RESET_REQUEST", "Too many requests from this ip address", false); + } + + return Response::RespTooManyRequests; + } + if (!servatrice->getEnableForgotPassword()) { if (servatrice->getEnableForgotPasswordAudit()) { sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), @@ -1929,6 +1939,16 @@ AbstractServerSocketInterface::cmdForgotPasswordChallenge(const Command_ForgotPa qCDebug(AbstractServerSocketInterfaceLog) << "Received reset password challenge from user:" << userName; + if (servatrice->getRateLimiter()->recordAttempt("forgot:" + this->getAddress(), + servatrice->getMaxForgotPasswordRequestsPerIp(), + servatrice->getForgotPasswordWindowSeconds())) { + if (servatrice->getEnableForgotPasswordAudit()) { + sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), + "PASSWORD_RESET_CHALLENGE", "Too many requests from this ip address", false); + } + return Response::RespTooManyRequests; + } + if (!servatrice->getEnableForgotPasswordChallenge()) { if (servatrice->getEnableForgotPasswordAudit()) { sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 857e0b041..02a7337d8 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -7,6 +7,7 @@ add_test(NAME expression_test COMMAND expression_test) add_test(NAME clamped_arithmetic_test COMMAND clamped_arithmetic_test) add_test(NAME test_age_formatting COMMAND test_age_formatting) add_test(NAME password_hash_test COMMAND password_hash_test) +add_test(NAME rate_limiter_test COMMAND rate_limiter_test) add_test(NAME server_card_counter_test COMMAND server_card_counter_test) add_test(NAME server_counter_test COMMAND server_counter_test) @@ -20,6 +21,7 @@ add_executable(expression_test expression_test.cpp) add_executable(clamped_arithmetic_test clamped_arithmetic_test.cpp) add_executable(test_age_formatting test_age_formatting.cpp) add_executable(password_hash_test password_hash_test.cpp) +add_executable(rate_limiter_test ../servatrice/src/ratelimiter.cpp rate_limiter_test.cpp) add_executable(deck_hash_performance_test deck_hash_performance_test.cpp) add_executable(server_card_counter_test server_card_counter_test.cpp) add_executable(server_counter_test server_counter_test.cpp) @@ -54,6 +56,7 @@ if(NOT GTEST_FOUND) add_dependencies(clamped_arithmetic_test gtest) add_dependencies(test_age_formatting gtest) add_dependencies(password_hash_test gtest) + add_dependencies(rate_limiter_test gtest) add_dependencies(deck_hash_performance_test gtest) add_dependencies(server_card_counter_test gtest) add_dependencies(server_counter_test gtest) @@ -71,6 +74,8 @@ target_link_libraries( target_link_libraries( password_hash_test libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} ) +target_include_directories(rate_limiter_test PRIVATE "${CMAKE_SOURCE_DIR}/servatrice/src") +target_link_libraries(rate_limiter_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}) target_link_libraries( deck_hash_performance_test libcockatrice_deck_list libcockatrice_utility Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES} diff --git a/tests/rate_limiter_test.cpp b/tests/rate_limiter_test.cpp new file mode 100644 index 000000000..ae495b122 --- /dev/null +++ b/tests/rate_limiter_test.cpp @@ -0,0 +1,49 @@ +#include "ratelimiter.h" + +#include "gtest/gtest.h" + +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)); +} + +TEST(RateLimiterTest, BlocksAttemptsOverLimit) +{ + 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)); +} + +TEST(RateLimiterTest, ClearAttempts) +{ + RateLimiter limiter; + ASSERT_TRUE(limiter.recordAttempt("ip", 0, 60)); + ASSERT_TRUE(limiter.isBlocked("ip", 0, 60)); + limiter.clearAttempts("ip"); + ASSERT_FALSE(limiter.isBlocked("ip", 0, 60)); +} + +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)); +} + +} // namespace + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}