From ac39ffdcd3dbc5679bc93b9ee9b1569c5154794c Mon Sep 17 00:00:00 2001 From: Pentarctagon Date: Wed, 19 May 2021 19:48:24 -0500 Subject: [PATCH] Hash passwords server-side instead of client-side. This removes the need for the Wesnoth client to need to know how to hash the password to match what is expected by the forum database. --- src/commandline_options.cpp | 4 + src/commandline_options.hpp | 6 ++ src/game_config.cpp | 2 + src/game_config.hpp | 2 + src/game_initialization/multiplayer.cpp | 71 ++++----------- src/multiplayer_error_codes.hpp | 31 +++---- src/server/wesnothd/server.cpp | 114 +++++++++++++++++------- src/server/wesnothd/server.hpp | 13 ++- src/wesnoth.cpp | 4 + 9 files changed, 142 insertions(+), 105 deletions(-) diff --git a/src/commandline_options.cpp b/src/commandline_options.cpp index 821288dbad1..c86d79abbfc 100644 --- a/src/commandline_options.cpp +++ b/src/commandline_options.cpp @@ -82,6 +82,7 @@ commandline_options::commandline_options(const std::vector& args) , debug(false) , debug_lua(false) , strict_lua(false) + , allow_insecure(false) #ifdef DEBUG_WINDOW_LAYOUT_GRAPHS , debug_dot_domain() , debug_dot_level() @@ -185,6 +186,7 @@ commandline_options::commandline_options(const std::vector& args) ("debug,d", "enables additional command mode options in-game.") ("debug-lua", "enables some Lua debugging mechanisms") ("strict-lua", "disallow deprecated Lua API calls") + ("allow-insecure", "Allows sending a plaintext password over an unencrypted connection. Should only ever be used for local testing.") #ifdef DEBUG_WINDOW_LAYOUT_GRAPHS ("debug-dot-level", po::value(), "sets the level of the debug dot files. should be a comma separated list of levels. These files are used for debugging the widgets especially the for the layout engine. When enabled the engine will produce dot files which can be converted to images with the dot tool. Available levels: size (generate the size info of the widget), state (generate the state info of the widget).") ("debug-dot-domain", po::value(), "sets the domain of the debug dot files. should be a comma separated list of domains. See --debug-dot-level for more info. Available domains: show (generate the data when the dialog is about to be shown), layout (generate the data during the layout phase - might result in multiple files). The data can also be generated when the F12 is pressed in a dialog.") @@ -350,6 +352,8 @@ commandline_options::commandline_options(const std::vector& args) debug_lua = true; if (vm.count("strict-lua")) strict_lua = true; + if (vm.count("allow-insecure")) + allow_insecure = true; #ifdef DEBUG_WINDOW_LAYOUT_GRAPHS if (vm.count("debug-dot-domain")) { debug_dot_domain = vm["debug-dot-domain"].as(); diff --git a/src/commandline_options.hpp b/src/commandline_options.hpp index c9072ea868a..ae61a1e88f7 100644 --- a/src/commandline_options.hpp +++ b/src/commandline_options.hpp @@ -76,6 +76,12 @@ public: bool debug_lua; /** True if --strict-lua was given in the commandline. Disallows use of deprecated APIs. */ bool strict_lua; + /** + * True if --allow-insecure was given in the commandline. + * Allows sending a plaintext password over an unencrypted connection. + * Should only ever be used for local testing. + */ + bool allow_insecure; #ifdef DEBUG_WINDOW_LAYOUT_GRAPHS /** Non-empty if --debug-dot-domain was given on the command line. */ std::optional debug_dot_domain; diff --git a/src/game_config.cpp b/src/game_config.cpp index 43c7d9be55b..21fb19a593a 100644 --- a/src/game_config.cpp +++ b/src/game_config.cpp @@ -92,6 +92,8 @@ const std::size_t max_loop = 65536; std::vector server_list; +bool allow_insecure = false; + // // Gamestate flags // diff --git a/src/game_config.hpp b/src/game_config.hpp index e3f03af766e..da4e1968811 100644 --- a/src/game_config.hpp +++ b/src/game_config.hpp @@ -62,6 +62,8 @@ namespace game_config extern bool debug_lua, strict_lua, editor, ignore_replay_errors, mp_debug, exit_at_end, no_delay, disable_autosave, no_addons; + extern bool allow_insecure; + extern const bool& debug; void set_debug(bool new_debug); diff --git a/src/game_initialization/multiplayer.cpp b/src/game_initialization/multiplayer.cpp index df3ea7825cc..38c0d739e3b 100644 --- a/src/game_initialization/multiplayer.cpp +++ b/src/game_initialization/multiplayer.cpp @@ -31,7 +31,6 @@ #include "gui/dialogs/multiplayer/mp_join_game.hpp" #include "gui/dialogs/multiplayer/mp_login.hpp" #include "gui/dialogs/multiplayer/mp_staging.hpp" -#include "hash.hpp" #include "log.hpp" #include "map_settings.hpp" #include "multiplayer_error_codes.hpp" @@ -361,58 +360,20 @@ std::unique_ptr mp_manager::open_connection(std::string hos ? (gui2::show_message(_("Confirm"), (*error)["message"], gui2::dialogs::message::ok_cancel_buttons) == gui2::retval::CANCEL) : false; - const bool is_pw_request = !((*error)["password_request"].empty()) && !(password.empty()); + // If: + // * the server asked for a password + // * the password isn't empty + // * the user didn't press Cancel + // * the connection is secure or the client was started with the option to use insecure connections + // send the password to the server + // otherwise go directly to the username/password dialog + if(!(*error)["password_request"].empty() && !password.empty() && !fall_through && (conn->using_tls() || game_config::allow_insecure)) { + // the possible cases here are that either: + // 1) TLS encryption is enabled, thus sending the plaintext password is still secure + // 2) TLS encryption is not enabled, in which case the server should not be requesting a password in the first place + // 3) This is being used for local testing/development, so using an insecure connection is enabled manually - // If the server asks for a password, provide one if we can - // or request a password reminder. - // Otherwise or if the user pressed 'cancel' in the confirmation dialog - // above go directly to the username/password dialog - if(is_pw_request && !fall_through) { - if((*error)["phpbb_encryption"].to_bool()) { - // Apparently HTML key-characters are passed to the hashing functions of phpbb in this escaped form. - // I will do closer investigations on this, for now let's just hope these are all of them. - - // Note: we must obviously replace '&' first, I wasted some time before I figured that out... :) - for(std::string::size_type pos = 0; (pos = password.find('&', pos)) != std::string::npos; ++pos) - password.replace(pos, 1, "&"); - for(std::string::size_type pos = 0; (pos = password.find('\"', pos)) != std::string::npos; ++pos) - password.replace(pos, 1, """); - for(std::string::size_type pos = 0; (pos = password.find('<', pos)) != std::string::npos; ++pos) - password.replace(pos, 1, "<"); - for(std::string::size_type pos = 0; (pos = password.find('>', pos)) != std::string::npos; ++pos) - password.replace(pos, 1, ">"); - - const std::string salt = (*error)["salt"]; - if(salt.length() < 12) { - throw wesnothd_error(_("Bad data received from server")); - } - - if(utils::md5::is_valid_prefix(salt)) { - sp["password"] = utils::md5( - utils::md5(password, utils::md5::get_salt(salt), utils::md5::get_iteration_count(salt)).base64_digest(), - salt.substr(12, 8) - ).base64_digest(); - } else if(utils::bcrypt::is_valid_prefix(salt)) { - try { - auto bcrypt_salt = utils::bcrypt::from_salted_salt(salt); - auto hash = utils::bcrypt::hash_pw(password, bcrypt_salt); - - const std::string outer_salt = salt.substr(bcrypt_salt.iteration_count_delim_pos + 23); - if(outer_salt.size() != 32) { - throw utils::hash_error("salt wrong size"); - } - - sp["password"] = utils::md5(hash.base64_digest(), outer_salt).base64_digest(); - } catch(const utils::hash_error& err) { - ERR_MP << "bcrypt hash failed: " << err.what() << std::endl; - throw wesnothd_error(_("Bad data received from server")); - } - } else { - throw wesnothd_error(_("Bad data received from server")); - } - } else { - sp["password"] = password; - } + sp["password"] = password; // Once again send our request... conn->send_data(response); @@ -442,7 +403,9 @@ std::unique_ptr mp_manager::open_connection(std::string hos const std::string ec = (*error)["error_code"]; - if(ec == MP_MUST_LOGIN) { + if(!(*error)["password_request"].empty() && !conn->using_tls() && !game_config::allow_insecure) { + error_message = _("The remote server requested a password while using an insecure connection."); + } else if(ec == MP_MUST_LOGIN) { error_message = _("You must login first."); } else if(ec == MP_NAME_TAKEN_ERROR) { error_message = VGETTEXT("The nickname ‘$nick’ is already taken.", i18n_symbols); @@ -487,6 +450,8 @@ std::unique_ptr mp_manager::open_connection(std::string hos error_message = _("The password you provided was incorrect."); } else if(ec == MP_TOO_MANY_ATTEMPTS_ERROR) { error_message = _("You have made too many login attempts."); + } else if(ec == MP_HASHING_PASSWORD_FAILED) { + error_message = _("Password hashing failed."); } else { error_message = (*error)["message"].str(); } diff --git a/src/multiplayer_error_codes.hpp b/src/multiplayer_error_codes.hpp index 8c387ff9e90..a2ab0fb8c8c 100644 --- a/src/multiplayer_error_codes.hpp +++ b/src/multiplayer_error_codes.hpp @@ -19,19 +19,20 @@ #pragma once -#define MP_MUST_LOGIN "100" -#define MP_NAME_TAKEN_ERROR "101" -#define MP_INVALID_CHARS_IN_NAME_ERROR "102" -#define MP_NAME_TOO_LONG_ERROR "103" -#define MP_NAME_RESERVED_ERROR "104" -#define MP_NAME_UNREGISTERED_ERROR "105" -#define MP_NAME_INACTIVE_WARNING "106" -#define MP_NAME_AUTH_BAN_USER_ERROR "107" -#define MP_NAME_AUTH_BAN_IP_ERROR "108" -#define MP_NAME_AUTH_BAN_EMAIL_ERROR "109" +#define MP_MUST_LOGIN "100" +#define MP_NAME_TAKEN_ERROR "101" +#define MP_INVALID_CHARS_IN_NAME_ERROR "102" +#define MP_NAME_TOO_LONG_ERROR "103" +#define MP_NAME_RESERVED_ERROR "104" +#define MP_NAME_UNREGISTERED_ERROR "105" +#define MP_NAME_INACTIVE_WARNING "106" +#define MP_NAME_AUTH_BAN_USER_ERROR "107" +#define MP_NAME_AUTH_BAN_IP_ERROR "108" +#define MP_NAME_AUTH_BAN_EMAIL_ERROR "109" -#define MP_PASSWORD_REQUEST "200" -#define MP_PASSWORD_REQUEST_FOR_LOGGED_IN_NAME "201" -#define MP_NO_SEED_ERROR "202" -#define MP_INCORRECT_PASSWORD_ERROR "203" -#define MP_TOO_MANY_ATTEMPTS_ERROR "204" +#define MP_PASSWORD_REQUEST "200" +#define MP_PASSWORD_REQUEST_FOR_LOGGED_IN_NAME "201" +#define MP_NO_SEED_ERROR "202" +#define MP_INCORRECT_PASSWORD_ERROR "203" +#define MP_TOO_MANY_ATTEMPTS_ERROR "204" +#define MP_HASHING_PASSWORD_FAILED "205" diff --git a/src/server/wesnothd/server.cpp b/src/server/wesnothd/server.cpp index db31ff90e8f..7cb6af51222 100644 --- a/src/server/wesnothd/server.cpp +++ b/src/server/wesnothd/server.cpp @@ -22,13 +22,13 @@ #include "config.hpp" #include "filesystem.hpp" #include "game_config.hpp" +#include "hash.hpp" #include "log.hpp" #include "multiplayer_error_codes.hpp" #include "serialization/parser.hpp" #include "serialization/preprocessor.hpp" #include "serialization/string_utils.hpp" #include "serialization/unicode.hpp" -#include #include "utils/general.hpp" #include "utils/iterable_pair.hpp" #include "game_version.hpp" @@ -53,12 +53,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include static lg::log_domain log_server("server"); @@ -880,17 +882,14 @@ template bool server::authenticate( { // Current login procedure for registered nicks is: // - Client asks to log in with a particular nick - // - Server sends client random nonce plus some info - // generated from the original hash that is required to - // regenerate the hash - // - Client generates hash for the user provided password - // and mixes it with the received random nonce - // - Server received password hash hashed with the nonce, - // applies the nonce to the valid hash and compares the results + // - Server sends client a password request (if TLS/database support is enabled) + // - Client sends the plaintext password + // - Server receives plaintext password, hashes it, and compares it to the password in the forum database registered = false; if(user_handler_) { + const auto [hashed_password, nonce] = hash_password(password, username, socket); const bool exists = user_handler_->user_exists(username); // This name is registered but the account is not active @@ -903,34 +902,34 @@ template bool server::authenticate( // This name is registered and no password provided if(password.empty()) { if(!name_taken) { - send_password_request(socket, "The nickname '" + username + "' is registered on this server.", - username, MP_PASSWORD_REQUEST); + send_password_request(socket, "The nickname '" + username + "' is registered on this server.", MP_PASSWORD_REQUEST); } else { send_password_request(socket, "The nickname '" + username + "' is registered on this server." "\n\nWARNING: There is already a client using this username, " "logging in will cause that client to be kicked!", - username, MP_PASSWORD_REQUEST_FOR_LOGGED_IN_NAME, true + MP_PASSWORD_REQUEST_FOR_LOGGED_IN_NAME, true ); } return false; } - // A password (or hashed password) was provided, however - // there is no seed - if(seeds_[socket.get()].empty()) { - send_password_request(socket, "Please try again.", username, MP_NO_SEED_ERROR); + // A password was provided, however the generated nonce is empty for some reason + if(nonce.empty()) { + send_password_request(socket, "Please try again.", MP_NO_SEED_ERROR); + return false; + } + // hashing the password failed + // note: this could be due to other related problems other than *just* the hashing step failing + else if(hashed_password.empty()) { + async_send_error(socket, "Password hashing failed.", MP_HASHING_PASSWORD_FAILED); return false; } - // This name is registered and an incorrect password provided - else if(!(user_handler_->login(username, password, seeds_[socket.get()]))) { + else if(!(user_handler_->login(username, hashed_password, nonce))) { const std::time_t now = std::time(nullptr); - // Reset the random seed - seeds_.erase(socket.get()); - login_log login_ip { client_address(socket), 0, now }; auto i = std::find(failed_logins_.begin(), failed_logins_.end(), login_ip); @@ -960,7 +959,7 @@ template bool server::authenticate( async_send_error(socket, "You have made too many failed login attempts.", MP_TOO_MANY_ATTEMPTS_ERROR); } else { send_password_request(socket, - "The password you provided for the nickname '" + username + "' was incorrect.", username, + "The password you provided for the nickname '" + username + "' was incorrect.", MP_INCORRECT_PASSWORD_ERROR); } @@ -974,7 +973,6 @@ template bool server::authenticate( registered = true; // Reset the random seed - seeds_.erase(socket.get()); user_handler_->user_logged_in(username); } } @@ -982,38 +980,86 @@ template bool server::authenticate( return true; } -template void server::send_password_request(SocketPtr socket, - const std::string& msg, - const std::string& user, - const char* error_code, - bool force_confirmation) +template std::pair server::hash_password(const std::string& pw, const std::string& user, SocketPtr socket) { - std::string salt = user_handler_->extract_salt(user); + std::string plain_salt = user_handler_->extract_salt(user); + std::string password = pw; // If using crypt_blowfish, use 32 random Base64 characters, cryptographic-strength, 192 bits entropy // else (phppass, MD5, $H$), use 8 random integer digits, not secure, do not use, this is crap, 29.8 bits entropy - std::string nonce{(salt[1] == '2') + std::string nonce{(plain_salt[1] == '2') ? user_handler_->create_secure_nonce() : user_handler_->create_unsecure_nonce()}; - std::string password_challenge = salt + nonce; + std::string salt = plain_salt + nonce; if(salt.empty()) { async_send_error(socket, "Even though your nickname is registered on this server you " "cannot log in due to an error in the hashing algorithm. " "Logging into your forum account on https://forums.wesnoth.org " "may fix this problem."); - return; + return std::make_pair("", ""); } - seeds_[socket.get()] = nonce; + // Apparently HTML key-characters are passed to the hashing functions of phpbb in this escaped form. + // I will do closer investigations on this, for now let's just hope these are all of them. + // Note: we must obviously replace '&' first, I wasted some time before I figured that out... :) + for(std::string::size_type pos = 0; (pos = password.find('&', pos)) != std::string::npos; ++pos) { + password.replace(pos, 1, "&"); + } + for(std::string::size_type pos = 0; (pos = password.find('\"', pos)) != std::string::npos; ++pos) { + password.replace(pos, 1, """); + } + for(std::string::size_type pos = 0; (pos = password.find('<', pos)) != std::string::npos; ++pos) { + password.replace(pos, 1, "<"); + } + for(std::string::size_type pos = 0; (pos = password.find('>', pos)) != std::string::npos; ++pos) { + password.replace(pos, 1, ">"); + } + + if(salt.length() < 12) { + ERR_SERVER << "Bad salt found for user: " << user << std::endl; + return std::make_pair("", ""); + } + + if(utils::md5::is_valid_prefix(salt)) { + std::string md5_1 = utils::md5(password, utils::md5::get_salt(salt), utils::md5::get_iteration_count(salt)).base64_digest(); + std::string md5_2 = utils::md5(md5_1, salt.substr(12, 8)).base64_digest(); + return std::make_pair(md5_2, nonce); + } else if(utils::bcrypt::is_valid_prefix(salt)) { + try { + auto bcrypt_salt = utils::bcrypt::from_salted_salt(salt); + auto hash = utils::bcrypt::hash_pw(password, bcrypt_salt); + + const std::string outer_salt = salt.substr(bcrypt_salt.iteration_count_delim_pos + 23); + if(outer_salt.size() != 32) { + throw utils::hash_error("salt wrong size"); + } + + return std::make_pair( + utils::md5(hash.base64_digest(), outer_salt).base64_digest(), + nonce + ); + } catch(const utils::hash_error& err) { + ERR_SERVER << "bcrypt hash failed: " << err.what() << std::endl; + return std::make_pair("", ""); + } + } else { + ERR_SERVER << "Unable to determine how to hash the password for user: " << user << std::endl; + return std::make_pair("", ""); + } +} + +template void server::send_password_request(SocketPtr socket, + const std::string& msg, + const char* error_code, + bool force_confirmation) +{ simple_wml::document doc; simple_wml::node& e = doc.root().add_child("error"); e.set_attr_dup("message", msg.c_str()); e.set_attr("password_request", "yes"); - e.set_attr("phpbb_encryption", "yes"); - e.set_attr_dup("salt", password_challenge.c_str()); e.set_attr("force_confirmation", force_confirmation ? "yes" : "no"); if(*error_code != '\0') { diff --git a/src/server/wesnothd/server.hpp b/src/server/wesnothd/server.hpp index 8744f7ec53d..2e2e830516e 100644 --- a/src/server/wesnothd/server.hpp +++ b/src/server/wesnothd/server.hpp @@ -43,8 +43,7 @@ private: template void login_client(boost::asio::yield_context yield, SocketPtr socket); template bool is_login_allowed(SocketPtr socket, const simple_wml::node* const login, const std::string& username, bool& registered, bool& is_moderator); template bool authenticate(SocketPtr socket, const std::string& username, const std::string& password, bool name_taken, bool& registered); - template void send_password_request(SocketPtr socket, const std::string& msg, - const std::string& user, const char* error_code = "", bool force_confirmation = false); + template void send_password_request(SocketPtr socket, const std::string& msg, const char* error_code = "", bool force_confirmation = false); bool accepting_connections() const { return !graceful_restart; } template void handle_player(boost::asio::yield_context yield, SocketPtr socket, const player& player); @@ -59,6 +58,15 @@ private: void handle_join_game(player_iterator player, simple_wml::node& join); void disconnect_player(player_iterator player); void remove_player(player_iterator player); + /** + * Handles hashing the password provided by the player before comparing it to the hashed password in the forum database. + * + * @param pw The plaintext password. + * @param user The player attempting to log in. + * @param socket The socket the player is connected with. + * @return The hashed password, or empty if the password couldn't be hashed. + */ + template std::pair hash_password(const std::string& pw, const std::string& user, SocketPtr socket); public: template void send_server_message(SocketPtr socket, const std::string& message, const std::string& type); @@ -115,7 +123,6 @@ private: std::deque failed_logins_; std::unique_ptr user_handler_; - std::map seeds_; std::mt19937 die_; diff --git a/src/wesnoth.cpp b/src/wesnoth.cpp index 2368242cd1c..b3a7935508f 100644 --- a/src/wesnoth.cpp +++ b/src/wesnoth.cpp @@ -429,6 +429,10 @@ static int process_command_args(const commandline_options& cmdline_opts) game_config::debug_lua = true; } + if(cmdline_opts.allow_insecure) { + game_config::allow_insecure = true; + } + if(cmdline_opts.strict_lua) { game_config::strict_lua = true; }