[Security] Add challenge-response auth with scrypt verifiers and stop storing plaintext passwords

Challenge-response authentication: the client derives a scrypt verifier
(RFC 7914, EVP_PBE_scrypt, N=32768, r=8, p=1) and authenticates with
HMAC-SHA256(key, nonce), so neither the password nor its hash is
transmitted. The stored format becomes
"$scrypt$<n>$<r>$<p>$<salt>$<verifier>" and Response_PasswordSalt
now carries the cost parameters. Strict servers only accept scrypt
verifiers; legacy accounts are migrated after a successful login.

Fix #344 for challenge-response servers: a saved profile stores the
derived verifier under the password key instead of the plaintext password.
The connect dialog loads it without revealing it, autoconnect passes it
through, the change-password dialog no longer prefills the old password
field with it, and the client only persists the verifier when
"Save password" is checked.

Took 3 minutes

Took 1 minute

Took 10 seconds

Took 7 minutes

servatrice/migrations/servatrice_0036_to_0037.sql

@@ -0,0 +1,8 @@
+-- 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
+-- column type is promotion-safe and avoids the row-format change ALGORITHM=INSTANT cannot do.
+ALTER TABLE `cockatrice_users` MODIFY `password_sha512` varchar(255) NOT NULL;
+
+UPDATE cockatrice_schema_version SET version=37 WHERE version=36;

servatrice/servatrice.ini.example

@@ -350,6 +350,22 @@ 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
 
+; How strictly new authentication features are enforced.  Possible values:
+; * legacy: accounts that still use the legacy 1000-round SHA-512 hash keep logging in with it
+;           (they are served the legacy salt, never a challenge-response nonce) and are never
+;           auto-migrated to scrypt. Already-migrated scrypt rows keep logging in via
+;           challenge-response. New credentials may use either format;
+; * mixed: accept both legacy hashes and challenge-response authentication (default);
+; * strict: only accept challenge-response authentication from clients that support it,
+;           and reject plain password submissions.  Legacy accounts are migrated to scrypt
+;           automatically on their next successful login.
+;
+; Challenge-response verifiers are scrypt (RFC 7914, N=32768, r=8, p=1) stored as
+; "$scrypt$<n>$<r>$<p>$<salt>$<verifier>". The stored verifier is password-equivalent:
+; plaintext passwords never reach the client configuration and never go over the wire, but
+; a database dump yields credentials that can answer a login challenge directly.
+authentication_strictness=mixed
+
 ; 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"

servatrice/servatrice.sql

@@ -20,7 +20,7 @@ CREATE TABLE IF NOT EXISTS `cockatrice_schema_version` (
   PRIMARY KEY  (`version`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 DEFAULT COLLATE utf8mb4_unicode_ci;
 
-INSERT INTO cockatrice_schema_version VALUES(36);
+INSERT INTO cockatrice_schema_version VALUES(37);
 
 -- users and user data tables
 CREATE TABLE IF NOT EXISTS `cockatrice_users` (
@@ -28,7 +28,7 @@ CREATE TABLE IF NOT EXISTS `cockatrice_users` (
   `admin` tinyint(1) NOT NULL,
   `name` varchar(35) NOT NULL,
   `realname` varchar(255) NOT NULL,
-  `password_sha512` char(120) NOT NULL,
+  `password_sha512` varchar(255) NOT NULL,
   `email` varchar(255) NOT NULL,
   `country` char(2) NOT NULL,
   `avatar_bmp` mediumblob NOT NULL,

servatrice/src/servatrice.cpp

@@ -900,6 +900,18 @@ QString Servatrice::getRequiredFeatures() const
     return settingsCache->value("server/requiredfeatures", "").toString();
 }
 
+Servatrice::AuthenticationStrictness Servatrice::getAuthenticationStrictness() const
+{
+    const QString strictness = settingsCache->value("security/authentication_strictness", "mixed").toString();
+    if (strictness == "strict") {
+        return AuthenticationStrict;
+    }
+    if (strictness == "legacy") {
+        return AuthenticationLegacy;
+    }
+    return AuthenticationMixed;
+}
+
 QString Servatrice::getDBTypeString() const
 {
     if (QProcessEnvironment::systemEnvironment().contains("DATABASE_URL")) {

servatrice/src/servatrice.h

@@ -140,6 +140,12 @@ public:
         AuthenticationSql,
         AuthenticationPassword
     };
+    enum AuthenticationStrictness
+    {
+        AuthenticationLegacy,
+        AuthenticationMixed,
+        AuthenticationStrict
+    };
 private slots:
     void statusUpdate();
     void shutdownTimeout();
@@ -219,6 +225,10 @@ public:
     {
         return serverRequiredFeatureList;
     }
+    bool requiresChallengeResponseAuth() const override
+    {
+        return getAuthenticationStrictness() == AuthenticationStrict;
+    }
     QString getServerName() const;
     QString getLoginMessage() const override
     {
@@ -240,6 +250,7 @@ public:
     {
         return authenticationMethod;
     }
+    AuthenticationStrictness getAuthenticationStrictness() const;
     bool permitUnregisteredUsers() const override
     {
         return authenticationMethod != AuthenticationNone;

servatrice/src/servatrice_database_interface.cpp

@@ -356,6 +356,47 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot
                     qCWarning(DatabaseInterfaceLog) << "Login denied: user not active";
                     return UserIsInactive;
                 }
+
+                if (password.startsWith("$challenge$")) {
+                    // Challenge-response login: verify HMAC(stored_key, nonce) without
+                    // ever transmitting the stored credential or password hash.
+                    const QStringList parts = password.split("$");
+                    if (parts.size() != 4) {
+                        return NotLoggedIn;
+                    }
+                    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)) {
+                        return NotLoggedIn;
+                    }
+
+                    QByteArray key;
+                    if (PasswordHasher::isLegacyFormat(correctPasswordSha512)) {
+                        key = correctPasswordSha512.toUtf8();
+                    } else {
+                        // Design note: the stored scrypt verifier IS the challenge-response key, so a
+                        // database dump yields credentials that can answer a login challenge directly.
+                        // We deliberately accept this trade: it removes plaintext passwords from the
+                        // client config and from the wire, but does not protect against DB compromise.
+                        // A password-equivalent proof scheme (e.g. SRP-6a/OPAQUE) would be the proper
+                        // escalation and is out of scope here.
+                        const PasswordVerifier verifier = PasswordHasher::parsePasswordVerifier(correctPasswordSha512);
+                        if (!verifier.isValid) {
+                            return NotLoggedIn;
+                        }
+                        key = verifier.verifier;
+                    }
+
+                    const QByteArray expected = PasswordHasher::computeResponse(key, nonce);
+                    handler->clearAuthNonce();
+                    if (PasswordHasher::constantTimeEquals(expected, response)) {
+                        qCDebug(DatabaseInterfaceLog) << "Login accepted: challenge-response password right";
+                        return PasswordRight;
+                    }
+                    qCDebug(DatabaseInterfaceLog) << "Login denied: challenge-response password wrong";
+                    return NotLoggedIn;
+                }
+
                 QString hashedPassword;
                 if (passwordNeedsHash) {
                     hashedPassword = PasswordHasher::computeHash(password, correctPasswordSha512.left(16));
@@ -558,6 +599,48 @@ QString Servatrice_DatabaseInterface::getUserSalt(const QString &user)
     return {};
 }
 
+QString Servatrice_DatabaseInterface::getUserPasswordData(const QString &user)
+{
+    if (server->getAuthenticationMethod() != Servatrice::AuthenticationSql) {
+        return {};
+    }
+
+    checkSql();
+
+    QSqlQuery *query = prepareQuery("SELECT password_sha512 FROM {prefix}_users WHERE name = :name");
+    query->bindValue(":name", user);
+    if (!execSqlQuery(query)) {
+        return {};
+    }
+
+    if (!query->next()) {
+        return {};
+    }
+
+    return query->value(0).toString();
+}
+
+bool Servatrice_DatabaseInterface::submitPasswordVerifier(const QString &user, const QString &passwordVerifier)
+{
+    if (server->getAuthenticationMethod() != Servatrice::AuthenticationSql) {
+        return false;
+    }
+
+    checkSql();
+
+    // Only migrate accounts that still use the legacy format; the query is a no-op otherwise.
+    QSqlQuery *query = prepareQuery(
+        "update {prefix}_users set password_sha512 = :verifier where name = :user and password_sha512 not like '$%'");
+    query->bindValue(":verifier", passwordVerifier);
+    query->bindValue(":user", user);
+    if (!execSqlQuery(query)) {
+        qCWarning(DatabaseInterfaceLog) << "Failed to submit password verifier for user" << user << query->lastError();
+        return false;
+    }
+    // The guard makes a re-migration a no-op; only report success when a row was actually updated.
+    return query->numRowsAffected() > 0;
+}
+
 int Servatrice_DatabaseInterface::getUserIdInDB(const QString &name)
 {
     if (server->getAuthenticationMethod() == Servatrice::AuthenticationSql) {
@@ -1139,13 +1222,13 @@ bool Servatrice_DatabaseInterface::changeUserPassword(const QString &user,
         return false;
     }
 
-    const QString correctPasswordSha512 = passwordQuery->value(0).toString();
-    QString oldPasswordSha512 = oldPassword;
-    if (oldPasswordNeedsHash) {
-        QString salt = correctPasswordSha512.left(16);
-        oldPasswordSha512 = PasswordHasher::computeHash(oldPassword, salt);
-    }
-    if (correctPasswordSha512 != oldPasswordSha512) {
+    const QString storedPassword = passwordQuery->value(0).toString();
+    // oldPasswordNeedsHash means the client sent the old password in plaintext. Verify it
+    // against whatever is stored: legacy salt+hash rows or already-migrated scrypt rows
+    // (which must NOT be re-hashed with a salt torn out of the "$scrypt$..." string).
+    const bool oldPasswordMatches = oldPasswordNeedsHash ? PasswordHasher::verifyPassword(oldPassword, storedPassword)
+                                                         : (oldPassword == storedPassword);
+    if (!oldPasswordMatches) {
         return false;
     }
 

servatrice/src/servatrice_database_interface.h

@@ -13,7 +13,7 @@
 #include <server.h>
 #include <server_database_interface.h>
 
-#define DATABASE_SCHEMA_VERSION 36
+#define DATABASE_SCHEMA_VERSION 37
 
 class Servatrice;
 
@@ -65,6 +65,8 @@ public:
     bool activeUserExists(const QString &user) override;
     bool userExists(const QString &user) override;
     QString getUserSalt(const QString &user) override;
+    QString getUserPasswordData(const QString &user) override;
+    bool submitPasswordVerifier(const QString &user, const QString &passwordVerifier) override;
     int getUserIdInDB(const QString &name);
     QMap<QString, ServerInfo_User> getBuddyList(const QString &name) override;
     QMap<QString, ServerInfo_User> getIgnoreList(const QString &name) override;

servatrice/src/serversocketinterface.cpp

@@ -107,6 +107,7 @@
 #include <libcockatrice/protocol/pb/serverinfo_user.pb.h>
 #include <libcockatrice/protocol/pb/serverinfo_user_alt.pb.h>
 #include <libcockatrice/protocol/pb/serverinfo_user_session.pb.h>
+#include <libcockatrice/utility/cryptoutil.h>
 #include <libcockatrice/utility/passwordhasher.h>
 #include <libcockatrice/utility/string_limits.h>
 #include <libcockatrice/utility/warning_categories.h>
@@ -139,7 +140,14 @@ bool AbstractServerSocketInterface::initSession()
     identEvent.set_server_version(VERSION_STRING);
     identEvent.set_protocol_version(protocolVersion);
     if (servatrice->getAuthenticationMethod() == Servatrice::AuthenticationSql) {
-        identEvent.set_server_options(Event_ServerIdentification::SupportsPasswordHash);
+        // Challenge-response is advertised in every strictness mode: legacy accounts keep
+        // logging in with the legacy hash, but already-migrated scrypt rows are always
+        // served challenge-response (authentication_strictness only governs NEW credentials).
+        Event_ServerIdentification::ServerOptions serverOptions =
+            static_cast<Event_ServerIdentification::ServerOptions>(
+                Event_ServerIdentification::SupportsPasswordHash |
+                Event_ServerIdentification::SupportsChallengeResponseAuth);
+        identEvent.set_server_options(serverOptions);
     }
     SessionEvent *identSe = prepareSessionEvent(identEvent);
     sendProtocolItem(*identSe);
@@ -257,6 +265,8 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedSessionComm
             return cmdReportAddComment(cmd.GetExtension(Command_ReportAddComment::ext), rc);
         case SessionCommand::REPORT_DETAILS:
             return cmdReportDetails(cmd.GetExtension(Command_ReportDetails::ext), rc);
+        case SessionCommand::SUBMIT_PASSWORD_VERIFIER:
+            return cmdSubmitPasswordVerifier(cmd.GetExtension(Command_SubmitPasswordVerifier::ext), rc);
         default:
             return Response::RespFunctionNotAllowed;
     }
@@ -2461,6 +2471,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRegisterAccount(const C
         password = QString::fromStdString(cmd.hashed_password());
     }
 
+    // Reject credential formats the configured authentication strictness does not accept.
+    if (!acceptsCredentialFormat(passwordNeedsHash, password)) {
+        return Response::RespClientUpdateRequired;
+    }
+
     bool requireEmailActivation = settingsCache->value("registration/requireemailactivation", true).toBool();
     bool regSucceeded = sqlInterface->registerUser(userName, realName, password, passwordNeedsHash, parsedEmailAddress,
                                                    country, !requireEmailActivation);
@@ -2507,6 +2522,21 @@ bool AbstractServerSocketInterface::tooManyRegistrationAttempts(const QString &i
     return false;
 }
 
+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);
+
+    // 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
+    // derive them whenever challenge-response is advertised (and it must be, so already-migrated
+    // scrypt rows keep logging in). strict is the only mode that rejects legacy formats.
+    if (servatrice->getAuthenticationStrictness() == Servatrice::AuthenticationStrict) {
+        return scryptFormat;
+    }
+    return true; // legacy and mixed accept either format
+}
+
 Response::ResponseCode AbstractServerSocketInterface::cmdActivateAccount(const Command_Activate &cmd,
                                                                          ResponseContainer & /*rc*/)
 {
@@ -2841,6 +2871,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdAccountPassword(const C
         newPassword = QString::fromStdString(cmd.hashed_new_password());
     }
 
+    // Reject new credential formats the configured authentication strictness does not accept.
+    if (!acceptsCredentialFormat(newPasswordNeedsHash, newPassword)) {
+        return Response::RespClientUpdateRequired;
+    }
+
     QString userName = QString::fromStdString(userInfo->name());
     if (!databaseInterface->changeUserPassword(userName, oldPassword, true, newPassword, newPasswordNeedsHash)) {
         return Response::RespWrongPassword;
@@ -2978,6 +3013,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdForgotPasswordReset(con
         password = QString::fromStdString(cmd.hashed_new_password());
     }
 
+    // Reject new credential formats the configured authentication strictness does not accept.
+    if (!acceptsCredentialFormat(passwordNeedsHash, password)) {
+        return Response::RespClientUpdateRequired;
+    }
+
     if (sqlInterface->changeUserPassword(nameFromStdString(cmd.user_name()), password, passwordNeedsHash)) {
         if (servatrice->getEnableForgotPasswordAudit()) {
             sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(),
@@ -3038,8 +3078,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con
                                                                              ResponseContainer &rc)
 {
     const QString userName = nameFromStdString(cmd.user_name());
-    QString passwordSalt = sqlInterface->getUserSalt(userName);
-    if (passwordSalt.isEmpty()) {
+    const QString storedPasswordData = sqlInterface->getUserPasswordData(userName);
+    if (storedPasswordData.isEmpty()) {
         if (server->getRegOnlyServerEnabled()) {
             return Response::RespRegistrationRequired;
         } else {
@@ -3047,8 +3087,36 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con
             return Response::RespOk;
         }
     }
+
     auto *re = new Response_PasswordSalt;
-    re->set_password_salt(passwordSalt.toStdString());
+    if (PasswordHasher::isLegacyFormat(storedPasswordData)) {
+        re->set_password_salt(storedPasswordData.left(16).toStdString());
+        re->set_needs_migration(true);
+        // Legacy rows get a challenge-response nonce only outside legacy mode (there the client
+        // logs in with the legacy hash and the account is migrated). In legacy mode the row is
+        // 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);
+            re->set_nonce(nonce.constData(), nonce.size());
+        }
+    } else {
+        const PasswordVerifier verifier = PasswordHasher::parsePasswordVerifier(storedPasswordData);
+        if (!verifier.isValid) {
+            delete re;
+            return Response::RespContextError;
+        }
+        re->set_password_salt(QString(verifier.salt.toBase64()).toStdString());
+        re->set_n(verifier.n);
+        re->set_r(verifier.r);
+        re->set_p(verifier.p);
+        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);
+        re->set_nonce(nonce.constData(), nonce.size());
+    }
+
     rc.setResponseExtension(re);
     return Response::RespOk;
 }
@@ -3147,6 +3215,39 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReport(const Command_Re
     return Response::RespOk;
 }
 
+Response::ResponseCode
+AbstractServerSocketInterface::cmdSubmitPasswordVerifier(const Command_SubmitPasswordVerifier &cmd,
+                                                         ResponseContainer & /*rc*/)
+{
+    if (authState != PasswordRight) {
+        return Response::RespLoginNeeded;
+    }
+
+    // Limit to the size of the database column (password_sha512 varchar(255)).
+    constexpr int MAX_PASSWORD_VERIFIER_LENGTH = 255;
+    const QString passwordVerifier = QString::fromStdString(cmd.password_verifier());
+    if (passwordVerifier.isEmpty() || passwordVerifier.length() > MAX_PASSWORD_VERIFIER_LENGTH ||
+        PasswordHasher::isLegacyFormat(passwordVerifier)) {
+        return Response::RespContextError;
+    }
+
+    // Reject unparseable or hostile cost parameters before they reach the database.
+    const PasswordVerifier parsedVerifier = PasswordHasher::parsePasswordVerifier(passwordVerifier);
+    if (!parsedVerifier.isValid) {
+        qCWarning(AbstractServerSocketInterfaceLog)
+            << "Rejecting password verifier submission with invalid or insane cost parameters";
+        return Response::RespContextError;
+    }
+
+    if (!sqlInterface->submitPasswordVerifier(QString::fromStdString(userInfo->name()), passwordVerifier)) {
+        return Response::RespContextError;
+    }
+
+    qCDebug(AbstractServerSocketInterfaceLog)
+        << "Password verifier migrated for user" << QString::fromStdString(userInfo->name());
+    return Response::RespOk;
+}
+
 // ADMIN FUNCTIONS.
 // Permission is checked by the calling function.
 

servatrice/src/serversocketinterface.h

@@ -80,6 +80,7 @@ signals:
 protected:
     void logDebugMessage(const QString &message) override;
     bool tooManyRegistrationAttempts(const QString &ipAddress);
+    bool acceptsCredentialFormat(bool passwordNeedsHash, const QString &password) const;
 
     virtual void writeToSocket(QByteArray &data) = 0;
     virtual void flushSocket() = 0;
@@ -145,6 +146,7 @@ private:
     Response::ResponseCode cmdReportDetails(const Command_ReportDetails &cmd, ResponseContainer &rc);
     Response::ResponseCode cmdReportAddComment(const Command_ReportAddComment &cmd, ResponseContainer &rc);
     Response::ResponseCode cmdReplayDownloadByGameId(const Command_ReplayDownloadByGameId &cmd, ResponseContainer &rc);
+    Response::ResponseCode cmdSubmitPasswordVerifier(const Command_SubmitPasswordVerifier &cmd, ResponseContainer &rc);
     Response::ResponseCode
     processExtendedSessionCommand(int cmdType, const SessionCommand &cmd, ResponseContainer &rc) override;
     Response::ResponseCode