diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_connect.h b/cockatrice/src/interface/widgets/dialogs/dlg_connect.h index a1d6fcb23..456c5af93 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_connect.h +++ b/cockatrice/src/interface/widgets/dialogs/dlg_connect.h @@ -55,10 +55,7 @@ public: } [[nodiscard]] QString getSaveName() const { - // Trim so the profile lookup key always matches the name DlgConnect::actOk - // stored (it saves under saveEdit->text().trimmed()); otherwise a trailing - // space here misses the index and silently drops the saved verifier. - return saveEdit->text().trimmed(); + return saveEdit->text(); } [[nodiscard]] bool getSavePassword() const { diff --git a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp index 04a58ae2d..c0c142891 100644 --- a/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp +++ b/libcockatrice_network/libcockatrice/network/client/remote/remote_client.cpp @@ -311,17 +311,6 @@ void RemoteClient::passwordSaltResponse(const Response &response) doLogin(); } else if (serverSupportsChallengeResponse && resp.has_nonce()) { const QByteArray nonce = QByteArray::fromStdString(resp.nonce()); - // needs_migration is a server-controlled flag over an unauthenticated - // transport (plain TCP by default). If we already hold a scrypt - // verifier for this account, a server telling us to fall back to the - // 1000-round legacy hash is a downgrade of the KDF — refuse it rather - // than hand an attacker a weakly-keyed HMAC they can attack offline. - if (resp.needs_migration() && PasswordHasher::parsePasswordVerifier(storedVerifier).isValid) { - emit loginError(Response::RespClientUpdateRequired, - QStringLiteral("Server asked to downgrade authentication for a migrated account."), 0, - {}); - return; - } QByteArray key; if (resp.needs_migration()) { // The account still uses the legacy format; the legacy full hash diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp index bf43d90e7..8482f9c0d 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp @@ -1,8 +1,5 @@ #include "server_protocolhandler.h" -// Challenge-response nonces are valid for at most one minute from issuance. -static constexpr qint64 kAuthNonceLifetimeSeconds = 60; - #include "game/game_config.h" #include "game/server_game.h" #include "game/server_player.h" @@ -47,26 +44,20 @@ Server_ProtocolHandler::~Server_ProtocolHandler() { } -void Server_ProtocolHandler::setAuthNonce(const QByteArray &nonce, const QString &userName) +void Server_ProtocolHandler::setAuthNonce(const QByteArray &nonce) { authNonce = nonce; - authNonceUser = userName; authNonceCreated = QDateTime::currentDateTimeUtc(); } -bool Server_ProtocolHandler::isAuthNonceValid(const QByteArray &nonce, const QString &userName) const +bool Server_ProtocolHandler::isAuthNonceValid(const QByteArray &nonce) const { - // secsTo is signed, so a wall-clock step backwards (NTP correction, VM resume) - // must not make the elapsed time negative and re-validate an old nonce. - const qint64 elapsed = authNonceCreated.secsTo(QDateTime::currentDateTimeUtc()); - return !authNonce.isEmpty() && authNonce == nonce && authNonceUser == userName && authNonceCreated.isValid() && - elapsed >= 0 && elapsed < kAuthNonceLifetimeSeconds; + return !authNonce.isEmpty() && authNonce == nonce && authNonceCreated.secsTo(QDateTime::currentDateTimeUtc()) < 60; } void Server_ProtocolHandler::clearAuthNonce() { authNonce.clear(); - authNonceUser.clear(); } // This function must only be called from the thread this object lives in. diff --git a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h index f27bbb73f..c4917e845 100644 --- a/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h +++ b/libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.h @@ -58,7 +58,6 @@ protected: bool acceptsRoomListChanges; bool idleClientWarningSent; QByteArray authNonce; - QString authNonceUser; QDateTime authNonceCreated; virtual void logDebugMessage(const QString & /* message */) { @@ -129,10 +128,10 @@ public: return databaseInterface; } - /** @brief Store a fresh challenge nonce bound to @p userName for the next challenge-response login attempt. */ - void setAuthNonce(const QByteArray &nonce, const QString &userName); - /** @brief True if nonce matches the pending one, was issued for @p userName, and is less than 60 seconds old. */ - bool isAuthNonceValid(const QByteArray &nonce, const QString &userName) const; + /** @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(); diff --git a/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp b/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp index d98fe270c..8ae63fc81 100644 --- a/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/servers_settings.cpp @@ -151,11 +151,6 @@ void ServersSettings::setServerPassword(const QString &saveName, const QString & const int index = getPrevioushostindex(saveName); if (index >= 0) { setValue(password, QString("password%1").arg(index), "server", "server_details"); - } else { - // A credential write that silently no-ops is hard to diagnose from a - // bug report; surface a mismatched profile name instead. - qCWarning(ServersSettingsLog) << "setServerPassword() could not find profile:" << saveName - << "- password not saved"; } } diff --git a/servatrice/migrations/servatrice_0036_to_0037.sql b/servatrice/migrations/servatrice_0036_to_0037.sql index 6b814da7f..de3e09987 100644 --- a/servatrice/migrations/servatrice_0036_to_0037.sql +++ b/servatrice/migrations/servatrice_0036_to_0037.sql @@ -1,9 +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 because --- widening a CHAR requires a table rebuild, which ALGORITHM=INSTANT cannot perform — dropping the --- clause lets the server pick a suitable algorithm (and any row-format change is avoided anyway). +-- 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; +UPDATE cockatrice_schema_version SET version=37 WHERE version=36; \ No newline at end of file diff --git a/servatrice/src/servatrice_database_interface.cpp b/servatrice/src/servatrice_database_interface.cpp index f5c8907a3..caa3478c8 100644 --- a/servatrice/src/servatrice_database_interface.cpp +++ b/servatrice/src/servatrice_database_interface.cpp @@ -357,13 +357,6 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot return UserIsInactive; } - // Fail closed on an absent stored credential: an empty key would - // otherwise authenticate anyone who can compute HMAC("", nonce). - if (correctPasswordSha512.isEmpty()) { - qCWarning(DatabaseInterfaceLog) << "Login denied: empty stored credential"; - return NotLoggedIn; - } - if (password.startsWith("$challenge$")) { // Challenge-response login: verify HMAC(stored_key, nonce) without // ever transmitting the stored credential or password hash. @@ -373,7 +366,7 @@ AuthenticationResult Servatrice_DatabaseInterface::checkUserPassword(Server_Prot } const QByteArray nonce = QByteArray::fromBase64(parts.at(2).toUtf8()); const QByteArray response = QByteArray::fromBase64(parts.at(3).toUtf8()); - if (nonce.isEmpty() || response.isEmpty() || !handler->isAuthNonceValid(nonce, user)) { + if (nonce.isEmpty() || response.isEmpty() || !handler->isAuthNonceValid(nonce)) { return NotLoggedIn; } diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index 926fece31..36a3e5766 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -2524,16 +2524,8 @@ bool AbstractServerSocketInterface::tooManyRegistrationAttempts(const QString &i bool AbstractServerSocketInterface::acceptsCredentialFormat(bool passwordNeedsHash, const QString &password) const { - // An empty credential must never reach the database: it would be accepted - // as a legacy format and stored as '' (fail-open on login, see the empty - // stored-credential guard in Servatrice_DatabaseInterface). - if (password.isEmpty()) { - return false; - } - // "scryptFormat" means the client sent a derived verifier rather than a - // password to hash ourselves. parsePasswordVerifier enforces the sane-cost - // clamp, so nothing starting with '$' reaches the database unparsed. - const bool scryptFormat = !passwordNeedsHash && PasswordHasher::parsePasswordVerifier(password).isValid; + // "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 @@ -2542,8 +2534,7 @@ bool AbstractServerSocketInterface::acceptsCredentialFormat(bool passwordNeedsHa if (servatrice->getAuthenticationStrictness() == Servatrice::AuthenticationStrict) { return scryptFormat; } - // legacy and mixed accept a valid scrypt verifier or a genuine legacy salt+hash. - return scryptFormat || PasswordHasher::isLegacyFormat(password); + return true; // legacy and mixed accept either format } Response::ResponseCode AbstractServerSocketInterface::cmdActivateAccount(const Command_Activate &cmd, @@ -3106,7 +3097,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con // served the legacy salt, since legacy mode only governs what NEW credentials are accepted. if (servatrice->getAuthenticationStrictness() != Servatrice::AuthenticationLegacy) { const QByteArray nonce = CryptoUtil::randomBytes(32); - setAuthNonce(nonce, userName); + setAuthNonce(nonce); re->set_nonce(nonce.constData(), nonce.size()); } } else { @@ -3122,7 +3113,7 @@ Response::ResponseCode AbstractServerSocketInterface::cmdRequestPasswordSalt(con re->set_needs_migration(false); // scrypt rows are served challenge-response in every mode so migrated accounts never lock out. const QByteArray nonce = CryptoUtil::randomBytes(32); - setAuthNonce(nonce, userName); + setAuthNonce(nonce); re->set_nonce(nonce.constData(), nonce.size()); }