From 8f52223322bf18eb9ac387ff2b4f471251d5d967 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:11:11 +0200 Subject: [PATCH 1/7] Bump actions/download-artifact from 7 to 8 (#7209) Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 7 to 8. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](https://github.com/actions/download-artifact/compare/v7...v8) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-version: '8' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/docker-release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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-* From dade7ae78a79cff8cc8b4db1cc7cd61398ec10fd Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:23:45 +0200 Subject: [PATCH 2/7] Bump actions/checkout from 6 to 7 (#7210) Bumps [actions/checkout](https://github.com/actions/checkout) from 6 to 7. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/codeql.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 6f86c45ea8e858f1ee7a3738acd51b3cf1a1137e Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:12:11 +0200 Subject: [PATCH 3/7] [Refactor] Extract shared deck conversion prompt helper (#7107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the deck-to-.cod conversion prompt logic (format check, saved preference handling, overwrite confirmation, dialog) out of DeckPreviewWidget into dlg_convert_deck_to_cod_format so the deck editor can reuse it without duplicating it. Took 4 minutes Took 4 minutes Took 1 minute # Commit time for manual adjustment: # Took 3 minutes Co-authored-by: Lukas Brübach --- .../dlg_convert_deck_to_cod_format.cpp | 76 +++++++++++++++++++ .../dialogs/dlg_convert_deck_to_cod_format.h | 18 +++++ .../deck_preview/deck_preview_widget.cpp | 58 +------------- 3 files changed, 96 insertions(+), 56 deletions(-) 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/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; + }); } From 68e4fa054de040105fb0a6dd7676c30ad66e9935 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:35:28 +0200 Subject: [PATCH 4/7] [UserList] Add invite button to hover popup (#7144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Client] Send game invites from the user context menu via a private message The user context menu gains an "Invite to Game" submenu listing the inviteable games in the room (the inviter's own games, honoring the buddy-only setting). Picking one opens a private message to the target user with a cockatrice://joingame link naming the game, so the target gets a clickable invite instead of a raw URL. Multi-game rooms offer a picker; a single inviteable game sends directly. Sending a message to an offline user no longer swallows the draft — it reports that the user is offline and keeps the typed text. Took 30 seconds Took 1 minute * [Client] Open the invite dialog taller by default without enforcing a minimum size --------- Co-authored-by: Lukas Brübach --- .../widgets/server/user/user_info_popup.cpp | 7 +++++++ .../widgets/server/user/user_info_popup.h | 14 ++++++++++++++ .../widgets/server/user/user_list_widget.cpp | 7 +++++++ .../widgets/server/user/user_list_widget.h | 1 + .../src/interface/widgets/tabs/tab_supervisor.cpp | 3 ++- 5 files changed, 31 insertions(+), 1 deletion(-) 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; From fd865c1ea96690101e7ecd2a229d7063194e6503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 4 Aug 2026 10:02:35 +0200 Subject: [PATCH 5/7] [Security] Use a CSPRNG for salts, tokens, and RNG seeding Password salts and activation tokens were generated with the global SFMT RNG, which was seeded from a 32-bit timestamp, making registration salts and activation tokens predictable. The game RNG used the same timestamp seed across restarts. Add CryptoUtil backed by OpenSSL RAND_bytes and use it for salt/token generation and to seed RNG_SFMT with a 64-bit CSPRNG value in both the client and server. Link libcockatrice_utility against OpenSSL::Crypto. Took 30 seconds Took 25 minutes --- .ci/Arch/Dockerfile | 1 + .ci/Debian12/Dockerfile | 1 + .ci/Debian13/Dockerfile | 1 + .ci/Fedora43/Dockerfile | 1 + .ci/Fedora44/Dockerfile | 1 + .ci/Servatrice_Debian12/Dockerfile | 1 + .ci/Ubuntu24.04/Dockerfile | 1 + .ci/Ubuntu26.04/Dockerfile | 1 + CMakeLists.txt | 5 --- Dockerfile | 2 + cockatrice/src/main.cpp | 3 +- .../libcockatrice/rng/rng_sfmt.cpp | 8 ++-- .../libcockatrice/rng/rng_sfmt.h | 2 +- libcockatrice_utility/CMakeLists.txt | 9 ++-- .../libcockatrice/utility/cryptoutil.cpp | 25 +++++++++++ .../libcockatrice/utility/cryptoutil.h | 13 ++++++ .../libcockatrice/utility/passwordhasher.cpp | 26 +++++++++--- servatrice/src/main.cpp | 3 +- tests/password_hash_test.cpp | 41 +++++++++++-------- 19 files changed, 108 insertions(+), 37 deletions(-) create mode 100644 libcockatrice_utility/libcockatrice/utility/cryptoutil.cpp create mode 100644 libcockatrice_utility/libcockatrice/utility/cryptoutil.h 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/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/main.cpp b/cockatrice/src/main.cpp index 84d5d175f..d8aa1cd08 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -53,6 +53,7 @@ #include #include #include +#include QTranslator *translator, *qtTranslator; RNG_Abstract *rng; @@ -292,7 +293,7 @@ int main(int argc, char *argv[]) } } - rng = new RNG_SFMT; + rng = new RNG_SFMT(CryptoUtil::randomUInt64()); themeManager = new ThemeManager; soundEngine = new SoundEngine; diff --git a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp b/libcockatrice_rng/libcockatrice/rng/rng_sfmt.cpp index 5b38deb3f..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,10 +10,11 @@ #define UINT64_MAX (~(uint64_t)0) #endif -RNG_SFMT::RNG_SFMT(QObject *parent) : RNG_Abstract(parent) +RNG_SFMT::RNG_SFMT(uint64_t seed, QObject *parent) : RNG_Abstract(parent) { - // initialize the random number generator with a 32bit integer seed (timestamp) - sfmt_init_gen_rand(&sfmt, QDateTime::currentDateTime().toSecsSinceEpoch()); + // initialize the random number generator with a 64bit seed, e.g. from a CSPRNG + uint32_t seedArray[2] = {static_cast(seed), static_cast(seed >> 32)}; + sfmt_init_by_array(&sfmt, seedArray, 2); } /** diff --git a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h b/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h index 7e9f53df3..a180dad99 100644 --- a/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h +++ b/libcockatrice_rng/libcockatrice/rng/rng_sfmt.h @@ -36,7 +36,7 @@ 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/CMakeLists.txt b/libcockatrice_utility/CMakeLists.txt index c6411ea76..065961ea3 100644 --- a/libcockatrice_utility/CMakeLists.txt +++ b/libcockatrice_utility/CMakeLists.txt @@ -6,13 +6,14 @@ set(CMAKE_AUTOUIC ON) set(CMAKE_AUTORCC ON) set(UTILITY_SOURCES - libcockatrice/utility/expression.cpp libcockatrice/utility/levenshtein.cpp libcockatrice/utility/passwordhasher.cpp - libcockatrice/utility/server_rate_limiter.cpp libcockatrice/utility/warning_categories.cpp + libcockatrice/utility/cryptoutil.cpp libcockatrice/utility/expression.cpp libcockatrice/utility/levenshtein.cpp + libcockatrice/utility/passwordhasher.cpp libcockatrice/utility/server_rate_limiter.cpp libcockatrice/utility/warning_categories.cpp ) set(UTILITY_HEADERS libcockatrice/utility/card_ref.h libcockatrice/utility/color.h + libcockatrice/utility/cryptoutil.h libcockatrice/utility/expression.h libcockatrice/utility/levenshtein.h libcockatrice/utility/macros.h @@ -32,7 +33,9 @@ add_library(libcockatrice_utility STATIC ${UTILITY_SOURCES} ${UTILITY_HEADERS}) target_include_directories(libcockatrice_utility PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}) -target_link_libraries(libcockatrice_utility PUBLIC libcockatrice_rng ${QT_CORE_MODULE}) +find_package(OpenSSL REQUIRED) + +target_link_libraries(libcockatrice_utility PUBLIC libcockatrice_rng OpenSSL::Crypto ${QT_CORE_MODULE}) set(ORACLE_LIBS) diff --git a/libcockatrice_utility/libcockatrice/utility/cryptoutil.cpp b/libcockatrice_utility/libcockatrice/utility/cryptoutil.cpp new file mode 100644 index 000000000..416ef261b --- /dev/null +++ b/libcockatrice_utility/libcockatrice/utility/cryptoutil.cpp @@ -0,0 +1,25 @@ +#include "cryptoutil.h" + +#include + +namespace CryptoUtil +{ +QByteArray randomBytes(int count) +{ + QByteArray bytes(count, '\0'); + if (RAND_bytes(reinterpret_cast(bytes.data()), count) != 1) { + // Randomness failure is fatal: never fall back to a predictable source. + qFatal("CryptoUtil::randomBytes: RAND_bytes failed"); + } + return bytes; +} + +quint64 randomUInt64() +{ + quint64 value; + if (RAND_bytes(reinterpret_cast(&value), sizeof(value)) != 1) { + qFatal("CryptoUtil::randomUInt64: RAND_bytes failed"); + } + return value; +} +} // namespace CryptoUtil diff --git a/libcockatrice_utility/libcockatrice/utility/cryptoutil.h b/libcockatrice_utility/libcockatrice/utility/cryptoutil.h new file mode 100644 index 000000000..dba9dc37d --- /dev/null +++ b/libcockatrice_utility/libcockatrice/utility/cryptoutil.h @@ -0,0 +1,13 @@ +#ifndef CRYPTOUTIL_H +#define CRYPTOUTIL_H + +#include +#include + +namespace CryptoUtil +{ +QByteArray randomBytes(int count); +quint64 randomUInt64(); +} // namespace CryptoUtil + +#endif diff --git a/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp b/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp index c40c5f94f..1c22fdcfa 100644 --- a/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp +++ b/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp @@ -1,7 +1,7 @@ #include "passwordhasher.h" #include -#include +#include QString PasswordHasher::computeHash(const QString &password, const QString &salt) { @@ -21,12 +21,28 @@ QString PasswordHasher::generateRandomSalt(const int len) static const char alphanum[] = "0123456789" "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz"; + const int size = sizeof(alphanum) - 1; + + // Two bytes per character, corrected for modulo bias via rejection sampling. + const int bucketSize = 65536 / size; + const int limit = bucketSize * size; QString ret; - int size = sizeof(alphanum) - 1; - + ret.reserve(len); + QByteArray random = CryptoUtil::randomBytes(len * 2); + int bytesUsed = 0; for (int i = 0; i < len; ++i) { - ret.append(alphanum[rng->rand(0, size)]); + unsigned int value; + do { + if (bytesUsed >= random.size()) { + random = CryptoUtil::randomBytes(len * 2); + bytesUsed = 0; + } + value = static_cast(static_cast(random.at(bytesUsed))) << 8 | + static_cast(static_cast(random.at(bytesUsed + 1))); + bytesUsed += 2; + } while (value >= limit); + ret.append(alphanum[value / bucketSize]); } return ret; @@ -34,5 +50,5 @@ QString PasswordHasher::generateRandomSalt(const int len) QString PasswordHasher::generateActivationToken() { - return QCryptographicHash::hash(generateRandomSalt().toUtf8(), QCryptographicHash::Md5).toBase64().left(16); + return QString(CryptoUtil::randomBytes(16).toBase64().left(16)); } diff --git a/servatrice/src/main.cpp b/servatrice/src/main.cpp index 9e7fe38d9..13bf95a82 100644 --- a/servatrice/src/main.cpp +++ b/servatrice/src/main.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include RNG_Abstract *rng; @@ -169,7 +170,7 @@ int main(int argc, char *argv[]) signalhandler = new SignalHandler(); - rng = new RNG_SFMT; + rng = new RNG_SFMT(CryptoUtil::randomUInt64()); std::cerr << "Servatrice " << VERSION_STRING << " starting." << std::endl; std::cerr << "-------------------------" << std::endl; diff --git a/tests/password_hash_test.cpp b/tests/password_hash_test.cpp index 38d9b6315..2b8f8bdb7 100644 --- a/tests/password_hash_test.cpp +++ b/tests/password_hash_test.cpp @@ -1,25 +1,9 @@ #include "gtest/gtest.h" -#include -#include +#include #include -RNG_Abstract *rng; - namespace { -class PasswordHashTest : public ::testing::Test -{ -protected: - void SetUp() override - { - rng = new RNG_SFMT; - } - - void TearDown() override - { - delete rng; - } -}; TEST(PasswordHashTest, RegressionTest) { @@ -29,6 +13,29 @@ TEST(PasswordHashTest, RegressionTest) QString hash = PasswordHasher::computeHash(password, salt); ASSERT_EQ(hash, salt + expected) << "The computed hash value remains the same"; } + +TEST(PasswordHashTest, SaltUsesAlphanumericCharset) +{ + static const char alphanum[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + const QString salt = PasswordHasher::generateRandomSalt(); + ASSERT_EQ(salt.size(), 16); + for (const QChar &c : salt) { + ASSERT_NE(strchr(alphanum, c.toLatin1()), nullptr); + } +} + +TEST(PasswordHashTest, SaltsAreUnique) +{ + const QString salt1 = PasswordHasher::generateRandomSalt(); + const QString salt2 = PasswordHasher::generateRandomSalt(); + ASSERT_NE(salt1, salt2); +} + +TEST(PasswordHashTest, TokenHasExpectedLength) +{ + const QString token = PasswordHasher::generateActivationToken(); + ASSERT_EQ(token.size(), 16); +} } // namespace int main(int argc, char **argv) From 5e94d0d3da8d77b4d0765c32a7bb9390a33303a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 10 Aug 2026 22:27:49 +0200 Subject: [PATCH 6/7] Lint. Took 4 minutes Took 36 seconds --- libcockatrice_utility/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libcockatrice_utility/CMakeLists.txt b/libcockatrice_utility/CMakeLists.txt index 065961ea3..db23f7951 100644 --- a/libcockatrice_utility/CMakeLists.txt +++ b/libcockatrice_utility/CMakeLists.txt @@ -7,7 +7,8 @@ set(CMAKE_AUTORCC ON) set(UTILITY_SOURCES libcockatrice/utility/cryptoutil.cpp libcockatrice/utility/expression.cpp libcockatrice/utility/levenshtein.cpp - libcockatrice/utility/passwordhasher.cpp libcockatrice/utility/server_rate_limiter.cpp libcockatrice/utility/warning_categories.cpp + libcockatrice/utility/passwordhasher.cpp libcockatrice/utility/server_rate_limiter.cpp + libcockatrice/utility/warning_categories.cpp ) set(UTILITY_HEADERS From 62dadf7a5683ac3d837acf95a69c54d3268f5263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Tue, 4 Aug 2026 11:36:10 +0200 Subject: [PATCH 7/7] [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$$$

$$" 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 --- .../remote_connection_controller.cpp | 21 ++- .../remote_connection_controller.h | 16 ++- .../interface/widgets/dialogs/dlg_connect.cpp | 18 ++- .../interface/widgets/dialogs/dlg_connect.h | 15 ++ .../widgets/dialogs/dlg_edit_password.cpp | 5 +- .../widgets/server/user/user_info_box.cpp | 4 +- cockatrice/src/interface/window_main.cpp | 5 +- .../client/abstract/abstract_client.cpp | 3 +- .../network/client/abstract/abstract_client.h | 5 + .../network/client/remote/remote_client.cpp | 114 ++++++++++++++- .../network/client/remote/remote_client.h | 21 +++ .../network/server/remote/server.h | 5 + .../server/remote/server_database_interface.h | 8 ++ .../server/remote/server_protocolhandler.cpp | 26 ++++ .../server/remote/server_protocolhandler.h | 11 ++ .../libcockatrice/protocol/featureset.cpp | 3 +- .../pb/event_server_identification.proto | 1 + .../protocol/pb/response_password_salt.proto | 10 ++ .../protocol/pb/session_commands.proto | 12 ++ .../settings/servers_settings.cpp | 8 ++ .../libcockatrice/settings/servers_settings.h | 2 + .../libcockatrice/utility/passwordhasher.cpp | 114 +++++++++++++++ .../libcockatrice/utility/passwordhasher.h | 43 ++++++ .../migrations/servatrice_0036_to_0037.sql | 8 ++ servatrice/servatrice.ini.example | 16 +++ servatrice/servatrice.sql | 4 +- servatrice/src/servatrice.cpp | 12 ++ servatrice/src/servatrice.h | 11 ++ .../src/servatrice_database_interface.cpp | 97 ++++++++++++- .../src/servatrice_database_interface.h | 4 +- servatrice/src/serversocketinterface.cpp | 109 ++++++++++++++- servatrice/src/serversocketinterface.h | 2 + tests/password_hash_test.cpp | 131 ++++++++++++++++++ 33 files changed, 834 insertions(+), 30 deletions(-) create mode 100644 servatrice/migrations/servatrice_0036_to_0037.sql 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 890a621c8..bc9fa0679 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp +++ b/cockatrice/src/client/network/connection_controller/remote_connection_controller.cpp @@ -83,6 +83,9 @@ void ConnectionController::wireClientSignals() connect(remoteClient, &RemoteClient::sigPromptForForgotPasswordChallenge, this, &ConnectionController::onPromptForgotPasswordChallenge); + + connect(remoteClient, &RemoteClient::sigPasswordVerifierReady, this, + &ConnectionController::onPasswordVerifierReady); } void ConnectionController::connectToServer() @@ -91,16 +94,32 @@ void ConnectionController::connectToServer() connect(dlgConnect, &DlgConnect::sigStartForgotPasswordRequest, this, &ConnectionController::forgotPasswordRequest); if (dlgConnect->exec()) { + pendingSaveName = dlgConnect->getSaveName(); + pendingSavePassword = dlgConnect->getSavePassword(); + remoteClient->setStoredVerifier(dlgConnect->getStoredVerifier()); remoteClient->connectToServer(dlgConnect->getHost(), static_cast(dlgConnect->getPort()), dlgConnect->getPlayerName(), dlgConnect->getPassword()); } } +void ConnectionController::onPasswordVerifierReady(const QString &verifier) +{ + if (pendingSavePassword) { + SettingsCache::instance().servers().setServerPassword(pendingSaveName, verifier); + } +} + void ConnectionController::connectToServerDirect(const QString &host, unsigned int port, const QString &playerName, - const QString &password) + const QString &password, + const QString &storedVerifier, + const QString &saveName, + bool savePassword) { + pendingSaveName = saveName; + pendingSavePassword = savePassword; + remoteClient->setStoredVerifier(storedVerifier); remoteClient->connectToServer(host, port, playerName, password); } 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 bae99a3e0..5e4784513 100644 --- a/cockatrice/src/client/network/connection_controller/remote_connection_controller.h +++ b/cockatrice/src/client/network/connection_controller/remote_connection_controller.h @@ -35,8 +35,13 @@ public: void registerToServer(); void forgotPasswordRequest(); void connectToServer(); - void - connectToServerDirect(const QString &host, unsigned int port, const QString &playerName, const QString &password); + void connectToServerDirect(const QString &host, + unsigned int port, + const QString &playerName, + const QString &password, + const QString &storedVerifier = QString(), + const QString &saveName = QString(), + bool savePassword = false); void disconnectFromServer(); void refreshWindowTitle() @@ -81,6 +86,9 @@ private slots: void onPromptForgotPasswordReset(); void onPromptForgotPasswordChallenge(); + // Persists the derived scrypt verifier after a successful challenge-response login + void onPasswordVerifierReady(const QString &verifier); + private: void wireClientSignals(); void updateWindowTitle(); @@ -97,6 +105,10 @@ private: // Kept as a member so the forgot-password signal can be wired to it DlgConnect *dlgConnect{nullptr}; + + // Captured from the connect dialog when a connection is initiated + QString pendingSaveName; + bool pendingSavePassword{false}; }; #endif // COCKATRICE_REMOTE_CONNECTION_CONTROLLER_H diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp index aa8a916f8..794cabe21 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_connect.cpp @@ -273,9 +273,15 @@ void DlgConnect::updateDisplayInfo(const QString &saveName) playernameEdit->setText(_data.at(3)); playernameEdit->setFocus(); savePasswordCheckBox->setChecked(savePasswordStatus); + storedVerifier.clear(); if (savePasswordStatus) { - passwordEdit->setText(_data.at(4)); + const QString stored = _data.at(4); + if (stored.startsWith("$")) { + storedVerifier = stored; + } else { + passwordEdit->setText(stored); + } } if (!_data.at(6).isEmpty()) { @@ -301,6 +307,7 @@ void DlgConnect::newHostSelected(bool state) portEdit->setDisabled(false); playernameEdit->clear(); passwordEdit->clear(); + storedVerifier.clear(); saveEdit->clear(); saveEdit->setPlaceholderText(tr("Unique Server Name")); saveEdit->setDisabled(false); @@ -326,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.")); @@ -333,10 +345,10 @@ void DlgConnect::actOk() } servers.addNewServer(saveEdit->text().trimmed(), hostEdit->text().trimmed(), portEdit->text().trimmed(), - playernameEdit->text().trimmed(), 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(), + playernameEdit->text().trimmed(), passwordToSave, savePasswordCheckBox->isChecked()); } diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_connect.h b/cockatrice/src/interface/widgets/dialogs/dlg_connect.h index 083dad0ad..456c5af93 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_connect.h +++ b/cockatrice/src/interface/widgets/dialogs/dlg_connect.h @@ -10,6 +10,7 @@ #include "../interface/widgets/server/handle_public_servers.h" #include "../interface/widgets/server/user/user_info_connection.h" +#include #include #include #include @@ -47,6 +48,19 @@ public: { return passwordEdit->text(); } + //! \brief Stored "$scrypt$..." verifier for challenge-response servers (never the plaintext password). + [[nodiscard]] QString getStoredVerifier() const + { + return storedVerifier; + } + [[nodiscard]] QString getSaveName() const + { + return saveEdit->text(); + } + [[nodiscard]] bool getSavePassword() const + { + return savePasswordCheckBox->isChecked(); + } public slots: void downloadThePublicServers(); @@ -77,6 +91,7 @@ private: QPushButton *btnConnect, *btnForgotPassword, *btnRefreshServers, *btnDeleteServer; QMap> savedHostList; HandlePublicServers *hps; + QString storedVerifier; const QString placeHolderText = tr("Downloading..."); }; #endif diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_edit_password.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_edit_password.cpp index 4310c03fc..9bb6c4dd1 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_edit_password.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_edit_password.cpp @@ -18,7 +18,10 @@ DlgEditPassword::DlgEditPassword(QWidget *parent) : QDialog(parent) auto &servers = SettingsCache::instance().servers(); if (servers.getSavePassword()) { - oldPasswordEdit->setText(servers.getPassword()); + const QString stored = servers.getPassword(); + if (!stored.startsWith("$")) { + oldPasswordEdit->setText(stored); + } } oldPasswordLabel->setBuddy(oldPasswordEdit); diff --git a/cockatrice/src/interface/widgets/server/user/user_info_box.cpp b/cockatrice/src/interface/widgets/server/user/user_info_box.cpp index 875bdfb05..3b32cc073 100644 --- a/cockatrice/src/interface/widgets/server/user/user_info_box.cpp +++ b/cockatrice/src/interface/widgets/server/user/user_info_box.cpp @@ -283,7 +283,9 @@ void UserInfoBox::changePassword(const QString &oldPassword, const QString &newP { Command_AccountPassword cmd; cmd.set_old_password(oldPassword.toStdString()); - if (client->getServerSupportsPasswordHash()) { + if (client->getServerSupportsChallengeResponse()) { + cmd.set_hashed_new_password(PasswordHasher::generatePasswordVerifier(newPassword).toStdString()); + } else if (client->getServerSupportsPasswordHash()) { auto passwordSalt = PasswordHasher::generateRandomSalt(); QString hashedPassword = PasswordHasher::computeHash(newPassword, passwordSalt); cmd.set_hashed_new_password(hashedPassword.toStdString()); diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index 4567991c8..7e7b0f87f 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -871,8 +871,9 @@ void MainWindow::changeEvent(QEvent *event) !startupDestinationConnectsToServer()) { qCInfo(WindowMainStartupAutoconnectLog) << "Attempting auto-connect..."; DlgConnect dlg(this); - connectionController->connectToServerDirect(dlg.getHost(), static_cast(dlg.getPort()), - dlg.getPlayerName(), dlg.getPassword()); + connectionController->connectToServerDirect( + dlg.getHost(), static_cast(dlg.getPort()), dlg.getPlayerName(), dlg.getPassword(), + dlg.getStoredVerifier(), dlg.getSaveName(), dlg.getSavePassword()); } } } diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp index d6316deb3..46a470487 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp +++ b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.cpp @@ -22,7 +22,8 @@ #include AbstractClient::AbstractClient(QObject *parent) - : QObject(parent), nextCmdId(0), status(StatusDisconnected), serverSupportsPasswordHash(false) + : QObject(parent), nextCmdId(0), status(StatusDisconnected), serverSupportsPasswordHash(false), + serverSupportsChallengeResponse(false) { qRegisterMetaType("QVariant"); qRegisterMetaType("CommandContainer"); diff --git a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h index 1ef9a31e4..b6cc5d7a4 100644 --- a/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h +++ b/libcockatrice_network/libcockatrice/network/client/abstract/abstract_client.h @@ -117,6 +117,7 @@ protected: QMap pendingCommands; QString userName, password, email, country, realName, token; bool serverSupportsPasswordHash; + bool serverSupportsChallengeResponse; void setStatus(ClientStatus _status); int getNewCmdId() { @@ -150,6 +151,10 @@ public: { return serverSupportsPasswordHash; } + bool getServerSupportsChallengeResponse() const + { + return serverSupportsChallengeResponse; + } const QString &getUserName() const { return userName; diff --git a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp index 53608db65..c0c142891 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp +++ b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp @@ -27,7 +27,8 @@ static const unsigned int protocolVersion = 14; RemoteClient::RemoteClient(QObject *parent, INetworkSettingsProvider *_networkSettingsProvider) : AbstractClient(parent), networkSettingsProvider(_networkSettingsProvider), timeRunning(0), lastDataReceived(0), - messageInProgress(false), handshakeStarted(false), usingWebSocket(false), messageLength(0), hashedPassword() + messageInProgress(false), handshakeStarted(false), usingWebSocket(false), messageLength(0), hashedPassword(), + passwordNeedsMigration(false) { clearNewClientFeatures(); @@ -114,6 +115,8 @@ void RemoteClient::processServerIdentificationEvent(const Event_ServerIdentifica return; } serverSupportsPasswordHash = event.server_options() & Event_ServerIdentification::SupportsPasswordHash; + serverSupportsChallengeResponse = + event.server_options() & Event_ServerIdentification::SupportsChallengeResponseAuth; if (getStatus() == StatusRequestingForgotPassword) { Command_ForgotPasswordRequest cmdForgotPasswordRequest; @@ -130,7 +133,13 @@ void RemoteClient::processServerIdentificationEvent(const Event_ServerIdentifica cmdForgotPasswordReset.set_user_name(userName.toStdString()); cmdForgotPasswordReset.set_clientid(getSrvClientID(lastHostname).toStdString()); cmdForgotPasswordReset.set_token(token.toStdString()); - if (!password.isEmpty() && serverSupportsPasswordHash) { + 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(); hashedPassword = PasswordHasher::computeHash(password, passwordSalt); cmdForgotPasswordReset.set_hashed_new_password(hashedPassword.toStdString()); @@ -158,7 +167,10 @@ void RemoteClient::processServerIdentificationEvent(const Event_ServerIdentifica if (getStatus() == StatusRegistering) { Command_Register cmdRegister; cmdRegister.set_user_name(userName.toStdString()); - if (!password.isEmpty() && serverSupportsPasswordHash) { + if (!password.isEmpty() && serverSupportsChallengeResponse) { + hashedPassword = PasswordHasher::generatePasswordVerifier(password); + cmdRegister.set_hashed_password(hashedPassword.toStdString()); + } else if (!password.isEmpty() && serverSupportsPasswordHash) { auto passwordSalt = PasswordHasher::generateRandomSalt(); hashedPassword = PasswordHasher::computeHash(password, passwordSalt); cmdRegister.set_hashed_password(hashedPassword.toStdString()); @@ -223,7 +235,9 @@ Command_Login RemoteClient::generateCommandLogin() void RemoteClient::doLogin() { - if (!password.isEmpty() && serverSupportsPasswordHash) { + if ((!password.isEmpty() || !storedVerifier.isEmpty()) && serverSupportsChallengeResponse) { + doRequestPasswordSalt(); // ask salt + nonce to build the challenge response + } else if (!password.isEmpty() && serverSupportsPasswordHash) { //! \todo Store and log in using stored hashed password. if (hashedPassword.isEmpty()) { doRequestPasswordSalt(); // ask salt to create hashedPassword, then log in @@ -257,6 +271,30 @@ void RemoteClient::doHashedLogin() sendCommand(pend); } +void RemoteClient::doSubmitPasswordVerifier() +{ + pendingVerifier = PasswordHasher::generatePasswordVerifier(password); + Command_SubmitPasswordVerifier cmdSubmitVerifier; + cmdSubmitVerifier.set_password_verifier(pendingVerifier.toStdString()); + + PendingCommand *pend = prepareSessionCommand(cmdSubmitVerifier); + connect(pend, &PendingCommand::finished, this, &RemoteClient::submitPasswordVerifierResponse); + sendCommand(pend); +} + +void RemoteClient::submitPasswordVerifierResponse(const Response &response) +{ + if (response.response_code() == Response::RespOk) { + qCDebug(RemoteClientLog) << "Password verifier migrated successfully"; + if (!pendingVerifier.isEmpty()) { + emit sigPasswordVerifierReady(pendingVerifier); + pendingVerifier.clear(); + } + } else { + qCWarning(RemoteClientLog) << "Failed to migrate password verifier:" << response.response_code(); + } +} + void RemoteClient::processConnectionClosedEvent(const Event_ConnectionClosed & /*event*/) { doDisconnectFromServer(); @@ -269,7 +307,59 @@ void RemoteClient::passwordSaltResponse(const Response &response) auto passwordSalt = QString::fromStdString(resp.password_salt()); if (passwordSalt.isEmpty()) { // the server does not recognize the user but allows them to enter unregistered password.clear(); // the password will not be used + storedVerifier.clear(); doLogin(); + } else if (serverSupportsChallengeResponse && resp.has_nonce()) { + const QByteArray nonce = QByteArray::fromStdString(resp.nonce()); + QByteArray key; + if (resp.needs_migration()) { + // The account still uses the legacy format; the legacy full hash + // is only derivable from the plaintext password. + if (password.isEmpty()) { + emit loginError(Response::RespClientUpdateRequired, + QStringLiteral("This account must be logged in with its password once."), 0, {}); + return; + } + 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) + .arg(p) + .arg(passwordSalt) + .arg(QString(key.toBase64())); + } else if (!storedVerifier.isEmpty()) { + const PasswordVerifier verifier = PasswordHasher::parsePasswordVerifier(storedVerifier); + if (!verifier.isValid) { + emit loginError(Response::RespClientUpdateRequired, QStringLiteral("Stored verifier is invalid."), + 0, {}); + return; + } + key = verifier.verifier; + } else { + emit loginError(Response::RespLoginNeeded, {}, 0, {}); + return; + } + passwordNeedsMigration = resp.needs_migration(); + const QByteArray responseBytes = PasswordHasher::computeResponse(key, nonce); + hashedPassword = "$challenge$" + QString(nonce.toBase64()) + "$" + QString(responseBytes.toBase64()); + doHashedLogin(); } else { hashedPassword = PasswordHasher::computeHash(password, passwordSalt); doHashedLogin(); @@ -294,6 +384,14 @@ void RemoteClient::loginResponse(const Response &response) setStatus(StatusLoggedIn); emit userInfoChanged(resp.user_info()); + if (passwordNeedsMigration) { + // The account still used the legacy password format; upgrade it to scrypt. + doSubmitPasswordVerifier(); + } else if (!derivedVerifier.isEmpty()) { + emit sigPasswordVerifierReady(derivedVerifier); + derivedVerifier.clear(); + } + QList buddyList; for (int i = resp.buddy_list_size() - 1; i >= 0; --i) { buddyList.append(resp.buddy_list(i)); @@ -550,6 +648,8 @@ void RemoteClient::doDisconnectFromServer() websocket->close(); } socket->close(); + derivedVerifier.clear(); + pendingVerifier.clear(); } void RemoteClient::ping() @@ -712,6 +812,12 @@ void RemoteClient::submitForgotPasswordResetResponse(const Response &response) { if (response.response_code() == Response::RespOk) { emit sigForgotPasswordSuccess(); + // 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 862dac06e..941eaba25 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h +++ b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.h @@ -54,6 +54,11 @@ signals: unsigned int port, 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. 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(); @@ -80,6 +85,8 @@ private slots: void doLogin(); void doHashedLogin(); Command_Login generateCommandLogin(); + void doSubmitPasswordVerifier(); + void submitPasswordVerifierResponse(const Response &response); void doDisconnectFromServer(); void doActivateToServer(const QString &_token); void doRequestForgotPasswordToServer(const QString &hostname, unsigned int port, const QString &_userName); @@ -111,6 +118,14 @@ private: QString lastHostname; unsigned int lastPort; QString hashedPassword; + bool passwordNeedsMigration; + //! \brief A previously stored "$scrypt$..." verifier used to authenticate + //! without the plaintext password. + QString storedVerifier; + //! \brief Verifier derived during the current login, persisted after success. + QString derivedVerifier; + //! \brief Verifier sent for migration, persisted once the server accepts it. + QString pendingVerifier; QString getSrvClientID(const QString &_hostname); bool newMissingFeatureFound(const QString &_serversMissingFeatures); @@ -149,6 +164,12 @@ public: } void connectToServer(const QString &hostname, unsigned int port, const QString &_userName, const QString &_password); + //! \brief Provide a stored "$scrypt$..." verifier so the client can + //! authenticate without the plaintext password. + void setStoredVerifier(const QString &verifier) + { + storedVerifier = verifier; + } void registerToServer(const QString &hostname, unsigned int port, const QString &_userName, diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server.h b/libcockatrice_network/libcockatrice/network/server/remote/server.h index 0ded27afa..bf1d39296 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server.h @@ -85,6 +85,11 @@ public: { return QMap(); } + /** @brief True when only challenge-response logins are accepted (strict mode). */ + virtual bool requiresChallengeResponseAuth() const + { + return false; + } void addClient(Server_ProtocolHandler *player); void removeClient(Server_ProtocolHandler *player); QList getOnlineModeratorList() const; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_database_interface.h b/libcockatrice_network/libcockatrice/network/server/remote/server_database_interface.h index 1e4fc990b..11b026180 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_database_interface.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_database_interface.h @@ -40,6 +40,14 @@ public: { return {}; } + virtual QString getUserPasswordData(const QString & /* user */) + { + return {}; + } + virtual bool submitPasswordVerifier(const QString & /* user */, const QString & /* passwordVerifier */) + { + return false; + } virtual QMap getBuddyList(const QString & /* name */) { return QMap(); diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index 899df6529..8482f9c0d 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -44,6 +44,22 @@ Server_ProtocolHandler::~Server_ProtocolHandler() { } +void Server_ProtocolHandler::setAuthNonce(const QByteArray &nonce) +{ + authNonce = nonce; + authNonceCreated = QDateTime::currentDateTimeUtc(); +} + +bool Server_ProtocolHandler::isAuthNonceValid(const QByteArray &nonce) const +{ + return !authNonce.isEmpty() && authNonce == nonce && authNonceCreated.secsTo(QDateTime::currentDateTimeUtc()) < 60; +} + +void Server_ProtocolHandler::clearAuthNonce() +{ + authNonce.clear(); +} + // This function must only be called from the thread this object lives in. // Except when the server is shutting down. // The thread must not hold any server locks when calling this (e.g. clientsLock, roomsLock). @@ -507,6 +523,16 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd return Response::RespContextError; } + // In strict mode only challenge-response logins are accepted. + if (server->requiresChallengeResponseAuth() && + (!cmd.has_hashed_password() || cmd.hashed_password().rfind("$challenge$", 0) != 0)) { + auto *re = new Response_Login; + re->set_denied_reason_str("Client upgrade required"); + re->add_missing_features("challenge_response_auth"); + rc.setResponseExtension(re); + return Response::RespClientUpdateRequired; + } + // check client feature set against server feature set FeatureSet features; QMap receivedClientFeatures; diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h index 0d05b91c8..c4917e845 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h @@ -4,6 +4,8 @@ #include "server.h" #include "server_abstractuserinterface.h" +#include +#include #include #include #include @@ -55,6 +57,8 @@ protected: bool acceptsUserListChanges; bool acceptsRoomListChanges; bool idleClientWarningSent; + QByteArray authNonce; + QDateTime authNonceCreated; virtual void logDebugMessage(const QString & /* message */) { } @@ -124,6 +128,13 @@ public: return databaseInterface; } + /** @brief Store a fresh challenge nonce for the next challenge-response login attempt. */ + void setAuthNonce(const QByteArray &nonce); + /** @brief True if nonce matches the pending one and was issued less than 60 seconds ago. */ + bool isAuthNonceValid(const QByteArray &nonce) const; + /** @brief Invalidate the pending nonce (single-use). */ + void clearAuthNonce(); + int getLastCommandTime() const { return timeRunning - lastDataReceived; diff --git a/libcockatrice_protocol/libcockatrice/protocol/featureset.cpp b/libcockatrice_protocol/libcockatrice/protocol/featureset.cpp index 3e687ef56..439c68748 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/featureset.cpp +++ b/libcockatrice_protocol/libcockatrice/protocol/featureset.cpp @@ -25,7 +25,8 @@ void FeatureSet::initalizeFeatureList(QMap &_featureList) _featureList.insert("idle_client", false); _featureList.insert("forgot_password", false); _featureList.insert("websocket", false); - // featureList.insert("hashed_password_login", false); + _featureList.insert("hashed_password_login", false); + _featureList.insert("challenge_response_auth", false); // These are temp to force users onto a newer client _featureList.insert("2.7.0_min_version", false); _featureList.insert("2.8.0_min_version", false); diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/event_server_identification.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/event_server_identification.proto index 987ab20d1..371ae1e95 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/event_server_identification.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/event_server_identification.proto @@ -8,6 +8,7 @@ message Event_ServerIdentification { enum ServerOptions { NoOptions = 0; SupportsPasswordHash = 1; + SupportsChallengeResponseAuth = 2; } optional string server_name = 1; optional string server_version = 2; diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/response_password_salt.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/response_password_salt.proto index 3fc228530..e6d036794 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/response_password_salt.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/response_password_salt.proto @@ -6,4 +6,14 @@ message Response_PasswordSalt { optional Response_PasswordSalt ext = 1017; } optional string password_salt = 1; + // scrypt cost parameters for password_salt. Absent/zero for legacy accounts. + optional int32 n = 2; + optional int32 r = 3; + optional int32 p = 4; + // Server-generated challenge. When present the client must authenticate + // with a challenge-response instead of transmitting the password hash. + optional bytes nonce = 5; + // True when the account still uses the legacy password format and should + // be migrated to the scrypt format after a successful login. + optional bool needs_migration = 6; } diff --git a/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto b/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto index fee8c36a8..d7d7ca54a 100644 --- a/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto +++ b/libcockatrice_protocol/libcockatrice/protocol/pb/session_commands.proto @@ -28,6 +28,7 @@ message SessionCommand { FORGOT_PASSWORD_CHALLENGE = 1023; REQUEST_PASSWORD_SALT = 1024; SET_CARD_ART_PARAMS = 1025; + SUBMIT_PASSWORD_VERIFIER = 1026; REPLAY_LIST = 1100; REPLAY_DOWNLOAD = 1101; REPLAY_MODIFY_MATCH = 1102; @@ -223,3 +224,14 @@ message Command_SetCardArtParams { optional double vertical_offset = 5; optional double zoom = 6; } + +// Client uploads the new password verifier to migrate a legacy account +// after a successful challenge-response login. Idempotent; only applies +// to accounts still using the legacy password format. +message Command_SubmitPasswordVerifier { + extend SessionCommand { + optional Command_SubmitPasswordVerifier ext = 1026; + } + // Full verifier string to store, e.g. "$scrypt$32768$8$1$$" + required string password_verifier = 1; +} diff --git a/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp b/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp index 811b0c842..8ae63fc81 100644 --- a/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp @@ -146,6 +146,14 @@ void ServersSettings::setFPPlayerName(QString playerName) setValue(playerName, "fpPlayerName"); } +void ServersSettings::setServerPassword(const QString &saveName, const QString &password) +{ + const int index = getPrevioushostindex(saveName); + if (index >= 0) { + setValue(password, QString("password%1").arg(index), "server", "server_details"); + } +} + QString ServersSettings::getFPPlayerName(QString defaultName) const { QVariant name = getValue("fpPlayerName"); diff --git a/libcockatrice_settings/libcockatrice/settings/servers_settings.h b/libcockatrice_settings/libcockatrice/settings/servers_settings.h index f9803a158..c4ee894c9 100644 --- a/libcockatrice_settings/libcockatrice/settings/servers_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/servers_settings.h @@ -46,6 +46,8 @@ public: void setFPHostName(QString hostname); void setFPPort(QString port); void setFPPlayerName(QString playerName); + //! \brief Store a password (or a "$scrypt$..." verifier) for the given saved server. + void setServerPassword(const QString &saveName, const QString &password); void addNewServer(const QString &saveName, const QString &serv, const QString &port, diff --git a/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp b/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp index 1c22fdcfa..ffde16a04 100644 --- a/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp +++ b/libcockatrice_utility/libcockatrice/utility/passwordhasher.cpp @@ -2,6 +2,9 @@ #include #include +#include +#include +#include QString PasswordHasher::computeHash(const QString &password, const QString &salt) { @@ -52,3 +55,114 @@ QString PasswordHasher::generateActivationToken() { return QString(CryptoUtil::randomBytes(16).toBase64().left(16)); } + +QByteArray PasswordHasher::deriveKey(const QString &password, const QByteArray &salt, int n, int r, int p) +{ + QByteArray key(SCRYPT_VERIFIER_LENGTH, '\0'); + const QByteArray passwordUtf8 = password.toUtf8(); + // EVP_PBE_scrypt aborts unless maxmem covers the required working memory, + // which is roughly 128 * n * r bytes (plus the small Salsa20/8 block array). + const auto maxmem = static_cast(128) * n * r + static_cast(128) * r * p + 4096; + 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) { + 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); + const QByteArray verifier = deriveKey(password, salt, SCRYPT_N, SCRYPT_R, SCRYPT_P); + return QString("$scrypt$%1$%2$%3$%4$%5") + .arg(SCRYPT_N) + .arg(SCRYPT_R) + .arg(SCRYPT_P) + .arg(QString(salt.toBase64())) + .arg(QString(verifier.toBase64())); +} + +PasswordVerifier PasswordHasher::parsePasswordVerifier(const QString &stored) +{ + PasswordVerifier result; + const QStringList parts = stored.split("$"); + if (parts.size() != 7 || parts.at(1) != "scrypt") { + return result; + } + + bool ok = false; + const int n = parts.at(2).toInt(&ok); + if (!ok) { + return result; + } + const int r = parts.at(3).toInt(&ok); + if (!ok) { + return result; + } + const int p = parts.at(4).toInt(&ok); + if (!ok || !costParamsAreSane(n, r, p)) { + return result; + } + + const QByteArray salt = QByteArray::fromBase64(parts.at(5).toUtf8()); + const QByteArray verifier = QByteArray::fromBase64(parts.at(6).toUtf8()); + if (salt.isEmpty() || verifier.size() != SCRYPT_VERIFIER_LENGTH) { + return result; + } + + result.format = PasswordFormat::Scrypt; + result.n = n; + result.r = r; + result.p = p; + result.salt = salt; + result.verifier = verifier; + result.isValid = true; + return result; +} + +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'); + unsigned int responseLength = 0; + if (HMAC(EVP_sha256(), key.constData(), key.size(), reinterpret_cast(nonce.constData()), + nonce.size(), reinterpret_cast(response.data()), &responseLength) == nullptr) { + qFatal("PasswordHasher::computeResponse: HMAC failed"); + } + response.resize(responseLength); + return response; +} + +bool PasswordHasher::constantTimeEquals(const QByteArray &a, const QByteArray &b) +{ + if (a.size() != b.size()) { + return false; + } + return CRYPTO_memcmp(a.constData(), b.constData(), a.size()) == 0; +} diff --git a/libcockatrice_utility/libcockatrice/utility/passwordhasher.h b/libcockatrice_utility/libcockatrice/utility/passwordhasher.h index 811ecef15..a00146b6e 100644 --- a/libcockatrice_utility/libcockatrice/utility/passwordhasher.h +++ b/libcockatrice_utility/libcockatrice/utility/passwordhasher.h @@ -1,14 +1,57 @@ #ifndef PASSWORDHASHER_H #define PASSWORDHASHER_H +#include #include +// scrypt cost parameters used for newly created password verifiers. These match +// the RFC 7914 recommended parameters for interactive use. +constexpr int SCRYPT_N = 32768; +constexpr int SCRYPT_R = 8; +constexpr int SCRYPT_P = 1; +constexpr int SCRYPT_SALT_LENGTH = 16; +constexpr int SCRYPT_VERIFIER_LENGTH = 64; + +enum class PasswordFormat +{ + None = 0, + Scrypt +}; + +struct PasswordVerifier +{ + PasswordFormat format = PasswordFormat::None; + int n = 0; + int r = 0; + int p = 0; + QByteArray salt; + QByteArray verifier; + bool isValid = false; +}; + class PasswordHasher { public: static QString computeHash(const QString &password, const QString &salt); static QString generateRandomSalt(const int len = 16); static QString generateActivationToken(); + + /** @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. */ + static bool constantTimeEquals(const QByteArray &a, const QByteArray &b); }; #endif diff --git a/servatrice/migrations/servatrice_0036_to_0037.sql b/servatrice/migrations/servatrice_0036_to_0037.sql new file mode 100644 index 000000000..de3e09987 --- /dev/null +++ b/servatrice/migrations/servatrice_0036_to_0037.sql @@ -0,0 +1,8 @@ +-- Servatrice db migration from version 36 to version 37 + +-- The column must hold "$scrypt$$$

$$" (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 c1940c22f..f7e1feed9 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -350,6 +350,22 @@ max_users_websocket=500 ; Maximum number of users that can connect from the same IP address; useful to avoid bots, default is 4 max_users_per_address=4 +; How strictly new authentication features are enforced. Possible values: +; * legacy: accounts that still use the legacy 1000-round SHA-512 hash keep logging in with it +; (they are served the legacy salt, never a challenge-response nonce) and are never +; auto-migrated to scrypt. Already-migrated scrypt rows keep logging in via +; challenge-response. New credentials may use either format; +; * mixed: accept both legacy hashes and challenge-response authentication (default); +; * strict: only accept challenge-response authentication from clients that support it, +; and reject plain password submissions. Legacy accounts are migrated to scrypt +; automatically on their next successful login. +; +; Challenge-response verifiers are scrypt (RFC 7914, N=32768, r=8, p=1) stored as +; "$scrypt$$$

$$". 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 5dbf69cbc..0bee10cd0 100644 --- a/servatrice/servatrice.sql +++ b/servatrice/servatrice.sql @@ -20,7 +20,7 @@ CREATE TABLE IF NOT EXISTS `cockatrice_schema_version` ( PRIMARY KEY (`version`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 DEFAULT COLLATE utf8mb4_unicode_ci; -INSERT INTO cockatrice_schema_version VALUES(36); +INSERT INTO cockatrice_schema_version VALUES(37); -- users and user data tables CREATE TABLE IF NOT EXISTS `cockatrice_users` ( @@ -28,7 +28,7 @@ CREATE TABLE IF NOT EXISTS `cockatrice_users` ( `admin` tinyint(1) NOT NULL, `name` varchar(35) NOT NULL, `realname` varchar(255) NOT NULL, - `password_sha512` char(120) NOT NULL, + `password_sha512` varchar(255) NOT NULL, `email` varchar(255) NOT NULL, `country` char(2) NOT NULL, `avatar_bmp` mediumblob NOT NULL, diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index db8751658..3344cf25a 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -900,6 +900,18 @@ QString Servatrice::getRequiredFeatures() const return settingsCache->value("server/requiredfeatures", "").toString(); } +Servatrice::AuthenticationStrictness Servatrice::getAuthenticationStrictness() const +{ + const QString strictness = settingsCache->value("security/authentication_strictness", "mixed").toString(); + if (strictness == "strict") { + return AuthenticationStrict; + } + if (strictness == "legacy") { + return AuthenticationLegacy; + } + return AuthenticationMixed; +} + QString Servatrice::getDBTypeString() const { if (QProcessEnvironment::systemEnvironment().contains("DATABASE_URL")) { diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 8b0f5ad60..47a7a7dcf 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -140,6 +140,12 @@ public: AuthenticationSql, AuthenticationPassword }; + enum AuthenticationStrictness + { + AuthenticationLegacy, + AuthenticationMixed, + AuthenticationStrict + }; private slots: void statusUpdate(); void shutdownTimeout(); @@ -219,6 +225,10 @@ public: { return serverRequiredFeatureList; } + bool requiresChallengeResponseAuth() const override + { + return getAuthenticationStrictness() == AuthenticationStrict; + } QString getServerName() const; QString getLoginMessage() const override { @@ -240,6 +250,7 @@ public: { return authenticationMethod; } + AuthenticationStrictness getAuthenticationStrictness() const; bool permitUnregisteredUsers() const override { return authenticationMethod != AuthenticationNone; diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index 847be61da..caa3478c8 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/servatrice/src/servatrice_database_interface.cpp @@ -356,6 +356,47 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot qCWarning(DatabaseInterfaceLog) << "Login denied: user not active"; return UserIsInactive; } + + if (password.startsWith("$challenge$")) { + // Challenge-response login: verify HMAC(stored_key, nonce) without + // ever transmitting the stored credential or password hash. + const QStringList parts = password.split("$"); + if (parts.size() != 4) { + return NotLoggedIn; + } + const QByteArray nonce = QByteArray::fromBase64(parts.at(2).toUtf8()); + const QByteArray response = QByteArray::fromBase64(parts.at(3).toUtf8()); + if (nonce.isEmpty() || response.isEmpty() || !handler->isAuthNonceValid(nonce)) { + return NotLoggedIn; + } + + QByteArray key; + if (PasswordHasher::isLegacyFormat(correctPasswordSha512)) { + key = correctPasswordSha512.toUtf8(); + } else { + // Design note: the stored scrypt verifier IS the challenge-response key, so a + // database dump yields credentials that can answer a login challenge directly. + // We deliberately accept this trade: it removes plaintext passwords from the + // client config and from the wire, but does not protect against DB compromise. + // A password-equivalent proof scheme (e.g. SRP-6a/OPAQUE) would be the proper + // escalation and is out of scope here. + const PasswordVerifier verifier = PasswordHasher::parsePasswordVerifier(correctPasswordSha512); + if (!verifier.isValid) { + return NotLoggedIn; + } + key = verifier.verifier; + } + + const QByteArray expected = PasswordHasher::computeResponse(key, nonce); + handler->clearAuthNonce(); + if (PasswordHasher::constantTimeEquals(expected, response)) { + qCDebug(DatabaseInterfaceLog) << "Login accepted: challenge-response password right"; + return PasswordRight; + } + qCDebug(DatabaseInterfaceLog) << "Login denied: challenge-response password wrong"; + return NotLoggedIn; + } + QString hashedPassword; if (passwordNeedsHash) { hashedPassword = PasswordHasher::computeHash(password, correctPasswordSha512.left(16)); @@ -558,6 +599,48 @@ QString Servatrice_DatabaseInterface::getUserSalt(const QString &user) return {}; } +QString Servatrice_DatabaseInterface::getUserPasswordData(const QString &user) +{ + if (server->getAuthenticationMethod() != Servatrice::AuthenticationSql) { + return {}; + } + + checkSql(); + + QSqlQuery *query = prepareQuery("SELECT password_sha512 FROM {prefix}_users WHERE name = :name"); + query->bindValue(":name", user); + if (!execSqlQuery(query)) { + return {}; + } + + if (!query->next()) { + return {}; + } + + return query->value(0).toString(); +} + +bool Servatrice_DatabaseInterface::submitPasswordVerifier(const QString &user, const QString &passwordVerifier) +{ + if (server->getAuthenticationMethod() != Servatrice::AuthenticationSql) { + return false; + } + + checkSql(); + + // Only migrate accounts that still use the legacy format; the query is a no-op otherwise. + QSqlQuery *query = prepareQuery( + "update {prefix}_users set password_sha512 = :verifier where name = :user and password_sha512 not like '$%'"); + query->bindValue(":verifier", passwordVerifier); + query->bindValue(":user", user); + if (!execSqlQuery(query)) { + qCWarning(DatabaseInterfaceLog) << "Failed to submit password verifier for user" << user << query->lastError(); + return false; + } + // The guard makes a re-migration a no-op; only report success when a row was actually updated. + return query->numRowsAffected() > 0; +} + int Servatrice_DatabaseInterface::getUserIdInDB(const QString &name) { if (server->getAuthenticationMethod() == Servatrice::AuthenticationSql) { @@ -1139,13 +1222,13 @@ bool Servatrice_DatabaseInterface::changeUserPassword(const QString &user, return false; } - const QString correctPasswordSha512 = passwordQuery->value(0).toString(); - QString oldPasswordSha512 = oldPassword; - if (oldPasswordNeedsHash) { - QString salt = correctPasswordSha512.left(16); - oldPasswordSha512 = PasswordHasher::computeHash(oldPassword, salt); - } - if (correctPasswordSha512 != oldPasswordSha512) { + const QString storedPassword = passwordQuery->value(0).toString(); + // oldPasswordNeedsHash means the client sent the old password in plaintext. Verify it + // against whatever is stored: legacy salt+hash rows or already-migrated scrypt rows + // (which must NOT be re-hashed with a salt torn out of the "$scrypt$..." string). + const bool oldPasswordMatches = oldPasswordNeedsHash ? PasswordHasher::verifyPassword(oldPassword, storedPassword) + : (oldPassword == storedPassword); + if (!oldPasswordMatches) { return false; } diff --git a/servatrice/src/servatrice_database_interface.h b/servatrice/src/servatrice_database_interface.h index cd76ae288..3323a420f 100644 --- a/servatrice/src/servatrice_database_interface.h +++ b/servatrice/src/servatrice_database_interface.h @@ -13,7 +13,7 @@ #include #include -#define DATABASE_SCHEMA_VERSION 36 +#define DATABASE_SCHEMA_VERSION 37 class Servatrice; @@ -65,6 +65,8 @@ public: bool activeUserExists(const QString &user) override; bool userExists(const QString &user) override; QString getUserSalt(const QString &user) override; + QString getUserPasswordData(const QString &user) override; + bool submitPasswordVerifier(const QString &user, const QString &passwordVerifier) override; int getUserIdInDB(const QString &name); QMap getBuddyList(const QString &name) override; QMap getIgnoreList(const QString &name) override; diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 2a8b5f0a4..36a3e5766 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -107,6 +107,7 @@ #include #include #include +#include #include #include #include @@ -139,7 +140,14 @@ bool AbstractServerSocketInterface::initSession() identEvent.set_server_version(VERSION_STRING); identEvent.set_protocol_version(protocolVersion); if (servatrice->getAuthenticationMethod() == Servatrice::AuthenticationSql) { - identEvent.set_server_options(Event_ServerIdentification::SupportsPasswordHash); + // Challenge-response is advertised in every strictness mode: legacy accounts keep + // logging in with the legacy hash, but already-migrated scrypt rows are always + // served challenge-response (authentication_strictness only governs NEW credentials). + Event_ServerIdentification::ServerOptions serverOptions = + static_cast( + Event_ServerIdentification::SupportsPasswordHash | + Event_ServerIdentification::SupportsChallengeResponseAuth); + identEvent.set_server_options(serverOptions); } SessionEvent *identSe = prepareSessionEvent(identEvent); sendProtocolItem(*identSe); @@ -257,6 +265,8 @@ Response::ResponseCode AbstractServerSocketInterface::processExtendedSessionComm return cmdReportAddComment(cmd.GetExtension(Command_ReportAddComment::ext), rc); case SessionCommand::REPORT_DETAILS: return cmdReportDetails(cmd.GetExtension(Command_ReportDetails::ext), rc); + case SessionCommand::SUBMIT_PASSWORD_VERIFIER: + return cmdSubmitPasswordVerifier(cmd.GetExtension(Command_SubmitPasswordVerifier::ext), rc); default: return Response::RespFunctionNotAllowed; } @@ -2461,6 +2471,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRegisterAccount(const C password = QString::fromStdString(cmd.hashed_password()); } + // Reject credential formats the configured authentication strictness does not accept. + if (!acceptsCredentialFormat(passwordNeedsHash, password)) { + return Response::RespClientUpdateRequired; + } + bool requireEmailActivation = settingsCache->value("registration/requireemailactivation", true).toBool(); bool regSucceeded = sqlInterface->registerUser(userName, realName, password, passwordNeedsHash, parsedEmailAddress, country, !requireEmailActivation); @@ -2507,6 +2522,21 @@ bool AbstractServerSocketInterface::tooManyRegistrationAttempts(const QString &i return false; } +bool AbstractServerSocketInterface::acceptsCredentialFormat(bool passwordNeedsHash, const QString &password) const +{ + // "scryptFormat" means the client sent a derived verifier rather than a password to hash ourselves. + const bool scryptFormat = !passwordNeedsHash && !PasswordHasher::isLegacyFormat(password); + + // The strictness mode governs how existing legacy accounts are served, not which new-credential + // formats are tolerated: a legacy-mode server must still accept scrypt verifiers, because clients + // derive them whenever challenge-response is advertised (and it must be, so already-migrated + // scrypt rows keep logging in). strict is the only mode that rejects legacy formats. + if (servatrice->getAuthenticationStrictness() == Servatrice::AuthenticationStrict) { + return scryptFormat; + } + return true; // legacy and mixed accept either format +} + Response::ResponseCode AbstractServerSocketInterface::cmdActivateAccount(const Command_Activate &cmd, ResponseContainer & /*rc*/) { @@ -2841,6 +2871,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdAccountPassword(const C newPassword = QString::fromStdString(cmd.hashed_new_password()); } + // Reject new credential formats the configured authentication strictness does not accept. + if (!acceptsCredentialFormat(newPasswordNeedsHash, newPassword)) { + return Response::RespClientUpdateRequired; + } + QString userName = QString::fromStdString(userInfo->name()); if (!databaseInterface->changeUserPassword(userName, oldPassword, true, newPassword, newPasswordNeedsHash)) { return Response::RespWrongPassword; @@ -2978,6 +3013,11 @@ Response::ResponseCode AbstractServerSocketInterface::cmdForgotPasswordReset(con password = QString::fromStdString(cmd.hashed_new_password()); } + // Reject new credential formats the configured authentication strictness does not accept. + if (!acceptsCredentialFormat(passwordNeedsHash, password)) { + return Response::RespClientUpdateRequired; + } + if (sqlInterface->changeUserPassword(nameFromStdString(cmd.user_name()), password, passwordNeedsHash)) { if (servatrice->getEnableForgotPasswordAudit()) { sqlInterface->addAuditRecord(userName.simplified(), this->getAddress(), clientId.simplified(), @@ -3038,8 +3078,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con ResponseContainer &rc) { const QString userName = nameFromStdString(cmd.user_name()); - QString passwordSalt = sqlInterface->getUserSalt(userName); - if (passwordSalt.isEmpty()) { + const QString storedPasswordData = sqlInterface->getUserPasswordData(userName); + if (storedPasswordData.isEmpty()) { if (server->getRegOnlyServerEnabled()) { return Response::RespRegistrationRequired; } else { @@ -3047,8 +3087,36 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con return Response::RespOk; } } + auto *re = new Response_PasswordSalt; - re->set_password_salt(passwordSalt.toStdString()); + if (PasswordHasher::isLegacyFormat(storedPasswordData)) { + re->set_password_salt(storedPasswordData.left(16).toStdString()); + re->set_needs_migration(true); + // Legacy rows get a challenge-response nonce only outside legacy mode (there the client + // logs in with the legacy hash and the account is migrated). In legacy mode the row is + // served the legacy salt, since legacy mode only governs what NEW credentials are accepted. + if (servatrice->getAuthenticationStrictness() != Servatrice::AuthenticationLegacy) { + const QByteArray nonce = CryptoUtil::randomBytes(32); + setAuthNonce(nonce); + re->set_nonce(nonce.constData(), nonce.size()); + } + } else { + const PasswordVerifier verifier = PasswordHasher::parsePasswordVerifier(storedPasswordData); + if (!verifier.isValid) { + delete re; + return Response::RespContextError; + } + re->set_password_salt(QString(verifier.salt.toBase64()).toStdString()); + re->set_n(verifier.n); + re->set_r(verifier.r); + re->set_p(verifier.p); + re->set_needs_migration(false); + // scrypt rows are served challenge-response in every mode so migrated accounts never lock out. + const QByteArray nonce = CryptoUtil::randomBytes(32); + setAuthNonce(nonce); + re->set_nonce(nonce.constData(), nonce.size()); + } + rc.setResponseExtension(re); return Response::RespOk; } @@ -3147,6 +3215,39 @@ Response::ResponseCode AbstractServerSocketInterface::cmdReport(const Command_Re return Response::RespOk; } +Response::ResponseCode +AbstractServerSocketInterface::cmdSubmitPasswordVerifier(const Command_SubmitPasswordVerifier &cmd, + ResponseContainer & /*rc*/) +{ + if (authState != PasswordRight) { + return Response::RespLoginNeeded; + } + + // Limit to the size of the database column (password_sha512 varchar(255)). + constexpr int MAX_PASSWORD_VERIFIER_LENGTH = 255; + const QString passwordVerifier = QString::fromStdString(cmd.password_verifier()); + if (passwordVerifier.isEmpty() || passwordVerifier.length() > MAX_PASSWORD_VERIFIER_LENGTH || + PasswordHasher::isLegacyFormat(passwordVerifier)) { + return Response::RespContextError; + } + + // Reject unparseable or hostile cost parameters before they reach the database. + const PasswordVerifier parsedVerifier = PasswordHasher::parsePasswordVerifier(passwordVerifier); + if (!parsedVerifier.isValid) { + qCWarning(AbstractServerSocketInterfaceLog) + << "Rejecting password verifier submission with invalid or insane cost parameters"; + return Response::RespContextError; + } + + if (!sqlInterface->submitPasswordVerifier(QString::fromStdString(userInfo->name()), passwordVerifier)) { + return Response::RespContextError; + } + + qCDebug(AbstractServerSocketInterfaceLog) + << "Password verifier migrated for user" << QString::fromStdString(userInfo->name()); + return Response::RespOk; +} + // ADMIN FUNCTIONS. // Permission is checked by the calling function. diff --git a/servatrice/src/serversocketinterface.h b/servatrice/src/serversocketinterface.h index 600796b5f..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; @@ -145,6 +146,7 @@ private: Response::ResponseCode cmdReportDetails(const Command_ReportDetails &cmd, ResponseContainer &rc); Response::ResponseCode cmdReportAddComment(const Command_ReportAddComment &cmd, ResponseContainer &rc); Response::ResponseCode cmdReplayDownloadByGameId(const Command_ReplayDownloadByGameId &cmd, ResponseContainer &rc); + Response::ResponseCode cmdSubmitPasswordVerifier(const Command_SubmitPasswordVerifier &cmd, ResponseContainer &rc); Response::ResponseCode processExtendedSessionCommand(int cmdType, const SessionCommand &cmd, ResponseContainer &rc) override; Response::ResponseCode diff --git a/tests/password_hash_test.cpp b/tests/password_hash_test.cpp index 2b8f8bdb7..727677ee5 100644 --- a/tests/password_hash_test.cpp +++ b/tests/password_hash_test.cpp @@ -36,6 +36,137 @@ TEST(PasswordHashTest, TokenHasExpectedLength) const QString token = PasswordHasher::generateActivationToken(); ASSERT_EQ(token.size(), 16); } + +TEST(PasswordHashTest, DeriveKeyMatchesKnownVector) +{ + // RFC 7914 scrypt test vector, P="password", S="NaCl", N=1024, r=8, p=16 + const QByteArray expected = QByteArray::fromHex("fdbabe1c9d3472007856e7190d01e9fe7c6ad7cbc8237830e77376634b" + "3731622eaf30d92e22a3886ff109279d9830dac727afb94a83ee6d8360cb" + "dfa2cc0640"); + const QByteArray derived = PasswordHasher::deriveKey("password", QByteArray("NaCl"), 1024, 8, 16); + ASSERT_EQ(derived.toHex(), expected.toHex()); +} + +TEST(PasswordHashTest, PasswordVerifierRoundTrip) +{ + const QString stored = PasswordHasher::generatePasswordVerifier("hunter2"); + ASSERT_FALSE(stored.isEmpty()); + ASSERT_TRUE(stored.startsWith("$scrypt$")); + + const PasswordVerifier parsed = PasswordHasher::parsePasswordVerifier(stored); + ASSERT_TRUE(parsed.isValid); + ASSERT_EQ(parsed.format, PasswordFormat::Scrypt); + ASSERT_EQ(parsed.n, SCRYPT_N); + ASSERT_EQ(parsed.r, SCRYPT_R); + ASSERT_EQ(parsed.p, SCRYPT_P); + ASSERT_EQ(parsed.salt.size(), SCRYPT_SALT_LENGTH); + ASSERT_EQ(parsed.verifier.size(), SCRYPT_VERIFIER_LENGTH); +} + +TEST(PasswordHashTest, PasswordVerifierInvalidInput) +{ + ASSERT_FALSE(PasswordHasher::parsePasswordVerifier("garbage").isValid); + ASSERT_FALSE(PasswordHasher::parsePasswordVerifier("$scrypt$not-an-int$8$1$AAAA$BBBB").isValid); + ASSERT_FALSE(PasswordHasher::parsePasswordVerifier("$scrypt$1024$8$1$AAAA$too-short").isValid); + ASSERT_FALSE(PasswordHasher::parsePasswordVerifier("$pbkdf2-sha512$1000$AAAA$BBBB").isValid); +} + +TEST(PasswordHashTest, LegacyFormatDetection) +{ + ASSERT_TRUE(PasswordHasher::isLegacyFormat("salt+hash")); + ASSERT_FALSE(PasswordHasher::isLegacyFormat(PasswordHasher::generatePasswordVerifier("password"))); +} + +TEST(PasswordHashTest, DeriveKeyDependsOnCostParameters) +{ + const QByteArray keyA = PasswordHasher::deriveKey("password", QByteArray("NaCl"), 1024, 8, 16); + const QByteArray keyB = PasswordHasher::deriveKey("password", QByteArray("NaCl"), 2048, 8, 16); + const QByteArray keyC = PasswordHasher::deriveKey("password", QByteArray("NaCl"), 1024, 8, 1); + ASSERT_NE(keyA, keyB); + ASSERT_NE(keyA, keyC); +} + +TEST(PasswordHashTest, ComputeResponseIsDeterministic) +{ + const QByteArray nonce = QByteArray("a nonce value"); + const QByteArray key = QByteArray("the verifier bytes"); + const QByteArray r1 = PasswordHasher::computeResponse(key, nonce); + const QByteArray r2 = PasswordHasher::computeResponse(key, nonce); + const QByteArray r3 = PasswordHasher::computeResponse(QByteArray("a different key"), nonce); + ASSERT_EQ(r1, r2); + ASSERT_NE(r1, r3); +} + +TEST(PasswordHashTest, ConstantTimeEquals) +{ + ASSERT_TRUE(PasswordHasher::constantTimeEquals(QByteArray("same"), QByteArray("same"))); + ASSERT_FALSE(PasswordHasher::constantTimeEquals(QByteArray("same"), QByteArray("diff"))); + 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)