[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

.ci/Arch/Dockerfile

@@ -8,6 +8,7 @@ RUN pacman --sync --refresh --sysupgrade --needed --noconfirm \
         gtest \
         mariadb-libs \
         ninja \
+        openssl \
         protobuf \
         qt6-base \
         qt6-declarative \

.ci/Debian12/Dockerfile

@@ -15,6 +15,7 @@ RUN apt-get update && \
         libprotobuf-dev \
         libqt6multimedia6 \
         libqt6sql6-mysql \
+        libssl-dev \
         ninja-build \
         protobuf-compiler \
         qt6-image-formats-plugins \

.ci/Debian13/Dockerfile

@@ -16,6 +16,7 @@ RUN apt-get update && \
         libprotobuf-dev \
         libqt6multimedia6 \
         libqt6sql6-mysql \
+        libssl-dev \
         ninja-build \
         protobuf-compiler \
         qt6-image-formats-plugins \

.ci/Fedora43/Dockerfile

@@ -7,6 +7,7 @@ RUN dnf install -y \
         git \
         mariadb-devel \
         ninja-build \
+        openssl-devel \
         protobuf-devel \
         qt6-{qtdeclarative,qtshadertools,qttools,qtsvg,qtmultimedia,qtwebsockets}-devel \
         qt6-qtimageformats \

.ci/Fedora44/Dockerfile

@@ -7,6 +7,7 @@ RUN dnf install -y \
         git \
         mariadb-devel \
         ninja-build \
+        openssl-devel \
         protobuf-devel \
         qt6-{qtdeclarative,qtshadertools,qttools,qtsvg,qtmultimedia,qtwebsockets}-devel \
         qt6-qtimageformats \

.ci/Servatrice_Debian12/Dockerfile

@@ -12,6 +12,7 @@ RUN apt-get update && \
         libmariadb-dev-compat \
         libprotobuf-dev \
         libqt6sql6-mysql \
+        libssl-dev \
         ninja-build \
         protobuf-compiler \
         qt6-tools-dev \

.ci/Ubuntu24.04/Dockerfile

@@ -15,6 +15,7 @@ RUN apt-get update && \
         libprotobuf-dev \
         libqt6multimedia6 \
         libqt6sql6-mysql \
+        libssl-dev \
         ninja-build \
         protobuf-compiler \
         qt6-image-formats-plugins \

.ci/Ubuntu26.04/Dockerfile

@@ -16,6 +16,7 @@ RUN apt-get update && \
         libprotobuf-dev \
         libqt6multimedia6 \
         libqt6sql6-mysql \
+        libssl-dev \
         ninja-build \
         protobuf-compiler \
         qt6-image-formats-plugins \

.github/workflows/codeql.yml

@@ -40,7 +40,7 @@ jobs:
 
     steps:
     - name: "Checkout repository"
-      uses: actions/checkout@v6
+      uses: actions/checkout@v7
 
     - name: "Initialize CodeQL"
       uses: github/codeql-action/init@v4

.github/workflows/docker-release.yml

@@ -127,7 +127,7 @@ jobs:
 
     steps:
       - name: "Download digests"
-        uses: actions/download-artifact@v7
+        uses: actions/download-artifact@v8
         with:
           path: ${{ runner.temp }}/digests
           pattern: digest-*

CMakeLists.txt

@@ -239,11 +239,6 @@ if(WIN32)
   find_package(OpenSSL REQUIRED)
   if(OPENSSL_FOUND)
     include_directories(${OPENSSL_INCLUDE_DIRS})
-  else()
-    message(
-      WARNING
-        "Could not find OpenSSL runtime libraries. They are not required for compiling, but needs to be available at runtime."
-    )
   endif()
 endif()
 

Dockerfile

@@ -14,6 +14,7 @@ RUN apt-get update \
       libmariadb-dev-compat \
       libprotobuf-dev \
       libqt6sql6-mysql \
+      libssl-dev \
       qt6-websockets-dev \
       protobuf-compiler \
       qt6-tools-dev \
@@ -42,6 +43,7 @@ RUN apt-get update \
       libprotobuf32t64 \
       libqt6sql6-mysql \
       libqt6websockets6 \
+      libssl3 \
     && apt-get clean \
     && rm -rf /var/lib/apt/lists/*
 

cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp

@@ -102,12 +102,8 @@ void ConnectionController::connectToServer()
     }
 }
 
-void ConnectionController::onPasswordVerifierReady(const QString &hostname,
-                                                   const QString &userName,
-                                                   const QString &verifier)
+void ConnectionController::onPasswordVerifierReady(const QString &verifier)
 {
-    Q_UNUSED(hostname);
-    Q_UNUSED(userName);
     if (pendingSavePassword) {
         SettingsCache::instance().servers().setServerPassword(pendingSaveName, verifier);
     }

cockatrice/src/client/network/connection_controller/remote_connection_controller.h

@@ -87,7 +87,7 @@ private slots:
     void onPromptForgotPasswordChallenge();
 
     // Persists the derived scrypt verifier after a successful challenge-response login
-    void onPasswordVerifierReady(const QString &hostname, const QString &userName, const QString &verifier);
+    void onPasswordVerifierReady(const QString &verifier);
 
 private:
     void wireClientSignals();

cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp

@@ -333,6 +333,11 @@ void DlgConnect::actOk()
 {
     ServersSettings &servers = SettingsCache::instance().servers();
 
+    // Never write a newly typed plaintext password to disk when a verifier is already stored:
+    // the typed value is used for this connection and a fresh verifier is persisted after a
+    // successful login. Without a stored verifier we keep the previous (plaintext legacy) behavior.
+    const QString passwordToSave = storedVerifier.isEmpty() ? passwordEdit->text() : storedVerifier;
+
     if (newHostButton->isChecked()) {
         if (saveEdit->text().isEmpty()) {
             QMessageBox::critical(this, tr("Connection Warning"), tr("You need to name your new connection profile."));
@@ -340,13 +345,10 @@ void DlgConnect::actOk()
         }
 
         servers.addNewServer(saveEdit->text().trimmed(), hostEdit->text().trimmed(), portEdit->text().trimmed(),
-                             playernameEdit->text().trimmed(),
-                             passwordEdit->text().isEmpty() ? storedVerifier : passwordEdit->text(),
-                             savePasswordCheckBox->isChecked());
+                             playernameEdit->text().trimmed(), passwordToSave, savePasswordCheckBox->isChecked());
     } else {
         servers.updateExistingServer(saveEdit->text().trimmed(), hostEdit->text().trimmed(), portEdit->text().trimmed(),
-                                     playernameEdit->text().trimmed(),
-                                     passwordEdit->text().isEmpty() ? storedVerifier : passwordEdit->text(),
+                                     playernameEdit->text().trimmed(), passwordToSave,
                                      savePasswordCheckBox->isChecked());
     }
 

cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp

@@ -1,9 +1,17 @@
 #include "dlg_convert_deck_to_cod_format.h"
 
+#include "../../../client/settings/cache_settings.h"
+#include "../../deck_loader/deck_loader.h"
+
 #include <QCheckBox>
 #include <QDialogButtonBox>
+#include <QDir>
+#include <QFile>
+#include <QFileInfo>
 #include <QLabel>
+#include <QMessageBox>
 #include <QVBoxLayout>
+#include <libcockatrice/settings/visual_deck_storage_settings.h>
 
 DialogConvertDeckToCodFormat::DialogConvertDeckToCodFormat(QWidget *parent) : QDialog(parent)
 {
@@ -38,3 +46,71 @@ bool DialogConvertDeckToCodFormat::dontAskAgain() const
 {
     return dontAskAgainCheckbox->isChecked();
 }
+
+namespace
+{
+
+bool confirmOverwriteIfExists(QWidget *parent, const QString &filePath)
+{
+    QFileInfo fileInfo(filePath);
+    QString newFileName = QDir::toNativeSeparators(fileInfo.path() + "/" + fileInfo.completeBaseName() + ".cod");
+
+    if (QFile::exists(newFileName)) {
+        QMessageBox::StandardButton reply =
+            QMessageBox::question(parent, QObject::tr("Overwrite Existing File?"),
+                                  QObject::tr("A .cod version of this deck already exists. Overwrite it?"),
+                                  QMessageBox::Yes | QMessageBox::No);
+        return reply == QMessageBox::Yes;
+    }
+    return true; // Safe to proceed
+}
+
+} // namespace
+
+bool DialogConvertDeckToCodFormat::promptIfRequired(QWidget *parent,
+                                                    const QString &filePath,
+                                                    const std::function<bool()> &convert)
+{
+    if (DeckFileFormat::getFormatFromName(filePath) == DeckFileFormat::Cockatrice) {
+        return true;
+    }
+
+    // Retrieve saved preference if the prompt is disabled
+    if (!SettingsCache::instance().visualDeckStorage().getVisualDeckStoragePromptForConversion()) {
+        if (!SettingsCache::instance().visualDeckStorage().getVisualDeckStorageAlwaysConvert()) {
+            return false;
+        }
+
+        if (!confirmOverwriteIfExists(parent, filePath)) {
+            return false;
+        }
+
+        return convert();
+    }
+
+    // Show the dialog to the user
+    DialogConvertDeckToCodFormat conversionDialog(parent);
+    if (conversionDialog.exec() != QDialog::Accepted) {
+        SettingsCache::instance().visualDeckStorage().setVisualDeckStoragePromptForConversion(
+            !conversionDialog.dontAskAgain());
+        SettingsCache::instance().visualDeckStorage().setVisualDeckStorageAlwaysConvert(false);
+
+        return false;
+    }
+
+    // Try to convert file
+    if (!confirmOverwriteIfExists(parent, filePath)) {
+        return false;
+    }
+
+    if (!convert()) {
+        return false;
+    }
+
+    if (conversionDialog.dontAskAgain()) {
+        SettingsCache::instance().visualDeckStorage().setVisualDeckStoragePromptForConversion(false);
+        SettingsCache::instance().visualDeckStorage().setVisualDeckStorageAlwaysConvert(true);
+    }
+
+    return true;
+}

cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.h

@@ -13,6 +13,9 @@
 #include <QDialogButtonBox>
 #include <QLabel>
 #include <QVBoxLayout>
+#include <functional>
+
+class QWidget;
 
 class DialogConvertDeckToCodFormat : public QDialog
 {
@@ -24,6 +27,21 @@ public:
 
     [[nodiscard]] bool dontAskAgain() const;
 
+    /**
+     * @brief Checks whether the deck file at \a filePath can store tags.
+     *
+     * If the file is not a .cod deck, prompts the user for conversion to the
+     * Cockatrice format, honoring the saved "always convert / don't ask again"
+     * preference. On acceptance \a convert is called to perform the conversion.
+     *
+     * @param parent The widget to parent the prompt to.
+     * @param filePath The path of the deck file to check.
+     * @param convert Called to convert the deck once the user agrees.
+     * @return true if tags can be stored (no conversion needed, or the conversion
+     *         was performed), false if the user declined to convert.
+     */
+    static bool promptIfRequired(QWidget *parent, const QString &filePath, const std::function<bool()> &convert);
+
 private:
     QVBoxLayout *layout;
     QLabel *label;

cockatrice/src/interface/widgets/server/user/user_info_popup.cpp

@@ -525,6 +525,13 @@ void UserInfoPopup::rebuildActionButtons(const ServerInfo_User &userInfo, bool o
     connect(games, &QPushButton::clicked, this, [this, name] { emit showGamesRequested(name); });
     add(games);
 
+    // ── Invite (only while the inviter has a joinable game for this user) ────
+    if (!isSelf && online && gameInviteAvailable && gameInviteAvailable(name)) {
+        auto *invite = makeBtn(tr("Invite"), tr("Invite to your game"), actionArea, theme);
+        connect(invite, &QPushButton::clicked, this, [this, name] { emit inviteRequested(name); });
+        add(invite);
+    }
+
     // ── Buddy / ignore (registered users only) ────────────────────────────────
     if (!isSelf && isReg) {
         if (isBuddy) {

cockatrice/src/interface/widgets/server/user/user_info_popup.h

@@ -9,6 +9,7 @@
 #include <QMap>
 #include <QPixmap>
 #include <QStandardItemModel>
+#include <functional>
 #include <libcockatrice/network/server/remote/user_level.h>
 #include <libcockatrice/protocol/pb/response.pb.h>
 #include <libcockatrice/protocol/pb/serverinfo_game.pb.h>
@@ -149,6 +150,17 @@ public:
     /** Re-pulls the avatar/card art for the currently shown user (e.g. after it loads). */
     void refreshHeader();
 
+    /**
+     * Sets a predicate evaluated on every action-button rebuild. It receives
+     * the name of the user the popup currently shows; when it returns true an
+     * "Invite" button is shown. The popup itself never resolves the invite
+     * link, it just forwards the request.
+     */
+    void setGameInviteAvailable(std::function<bool(const QString &userName)> available)
+    {
+        gameInviteAvailable = std::move(available);
+    }
+
 signals:
     void mouseEnteredPopup();
     void mouseLeftPopup();
@@ -159,6 +171,7 @@ signals:
 
     // ── Action signals — connect to UserContextMenu::exec*() ──────────────────
     void chatRequested(const QString &userName);
+    void inviteRequested(const QString &userName);
     void detailsRequested(const QString &userName);
     void showGamesRequested(const QString &userName);
     void addBuddyRequested(const QString &userName);
@@ -200,6 +213,7 @@ private:
     QString currentUser;
     ServerInfo_User currentUserInfo;
     bool currentOnline = false;
+    std::function<bool(const QString &userName)> gameInviteAvailable;
 
     UserInfoHeaderWidget *header;
     QWidget *actionArea; ///< rebuilt per user

cockatrice/src/interface/widgets/server/user/user_list_widget.cpp

@@ -345,6 +345,11 @@ UserListWidget::UserListWidget(TabSupervisor *_tabSupervisor,
                                           &cardArtProvider->cache(), &cardArtParamsMap,
                                           window()); // parented to main window so it floats above siblings
 
+        // The invite availability is scoped to the room this list belongs to,
+        // and gated on the room's buddy-only setting for the hovered user.
+        userInfoPopup->setGameInviteAvailable(
+            [this](const QString &userName) { return userContextMenu->hasGameInviteLink(userName); });
+
         userInfoPopup->hide();
         userInfoPopup->setWindowOpacity(0.0);
         userInfoPopup->installEventFilter(this);
@@ -662,6 +667,8 @@ void UserListWidget::connectPopupSignals()
 
     // Wire all action signals to UserContextMenu::exec*()
     connect(userInfoPopup, &UserInfoPopup::chatRequested, userContextMenu, &UserContextMenu::execChat);
+    connect(userInfoPopup, &UserInfoPopup::inviteRequested, this,
+            [this](const QString &userName) { userContextMenu->execInvite(userName); });
     connect(userInfoPopup, &UserInfoPopup::detailsRequested, userContextMenu, &UserContextMenu::execDetails);
     connect(userInfoPopup, &UserInfoPopup::showGamesRequested, userContextMenu, &UserContextMenu::execShowGames);
     connect(userInfoPopup, &UserInfoPopup::addBuddyRequested, userContextMenu, &UserContextMenu::execAddToBuddy);

cockatrice/src/interface/widgets/server/user/user_list_widget.h

@@ -22,6 +22,7 @@
 #include <QTextEdit>
 #include <QTreeWidgetItem>
 #include <functional>
+#include <libcockatrice/network/server/remote/user_level.h>
 #include <libcockatrice/protocol/pb/moderator_commands.pb.h>
 
 class QTreeWidget;

cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp

@@ -1091,7 +1091,8 @@ QList<GameInviteOption> TabSupervisor::getGameInviteLinksForRoom(int roomId) con
     // The inviter may be in several games of the same room (hosting one and
     // spectating another, for example). Return every game so the caller can
     // let the user choose which one to invite to.
-    for (TabGame *tab : gameTabs) {
+    for (auto it = gameTabs.cbegin(); it != gameTabs.cend(); ++it) {
+        TabGame *tab = it.value();
         GameMetaInfo *metaInfo = tab->getGame()->getGameMetaInfo();
         if (metaInfo->proto().room_id() != roomId) {
             continue;

cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_widget.cpp

@@ -10,8 +10,6 @@
 #include "../visual_deck_storage_widget.h"
 #include "deck_preview_deck_tags_display_widget.h"
 
-#include <QDir>
-#include <QFile>
 #include <QFileInfo>
 #include <QInputDialog>
 #include <QLabel>
@@ -499,21 +497,6 @@ void DeckPreviewWidget::actDeleteFile()
     // The folder widget removes this preview once the row is gone.
 }
 
-static bool confirmOverwriteIfExists(QWidget *parent, const QString &filePath)
-{
-    QFileInfo fileInfo(filePath);
-    QString newFileName = QDir::toNativeSeparators(fileInfo.path() + "/" + fileInfo.completeBaseName() + ".cod");
-
-    if (QFile::exists(newFileName)) {
-        QMessageBox::StandardButton reply =
-            QMessageBox::question(parent, QObject::tr("Overwrite Existing File?"),
-                                  QObject::tr("A .cod version of this deck already exists. Overwrite it?"),
-                                  QMessageBox::Yes | QMessageBox::No);
-        return reply == QMessageBox::Yes;
-    }
-    return true; // Safe to proceed
-}
-
 /**
  * Checks if the deck's file format supports tags.
  * If not, then prompt the user for file conversion.
@@ -521,45 +504,8 @@ static bool confirmOverwriteIfExists(QWidget *parent, const QString &filePath)
  */
 bool DeckPreviewWidget::promptFileConversionIfRequired()
 {
-    if (DeckFileFormat::getFormatFromName(filePath) == DeckFileFormat::Cockatrice) {
-        return true;
-    }
-
-    // Retrieve saved preference if the prompt is disabled
-    if (!SettingsCache::instance().visualDeckStorage().getVisualDeckStoragePromptForConversion()) {
-        if (!SettingsCache::instance().visualDeckStorage().getVisualDeckStorageAlwaysConvert()) {
-            return false;
-        }
-
-        if (!confirmOverwriteIfExists(this, filePath)) {
-            return false;
-        }
-
+    return DialogConvertDeckToCodFormat::promptIfRequired(this, filePath, [this] {
         model->convertToCockatriceFormat(row());
         return true;
-    }
-
-    // Show the dialog to the user
-    DialogConvertDeckToCodFormat conversionDialog(this);
-    if (conversionDialog.exec() != QDialog::Accepted) {
-        SettingsCache::instance().visualDeckStorage().setVisualDeckStoragePromptForConversion(
-            !conversionDialog.dontAskAgain());
-        SettingsCache::instance().visualDeckStorage().setVisualDeckStorageAlwaysConvert(false);
-
-        return false;
-    }
-
-    // Try to convert file
-    if (!confirmOverwriteIfExists(this, filePath)) {
-        return false;
-    }
-
-    model->convertToCockatriceFormat(row());
-
-    if (conversionDialog.dontAskAgain()) {
-        SettingsCache::instance().visualDeckStorage().setVisualDeckStoragePromptForConversion(false);
-        SettingsCache::instance().visualDeckStorage().setVisualDeckStorageAlwaysConvert(true);
-    }
-
-    return true;
+    });
 }

libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp

@@ -135,6 +135,9 @@ void RemoteClient::processServerIdentificationEvent(const Event_ServerIdentifica
         cmdForgotPasswordReset.set_token(token.toStdString());
         if (!password.isEmpty() && serverSupportsChallengeResponse) {
             hashedPassword = PasswordHasher::generatePasswordVerifier(password);
+            // Only this branch yields a challenge-response verifier worth persisting; the
+            // legacy-hash and plaintext branches below must not be saved under the password key.
+            derivedVerifier = hashedPassword;
             cmdForgotPasswordReset.set_hashed_new_password(hashedPassword.toStdString());
         } else if (!password.isEmpty() && serverSupportsPasswordHash) {
             auto passwordSalt = PasswordHasher::generateRandomSalt();
@@ -284,7 +287,7 @@ void RemoteClient::submitPasswordVerifierResponse(const Response &response)
     if (response.response_code() == Response::RespOk) {
         qCDebug(RemoteClientLog) << "Password verifier migrated successfully";
         if (!pendingVerifier.isEmpty()) {
-            emit sigPasswordVerifierReady(lastHostname, userName, pendingVerifier);
+            emit sigPasswordVerifierReady(pendingVerifier);
             pendingVerifier.clear();
         }
     } else {
@@ -319,10 +322,22 @@ void RemoteClient::passwordSaltResponse(const Response &response)
                 }
                 key = PasswordHasher::computeHash(password, passwordSalt).toUtf8();
             } else if (!password.isEmpty()) {
+                // A hostile server must not be able to make us run or allocate for
+                // unreasonable scrypt parameters.
                 const int n = resp.has_n() ? resp.n() : SCRYPT_N;
                 const int r = resp.has_r() ? resp.r() : SCRYPT_R;
                 const int p = resp.has_p() ? resp.p() : SCRYPT_P;
+                if (!PasswordHasher::costParamsAreSane(n, r, p)) {
+                    emit loginError(Response::RespClientUpdateRequired,
+                                    QStringLiteral("The server requested unreasonable scrypt cost parameters."), 0, {});
+                    return;
+                }
                 key = PasswordHasher::deriveKey(password, QByteArray::fromBase64(passwordSalt.toUtf8()), n, r, p);
+                if (key.isEmpty()) {
+                    emit loginError(Response::RespClientUpdateRequired, QStringLiteral("Unable to derive verifier."), 0,
+                                    {});
+                    return;
+                }
                 derivedVerifier = QString("$scrypt$%1$%2$%3$%4$%5")
                                       .arg(n)
                                       .arg(r)
@@ -373,7 +388,7 @@ void RemoteClient::loginResponse(const Response &response)
             // The account still used the legacy password format; upgrade it to scrypt.
             doSubmitPasswordVerifier();
         } else if (!derivedVerifier.isEmpty()) {
-            emit sigPasswordVerifierReady(lastHostname, userName, derivedVerifier);
+            emit sigPasswordVerifierReady(derivedVerifier);
             derivedVerifier.clear();
         }
 
@@ -797,8 +812,11 @@ void RemoteClient::submitForgotPasswordResetResponse(const Response &response)
 {
     if (response.response_code() == Response::RespOk) {
         emit sigForgotPasswordSuccess();
-        if (!hashedPassword.isEmpty()) {
-            emit sigPasswordVerifierReady(lastHostname, userName, hashedPassword);
+        // Persist only a real scrypt verifier; a legacy hash must not be stored under
+        // the password key, where it would break future challenge-response logins.
+        if (!derivedVerifier.isEmpty()) {
+            emit sigPasswordVerifierReady(derivedVerifier);
+            derivedVerifier.clear();
         }
     } else {
         emit sigForgotPasswordError();

libcockatrice_network/libcockatrice/network/client/remote/remote_client.h

@@ -55,8 +55,10 @@ signals:
                                                   const QString &_userName,
                                                   const QString &_email);
     //! \brief Emitted once a scrypt verifier for the given account is known and
-    //! can be persisted instead of the plaintext password.
-    void sigPasswordVerifierReady(const QString &hostname, const QString &userName, const QString &verifier);
+    //! can be persisted instead of the plaintext password. The receiving side
+    //! should only store it for the connection it is currently negotiating, so
+    //! the hostname and user name are intentionally not part of the signal.
+    void sigPasswordVerifierReady(const QString &verifier);
 private slots:
     void slotConnected();
     void readData();

libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto

@@ -232,6 +232,6 @@ message Command_SubmitPasswordVerifier {
     extend SessionCommand {
         optional Command_SubmitPasswordVerifier ext = 1026;
     }
-    // Full verifier string to store, e.g. "$pbkdf2-sha512$210000$<salt>$<verifier>"
+    // Full verifier string to store, e.g. "$scrypt$32768$8$1$<salt>$<verifier>"
     required string password_verifier = 1;
 }

libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp

@@ -1,6 +1,5 @@
 #include "rng_sfmt.h"
 
-#include <QDateTime>
 #include <algorithm>
 #include <climits>
 #include <stdexcept>
@@ -11,12 +10,6 @@
 #define UINT64_MAX (~(uint64_t)0)
 #endif
 
-RNG_SFMT::RNG_SFMT(QObject *parent) : RNG_Abstract(parent)
-{
-    // initialize the random number generator with a 32bit integer seed (timestamp)
-    sfmt_init_gen_rand(&sfmt, QDateTime::currentDateTime().toSecsSinceEpoch());
-}
-
 RNG_SFMT::RNG_SFMT(uint64_t seed, QObject *parent) : RNG_Abstract(parent)
 {
     // initialize the random number generator with a 64bit seed, e.g. from a CSPRNG

libcockatrice_rng/libcockatrice/rng/rng_sfmt.h

@@ -36,7 +36,6 @@ private:
     unsigned int cdf(unsigned int min, unsigned int max);
 
 public:
-    explicit RNG_SFMT(QObject *parent = nullptr);
     explicit RNG_SFMT(uint64_t seed, QObject *parent = nullptr);
     unsigned int rand(int min, int max) override;
 };

libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp

@@ -66,11 +66,18 @@ QByteArray PasswordHasher::deriveKey(const QString &password, const QByteArray &
     if (EVP_PBE_scrypt(passwordUtf8.constData(), passwordUtf8.size(),
                        reinterpret_cast<const unsigned char *>(salt.constData()), salt.size(), n, r, p, maxmem,
                        reinterpret_cast<unsigned char *>(key.data()), key.size()) != 1) {
-        qFatal("PasswordHasher::deriveKey: EVP_PBE_scrypt failed");
+        return QByteArray();
     }
     return key;
 }
 
+bool PasswordHasher::costParamsAreSane(int n, int r, int p)
+{
+    // Bounds adopted during review: n in [1024, 2**20] and a power of two, r in [1, 32],
+    // p in [1, 16]. Anything else is rejected before we allocate or derive for it.
+    return n >= 1024 && n <= (1 << 20) && (n & (n - 1)) == 0 && r >= 1 && r <= 32 && p >= 1 && p <= 16;
+}
+
 QString PasswordHasher::generatePasswordVerifier(const QString &password)
 {
     const QByteArray salt = CryptoUtil::randomBytes(SCRYPT_SALT_LENGTH);
@@ -93,15 +100,15 @@ PasswordVerifier PasswordHasher::parsePasswordVerifier(const QString &stored)
 
     bool ok = false;
     const int n = parts.at(2).toInt(&ok);
-    if (!ok || n <= 0) {
+    if (!ok) {
         return result;
     }
     const int r = parts.at(3).toInt(&ok);
-    if (!ok || r <= 0) {
+    if (!ok) {
         return result;
     }
     const int p = parts.at(4).toInt(&ok);
-    if (!ok || p <= 0) {
+    if (!ok || !costParamsAreSane(n, r, p)) {
         return result;
     }
 
@@ -126,6 +133,20 @@ bool PasswordHasher::isLegacyFormat(const QString &stored)
     return !stored.startsWith("$");
 }
 
+bool PasswordHasher::verifyPassword(const QString &password, const QString &storedPasswordData)
+{
+    if (isLegacyFormat(storedPasswordData)) {
+        return storedPasswordData == computeHash(password, storedPasswordData.left(16));
+    }
+
+    const PasswordVerifier verifier = parsePasswordVerifier(storedPasswordData);
+    if (!verifier.isValid) {
+        return false;
+    }
+    const QByteArray derived = deriveKey(password, verifier.salt, verifier.n, verifier.r, verifier.p);
+    return !derived.isEmpty() && constantTimeEquals(derived, verifier.verifier);
+}
+
 QByteArray PasswordHasher::computeResponse(const QByteArray &key, const QByteArray &nonce)
 {
     QByteArray response(EVP_MAX_MD_SIZE, '\0');

libcockatrice_utility/libcockatrice/utility/passwordhasher.h

@@ -36,14 +36,18 @@ public:
     static QString generateRandomSalt(const int len = 16);
     static QString generateActivationToken();
 
-    /** @brief Derive the scrypt verifier for the given password, salt and cost parameters. */
+    /** @brief Derive the scrypt verifier for the given password, salt and cost parameters. Empty on failure. */
     static QByteArray deriveKey(const QString &password, const QByteArray &salt, int n, int r, int p);
+    /** @brief True if the scrypt cost parameters are acceptable for server and client use. */
+    static bool costParamsAreSane(int n, int r, int p);
     /** @brief Build a "$scrypt$<n>$<r>$<p>$<salt>$<verifier>" string with a fresh random salt. */
     static QString generatePasswordVerifier(const QString &password);
     /** @brief Parse a stored "$scrypt$..." string into its components. */
     static PasswordVerifier parsePasswordVerifier(const QString &stored);
     /** @brief True if the stored value is not in the scrypt format (legacy salt+hash). */
     static bool isLegacyFormat(const QString &stored);
+    /** @brief True if the password matches the stored credential, whether legacy salt+hash or scrypt. */
+    static bool verifyPassword(const QString &password, const QString &storedPasswordData);
     /** @brief HMAC-SHA256 of nonce keyed with the password verifier, used for challenge-response logins. */
     static QByteArray computeResponse(const QByteArray &key, const QByteArray &nonce);
     /** @brief Constant-time byte comparison. */

servatrice/migrations/servatrice_0036_to_0037.sql

@@ -1,5 +1,8 @@
 -- Servatrice db migration from version 36 to version 37
 
-ALTER TABLE `cockatrice_users` MODIFY `password_sha512` char(255) NOT NULL, ALGORITHM=INSTANT;
+-- 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

@@ -103,16 +103,6 @@ password=123456
 ; Accept only registered users? default is false (accept unregistered users)
 regonly=false
 
-[security]
-
-; How strictly new authentication features are enforced.  Possible values:
-; * legacy: only accept the legacy 1000-round SHA-512 password hashes;
-; * 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 PBKDF2
-;           automatically on their next successful login.
-authentication_strictness=mixed
-
 [users]
 
 ; The minimum length a username can be
@@ -360,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

@@ -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(255) 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_database_interface.cpp

@@ -374,6 +374,12 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot
                     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;
@@ -631,7 +637,8 @@ bool Servatrice_DatabaseInterface::submitPasswordVerifier(const QString &user, c
         qCWarning(DatabaseInterfaceLog) << "Failed to submit password verifier for user" << user << query->lastError();
         return false;
     }
-    return true;
+    // 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)
@@ -1215,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/serversocketinterface.cpp

@@ -140,11 +140,13 @@ bool AbstractServerSocketInterface::initSession()
     identEvent.set_server_version(VERSION_STRING);
     identEvent.set_protocol_version(protocolVersion);
     if (servatrice->getAuthenticationMethod() == Servatrice::AuthenticationSql) {
-        Event_ServerIdentification::ServerOptions serverOptions = Event_ServerIdentification::SupportsPasswordHash;
-        if (servatrice->getAuthenticationStrictness() != Servatrice::AuthenticationLegacy) {
-            serverOptions = static_cast<Event_ServerIdentification::ServerOptions>(
-                serverOptions | Event_ServerIdentification::SupportsChallengeResponseAuth);
-        }
+        // 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);
@@ -265,7 +267,6 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedSessionComm
             return cmdReportDetails(cmd.GetExtension(Command_ReportDetails::ext), rc);
         case SessionCommand::SUBMIT_PASSWORD_VERIFIER:
             return cmdSubmitPasswordVerifier(cmd.GetExtension(Command_SubmitPasswordVerifier::ext), rc);
-            break;
         default:
             return Response::RespFunctionNotAllowed;
     }
@@ -2470,9 +2471,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRegisterAccount(const C
         password = QString::fromStdString(cmd.hashed_password());
     }
 
-    // In strict mode only scrypt verifiers are accepted for new accounts.
-    if (servatrice->requiresChallengeResponseAuth() &&
-        (passwordNeedsHash || PasswordHasher::isLegacyFormat(password))) {
+    // Reject credential formats the configured authentication strictness does not accept.
+    if (!acceptsCredentialFormat(passwordNeedsHash, password)) {
         return Response::RespClientUpdateRequired;
     }
 
@@ -2522,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*/)
 {
@@ -2856,9 +2871,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdAccountPassword(const C
         newPassword = QString::fromStdString(cmd.hashed_new_password());
     }
 
-    // In strict mode only scrypt verifiers are accepted.
-    if (servatrice->requiresChallengeResponseAuth() &&
-        (newPasswordNeedsHash || PasswordHasher::isLegacyFormat(newPassword))) {
+    // Reject new credential formats the configured authentication strictness does not accept.
+    if (!acceptsCredentialFormat(newPasswordNeedsHash, newPassword)) {
         return Response::RespClientUpdateRequired;
     }
 
@@ -2999,9 +3013,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdForgotPasswordReset(con
         password = QString::fromStdString(cmd.hashed_new_password());
     }
 
-    // In strict mode only scrypt verifiers are accepted.
-    if (servatrice->requiresChallengeResponseAuth() &&
-        (passwordNeedsHash || PasswordHasher::isLegacyFormat(password))) {
+    // Reject new credential formats the configured authentication strictness does not accept.
+    if (!acceptsCredentialFormat(passwordNeedsHash, password)) {
         return Response::RespClientUpdateRequired;
     }
 
@@ -3076,10 +3089,17 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con
     }
 
     auto *re = new Response_PasswordSalt;
-    const bool challengeResponseEnabled = servatrice->getAuthenticationStrictness() != Servatrice::AuthenticationLegacy;
     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) {
@@ -3091,9 +3111,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con
         re->set_r(verifier.r);
         re->set_p(verifier.p);
         re->set_needs_migration(false);
-    }
-
-    if (challengeResponseEnabled) {
+        // 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());
@@ -3205,12 +3223,22 @@ AbstractServerSocketInterface::cmdSubmitPasswordVerifier(const Command_SubmitPas
         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_NAME_LENGTH ||
+    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;
     }

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;

tests/password_hash_test.cpp

@@ -104,6 +104,69 @@ TEST(PasswordHashTest, ConstantTimeEquals)
     ASSERT_FALSE(PasswordHasher::constantTimeEquals(QByteArray("short"), QByteArray("longer")));
 }
 
+TEST(PasswordHashTest, CostParamsAreSane)
+{
+    // Accept the recommended interactive parameters and the RFC 7914 test vector's.
+    ASSERT_TRUE(PasswordHasher::costParamsAreSane(SCRYPT_N, SCRYPT_R, SCRYPT_P));
+    ASSERT_TRUE(PasswordHasher::costParamsAreSane(1024, 8, 16));
+
+    // n must be in [1024, 2**20] and a power of two.
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(512, 8, 1));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(1 << 21, 8, 1));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(1025, 8, 1));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(0, 8, 1));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(-1024, 8, 1));
+
+    // r in [1, 32], p in [1, 16].
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(1024, 0, 1));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(1024, 33, 1));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(1024, 8, 0));
+    ASSERT_FALSE(PasswordHasher::costParamsAreSane(1024, 8, 17));
+}
+
+TEST(PasswordHashTest, ParsePasswordVerifierRejectsHostileCostParams)
+{
+    // 16 bytes of salt and 64 bytes of verifier, base64 encoded.
+    const QString saltB64 = QLatin1String("c2FsdHNhbHRzYWx0c2FsdA==");
+    const QString verifierB64 = QString(QByteArray(SCRYPT_VERIFIER_LENGTH, '\x42').toBase64());
+
+    ASSERT_TRUE(
+        PasswordHasher::parsePasswordVerifier(QString("$scrypt$1024$8$1$%1$%2").arg(saltB64).arg(verifierB64)).isValid);
+
+    // n not a power of two, below 1024, or above 2**20.
+    ASSERT_FALSE(
+        PasswordHasher::parsePasswordVerifier(QString("$scrypt$1025$8$1$%1$%2").arg(saltB64).arg(verifierB64)).isValid);
+    ASSERT_FALSE(
+        PasswordHasher::parsePasswordVerifier(QString("$scrypt$512$8$1$%1$%2").arg(saltB64).arg(verifierB64)).isValid);
+    ASSERT_FALSE(
+        PasswordHasher::parsePasswordVerifier(QString("$scrypt$1073741824$8$1$%1$%2").arg(saltB64).arg(verifierB64))
+            .isValid);
+
+    // r and p out of range.
+    ASSERT_FALSE(PasswordHasher::parsePasswordVerifier(QString("$scrypt$1024$33$1$%1$%2").arg(saltB64).arg(verifierB64))
+                     .isValid);
+    ASSERT_FALSE(PasswordHasher::parsePasswordVerifier(QString("$scrypt$1024$8$17$%1$%2").arg(saltB64).arg(verifierB64))
+                     .isValid);
+}
+
+TEST(PasswordHashTest, VerifyPasswordLegacyRow)
+{
+    const QString salt = PasswordHasher::generateRandomSalt();
+    const QString legacyStored = PasswordHasher::computeHash("correct horse", salt);
+    ASSERT_TRUE(PasswordHasher::verifyPassword("correct horse", legacyStored));
+    ASSERT_FALSE(PasswordHasher::verifyPassword("battery staple", legacyStored));
+}
+
+TEST(PasswordHashTest, VerifyPasswordScryptRow)
+{
+    const QString scryptStored = PasswordHasher::generatePasswordVerifier("correct horse");
+    // Regression for the changeUserPassword bug that re-hashed the old password with
+    // a 16-char salt torn out of the "$scrypt$..." string, which could never match.
+    ASSERT_TRUE(PasswordHasher::verifyPassword("correct horse", scryptStored));
+    ASSERT_FALSE(PasswordHasher::verifyPassword("battery staple", scryptStored));
+    ASSERT_FALSE(PasswordHasher::verifyPassword("correct horse", "garbage"));
+}
+
 } // namespace
 
 int main(int argc, char **argv)