From 4252f69d2b7f4c62373bfa9f3cd130f21a22a775 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Wed, 2 Sep 2026 20:12:44 +0200 Subject: [PATCH] [Security] Harden credential format acceptance and challenge nonce validation --- .../server/remote/server_protocolhandler.cpp | 15 ++++++++++++--- .../server/remote/server_protocolhandler.h | 9 +++++---- .../migrations/servatrice_0036_to_0037.sql | 7 ++++--- .../src/servatrice_database_interface.cpp | 9 ++++++++- servatrice/src/serversocketinterface.cpp | 19 ++++++++++++++----- 5 files changed, 43 insertions(+), 16 deletions(-) diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index 8482f9c0d..bf43d90e7 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -1,5 +1,8 @@ #include "server_protocolhandler.h" +// Challenge-response nonces are valid for at most one minute from issuance. +static constexpr qint64 kAuthNonceLifetimeSeconds = 60; + #include "game/game_config.h" #include "game/server_game.h" #include "game/server_player.h" @@ -44,20 +47,26 @@ Server_ProtocolHandler::~Server_ProtocolHandler() { } -void Server_ProtocolHandler::setAuthNonce(const QByteArray &nonce) +void Server_ProtocolHandler::setAuthNonce(const QByteArray &nonce, const QString &userName) { authNonce = nonce; + authNonceUser = userName; authNonceCreated = QDateTime::currentDateTimeUtc(); } -bool Server_ProtocolHandler::isAuthNonceValid(const QByteArray &nonce) const +bool Server_ProtocolHandler::isAuthNonceValid(const QByteArray &nonce, const QString &userName) const { - return !authNonce.isEmpty() && authNonce == nonce && authNonceCreated.secsTo(QDateTime::currentDateTimeUtc()) < 60; + // secsTo is signed, so a wall-clock step backwards (NTP correction, VM resume) + // must not make the elapsed time negative and re-validate an old nonce. + const qint64 elapsed = authNonceCreated.secsTo(QDateTime::currentDateTimeUtc()); + return !authNonce.isEmpty() && authNonce == nonce && authNonceUser == userName && authNonceCreated.isValid() && + elapsed >= 0 && elapsed < kAuthNonceLifetimeSeconds; } void Server_ProtocolHandler::clearAuthNonce() { authNonce.clear(); + authNonceUser.clear(); } // This function must only be called from the thread this object lives in. diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h index c4917e845..f27bbb73f 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h @@ -58,6 +58,7 @@ protected: bool acceptsRoomListChanges; bool idleClientWarningSent; QByteArray authNonce; + QString authNonceUser; QDateTime authNonceCreated; virtual void logDebugMessage(const QString & /* message */) { @@ -128,10 +129,10 @@ public: return databaseInterface; } - /** @brief Store a fresh challenge nonce for the next challenge-response login attempt. */ - void setAuthNonce(const QByteArray &nonce); - /** @brief True if nonce matches the pending one and was issued less than 60 seconds ago. */ - bool isAuthNonceValid(const QByteArray &nonce) const; + /** @brief Store a fresh challenge nonce bound to @p userName for the next challenge-response login attempt. */ + void setAuthNonce(const QByteArray &nonce, const QString &userName); + /** @brief True if nonce matches the pending one, was issued for @p userName, and is less than 60 seconds old. */ + bool isAuthNonceValid(const QByteArray &nonce, const QString &userName) const; /** @brief Invalidate the pending nonce (single-use). */ void clearAuthNonce(); diff --git a/servatrice/migrations/servatrice_0036_to_0037.sql b/servatrice/migrations/servatrice_0036_to_0037.sql index de3e09987..6b814da7f 100644 --- a/servatrice/migrations/servatrice_0036_to_0037.sql +++ b/servatrice/migrations/servatrice_0036_to_0037.sql @@ -1,8 +1,9 @@ -- Servatrice db migration from version 36 to version 37 -- The column must hold "$scrypt$$$

$$" (up to ~255 chars) and arbitrary --- legacy base64 hashes, so it grows beyond the old 120-char size. varchar(255) is used as the --- column type is promotion-safe and avoids the row-format change ALGORITHM=INSTANT cannot do. +-- legacy base64 hashes, so it grows beyond the old 120-char size. varchar(255) is used because +-- widening a CHAR requires a table rebuild, which ALGORITHM=INSTANT cannot perform — dropping the +-- clause lets the server pick a suitable algorithm (and any row-format change is avoided anyway). ALTER TABLE `cockatrice_users` MODIFY `password_sha512` varchar(255) NOT NULL; -UPDATE cockatrice_schema_version SET version=37 WHERE version=36; \ No newline at end of file +UPDATE cockatrice_schema_version SET version=37 WHERE version=36; diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index caa3478c8..f5c8907a3 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/servatrice/src/servatrice_database_interface.cpp @@ -357,6 +357,13 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot return UserIsInactive; } + // Fail closed on an absent stored credential: an empty key would + // otherwise authenticate anyone who can compute HMAC("", nonce). + if (correctPasswordSha512.isEmpty()) { + qCWarning(DatabaseInterfaceLog) << "Login denied: empty stored credential"; + return NotLoggedIn; + } + if (password.startsWith("$challenge$")) { // Challenge-response login: verify HMAC(stored_key, nonce) without // ever transmitting the stored credential or password hash. @@ -366,7 +373,7 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot } const QByteArray nonce = QByteArray::fromBase64(parts.at(2).toUtf8()); const QByteArray response = QByteArray::fromBase64(parts.at(3).toUtf8()); - if (nonce.isEmpty() || response.isEmpty() || !handler->isAuthNonceValid(nonce)) { + if (nonce.isEmpty() || response.isEmpty() || !handler->isAuthNonceValid(nonce, user)) { return NotLoggedIn; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 36a3e5766..926fece31 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -2524,8 +2524,16 @@ bool AbstractServerSocketInterface::tooManyRegistrationAttempts(const QString &i bool AbstractServerSocketInterface::acceptsCredentialFormat(bool passwordNeedsHash, const QString &password) const { - // "scryptFormat" means the client sent a derived verifier rather than a password to hash ourselves. - const bool scryptFormat = !passwordNeedsHash && !PasswordHasher::isLegacyFormat(password); + // An empty credential must never reach the database: it would be accepted + // as a legacy format and stored as '' (fail-open on login, see the empty + // stored-credential guard in Servatrice_DatabaseInterface). + if (password.isEmpty()) { + return false; + } + // "scryptFormat" means the client sent a derived verifier rather than a + // password to hash ourselves. parsePasswordVerifier enforces the sane-cost + // clamp, so nothing starting with '$' reaches the database unparsed. + const bool scryptFormat = !passwordNeedsHash && PasswordHasher::parsePasswordVerifier(password).isValid; // The strictness mode governs how existing legacy accounts are served, not which new-credential // formats are tolerated: a legacy-mode server must still accept scrypt verifiers, because clients @@ -2534,7 +2542,8 @@ bool AbstractServerSocketInterface::acceptsCredentialFormat(bool passwordNeedsHa if (servatrice->getAuthenticationStrictness() == Servatrice::AuthenticationStrict) { return scryptFormat; } - return true; // legacy and mixed accept either format + // legacy and mixed accept a valid scrypt verifier or a genuine legacy salt+hash. + return scryptFormat || PasswordHasher::isLegacyFormat(password); } Response::ResponseCode AbstractServerSocketInterface::cmdActivateAccount(const Command_Activate &cmd, @@ -3097,7 +3106,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con // served the legacy salt, since legacy mode only governs what NEW credentials are accepted. if (servatrice->getAuthenticationStrictness() != Servatrice::AuthenticationLegacy) { const QByteArray nonce = CryptoUtil::randomBytes(32); - setAuthNonce(nonce); + setAuthNonce(nonce, userName); re->set_nonce(nonce.constData(), nonce.size()); } } else { @@ -3113,7 +3122,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con re->set_needs_migration(false); // scrypt rows are served challenge-response in every mode so migrated accounts never lock out. const QByteArray nonce = CryptoUtil::randomBytes(32); - setAuthNonce(nonce); + setAuthNonce(nonce, userName); re->set_nonce(nonce.constData(), nonce.size()); }