diff --git a/.ci/Arch/Dockerfile b/.ci/Arch/Dockerfile index f37315262..b08e568f3 100644 --- a/.ci/Arch/Dockerfile +++ b/.ci/Arch/Dockerfile @@ -8,6 +8,7 @@ RUN pacman --sync --refresh --sysupgrade --needed --noconfirm \ gtest \ mariadb-libs \ ninja \ + openssl \ protobuf \ qt6-base \ qt6-declarative \ diff --git a/.ci/Debian12/Dockerfile b/.ci/Debian12/Dockerfile index 0fa227d6f..e3df94ab5 100644 --- a/.ci/Debian12/Dockerfile +++ b/.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 \ diff --git a/.ci/Debian13/Dockerfile b/.ci/Debian13/Dockerfile index 13e8b35c7..60e490c98 100644 --- a/.ci/Debian13/Dockerfile +++ b/.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 \ diff --git a/.ci/Fedora43/Dockerfile b/.ci/Fedora43/Dockerfile index 68e894543..4005bbf67 100644 --- a/.ci/Fedora43/Dockerfile +++ b/.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 \ diff --git a/.ci/Fedora44/Dockerfile b/.ci/Fedora44/Dockerfile index ffd7c1b9b..e0224cdc6 100644 --- a/.ci/Fedora44/Dockerfile +++ b/.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 \ diff --git a/.ci/Servatrice_Debian12/Dockerfile b/.ci/Servatrice_Debian12/Dockerfile index 21f6a036e..321aa7c0f 100644 --- a/.ci/Servatrice_Debian12/Dockerfile +++ b/.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 \ diff --git a/.ci/Ubuntu24.04/Dockerfile b/.ci/Ubuntu24.04/Dockerfile index 12320c276..10adc5e64 100644 --- a/.ci/Ubuntu24.04/Dockerfile +++ b/.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 \ diff --git a/.ci/Ubuntu26.04/Dockerfile b/.ci/Ubuntu26.04/Dockerfile index ce3d9cd6c..1b6cf825f 100644 --- a/.ci/Ubuntu26.04/Dockerfile +++ b/.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 \ diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index e895e2220..75fbc59f1 100644 --- a/.github/workflows/codeql.yml +++ b/.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 diff --git a/.github/workflows/docker-release.yml b/.github/workflows/docker-release.yml index df4fe233c..255e8b045 100644 --- a/.github/workflows/docker-release.yml +++ b/.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-* diff --git a/CMakeLists.txt b/CMakeLists.txt index bac46c2bc..4006ead2f 100644 --- a/CMakeLists.txt +++ b/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() diff --git a/Dockerfile b/Dockerfile index 382309d47..7d3deb5fb 100644 --- a/Dockerfile +++ b/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/* diff --git a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp b/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp index c368e46c0..bc9fa0679 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp +++ b/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); } diff --git a/cockatrice/src/client/network/connection_controller/remote_connection_controller.h b/cockatrice/src/client/network/connection_controller/remote_connection_controller.h index c5dc87f99..5e4784513 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.h +++ b/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(); diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp index 9eb074f53..794cabe21 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp +++ b/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()); } diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp index 198fa259b..a4a31d78d 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp +++ b/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 #include +#include +#include +#include #include +#include #include +#include 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 &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; +} diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.h b/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.h index 6642ad8c6..526582135 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.h +++ b/cockatrice/src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.h @@ -13,6 +13,9 @@ #include #include #include +#include + +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 &convert); + private: QVBoxLayout *layout; QLabel *label; diff --git a/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp b/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp index f6f34a6a5..fb610e814 100644 --- a/cockatrice/src/interface/widgets/server/user/user_info_popup.cpp +++ b/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) { diff --git a/cockatrice/src/interface/widgets/server/user/user_info_popup.h b/cockatrice/src/interface/widgets/server/user/user_info_popup.h index 02cc2b44e..ed7320fba 100644 --- a/cockatrice/src/interface/widgets/server/user/user_info_popup.h +++ b/cockatrice/src/interface/widgets/server/user/user_info_popup.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -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 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 gameInviteAvailable; UserInfoHeaderWidget *header; QWidget *actionArea; ///< rebuilt per user diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp b/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp index 2cacfc4f9..a8c99c979 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.cpp +++ b/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); diff --git a/cockatrice/src/interface/widgets/server/user/user_list_widget.h b/cockatrice/src/interface/widgets/server/user/user_list_widget.h index 7531ef925..412271160 100644 --- a/cockatrice/src/interface/widgets/server/user/user_list_widget.h +++ b/cockatrice/src/interface/widgets/server/user/user_list_widget.h @@ -22,6 +22,7 @@ #include #include #include +#include #include class QTreeWidget; diff --git a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp index f96c139b3..b0dac3e7c 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp @@ -1091,7 +1091,8 @@ QList 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; diff --git a/cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_widget.cpp b/cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_widget.cpp index 04dcdf7f2..876fbf6ad 100644 --- a/cockatrice/src/interface/widgets/visual_deck_storage/deck_preview/deck_preview_widget.cpp +++ b/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 -#include #include #include #include @@ -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; + }); } diff --git a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp index 8204b6842..c0c142891 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp +++ b/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(); diff --git a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h index ca467ae2f..941eaba25 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h +++ b/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(); diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto index c06ac9516..d7d7ca54a 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto +++ b/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$$" + // Full verifier string to store, e.g. "$scrypt$32768$8$1$$" required string password_verifier = 1; } diff --git a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp b/libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp index 17cc77a22..4c578b4e4 100644 --- a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp +++ b/libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp @@ -1,6 +1,5 @@ #include "rng_sfmt.h" -#include #include #include #include @@ -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 diff --git a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h b/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h index b12401799..a180dad99 100644 --- a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h +++ b/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; }; diff --git a/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp b/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp index d4c361f1e..ffde16a04 100644 --- a/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp +++ b/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(salt.constData()), salt.size(), n, r, p, maxmem, reinterpret_cast(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'); diff --git a/libcockatrice_utility/libcockatrice/utility/passwordhasher.h b/libcockatrice_utility/libcockatrice/utility/passwordhasher.h index cb3c9be5d..a00146b6e 100644 --- a/libcockatrice_utility/libcockatrice/utility/passwordhasher.h +++ b/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$$$

$$" 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. */ diff --git a/servatrice/migrations/servatrice_0036_to_0037.sql b/servatrice/migrations/servatrice_0036_to_0037.sql index 6f83cbec4..de3e09987 100644 --- a/servatrice/migrations/servatrice_0036_to_0037.sql +++ b/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$$$

$$" (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; \ No newline at end of file diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index 11aa24494..f7e1feed9 100644 --- a/servatrice/servatrice.ini.example +++ b/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$$$

$$". 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" diff --git a/servatrice/servatrice.sql b/servatrice/servatrice.sql index 43851d016..0bee10cd0 100644 --- a/servatrice/servatrice.sql +++ b/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, diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index ac45382a8..caa3478c8 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/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; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index dc71c8e18..36a3e5766 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/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( - 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::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; } diff --git a/servatrice/src/serversocketinterface.h b/servatrice/src/serversocketinterface.h index 6890d9e9f..b393d2f17 100644 --- a/servatrice/src/serversocketinterface.h +++ b/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; diff --git a/tests/password_hash_test.cpp b/tests/password_hash_test.cpp index c1b5b22a9..727677ee5 100644 --- a/tests/password_hash_test.cpp +++ b/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)