[Security] Harden credential format acceptance and challenge nonce validation

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.

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. */
+    /** @brief Store a fresh challenge nonce bound to @p userName for the next challenge-response login attempt. */
-    void setAuthNonce(const QByteArray &nonce);
+    void setAuthNonce(const QByteArray &nonce, const QString &userName);
-    /** @brief True if nonce matches the pending one and was issued less than 60 seconds ago. */
+    /** @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;
+    bool isAuthNonceValid(const QByteArray &nonce, const QString &userName) const;
     /** @brief Invalidate the pending nonce (single-use). */
     void clearAuthNonce();
 

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$<n>$<r>$<p>$<salt>$<verifier>" (up to ~255 chars) and arbitrary
--- legacy base64 hashes, so it grows beyond the old 120-char size. varchar(255) is used as the
+-- legacy base64 hashes, so it grows beyond the old 120-char size. varchar(255) is used because
--- column type is promotion-safe and avoids the row-format change ALGORITHM=INSTANT cannot do.
+-- 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;
+UPDATE cockatrice_schema_version SET version=37 WHERE version=36;

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;
                     }
 

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.
+    // An empty credential must never reach the database: it would be accepted
-    const bool scryptFormat = !passwordNeedsHash && !PasswordHasher::isLegacyFormat(password);
+    // 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());
     }