From 83b02e3a1ac15afdb25774f128a9de8bd170e4de Mon Sep 17 00:00:00 2001 From: SFENCE Date: Mon, 1 Jan 2024 00:03:40 +0100 Subject: [PATCH] Change the way how password is stored in C++ engine. --- src/client/CMakeLists.txt | 1 + src/client/client.cpp | 52 +++++--------- src/client/client.h | 8 +-- src/client/clientauth.cpp | 106 ++++++++++++++++++++++++++++ src/client/clientauth.h | 58 +++++++++++++++ src/client/clientlauncher.cpp | 3 + src/client/game.cpp | 13 ++-- src/client/game.h | 2 +- src/gameparams.h | 6 ++ src/network/clientpackethandler.cpp | 17 +++-- src/util/auth.cpp | 25 +++++++ src/util/auth.h | 3 + 12 files changed, 242 insertions(+), 52 deletions(-) create mode 100644 src/client/clientauth.cpp create mode 100644 src/client/clientauth.h diff --git a/src/client/CMakeLists.txt b/src/client/CMakeLists.txt index d451d0911..e59d45553 100644 --- a/src/client/CMakeLists.txt +++ b/src/client/CMakeLists.txt @@ -38,6 +38,7 @@ set(client_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/activeobjectmgr.cpp ${CMAKE_CURRENT_SOURCE_DIR}/camera.cpp ${CMAKE_CURRENT_SOURCE_DIR}/client.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/clientauth.cpp ${CMAKE_CURRENT_SOURCE_DIR}/clientenvironment.cpp ${CMAKE_CURRENT_SOURCE_DIR}/clientlauncher.cpp ${CMAKE_CURRENT_SOURCE_DIR}/clientmap.cpp diff --git a/src/client/client.cpp b/src/client/client.cpp index ca3f91e78..b2759be1d 100644 --- a/src/client/client.cpp +++ b/src/client/client.cpp @@ -97,7 +97,7 @@ void PacketCounter::print(std::ostream &o) const */ Client::Client( - const char *playername, + const std::string &playername, const std::string &password, MapDrawControl &control, IWritableTextureSource *tsrc, @@ -126,7 +126,7 @@ Client::Client( m_allow_login_or_register(allow_login_or_register), m_server_ser_ver(SER_FMT_VER_INVALID), m_last_chat_message_sent(time(NULL)), - m_password(password), + m_auth(playername, password), m_chosen_auth_mech(AUTH_MECHANISM_NONE), m_media_downloader(new ClientMediaDownloader()), m_state(LC_Created), @@ -134,7 +134,7 @@ Client::Client( m_modchannel_mgr(new ModChannelMgr()) { // Add local player - m_env.setLocalPlayer(new LocalPlayer(this, playername)); + m_env.setLocalPlayer(new LocalPlayer(this, playername.c_str())); // Make the mod storage database and begin the save for later m_mod_storage_database = @@ -1113,20 +1113,7 @@ void Client::interact(InteractAction action, const PointedThing& pointed) void Client::deleteAuthData() { - if (!m_auth_data) - return; - - switch (m_chosen_auth_mech) { - case AUTH_MECHANISM_FIRST_SRP: - break; - case AUTH_MECHANISM_SRP: - case AUTH_MECHANISM_LEGACY_PASSWORD: - srp_user_delete((SRPUser *) m_auth_data); - m_auth_data = NULL; - break; - case AUTH_MECHANISM_NONE: - break; - } + m_auth.clear(); m_chosen_auth_mech = AUTH_MECHANISM_NONE; } @@ -1168,36 +1155,34 @@ void Client::startAuth(AuthMechanism chosen_auth_mechanism) switch (chosen_auth_mechanism) { case AUTH_MECHANISM_FIRST_SRP: { // send srp verifier to server - std::string verifier; - std::string salt; - generate_srp_verifier_and_salt(playername, m_password, - &verifier, &salt); + const std::string &verifier = m_auth.getSrpVerifier(); + const std::string &salt = m_auth.getSrpSalt(); NetworkPacket resp_pkt(TOSERVER_FIRST_SRP, 0); - resp_pkt << salt << verifier << (u8)((m_password.empty()) ? 1 : 0); + resp_pkt << salt << verifier << (u8)((m_auth.getIsEmpty()) ? 1 : 0); Send(&resp_pkt); break; } case AUTH_MECHANISM_SRP: case AUTH_MECHANISM_LEGACY_PASSWORD: { - u8 based_on = 1; + u8 based_on; + void * auth_data; if (chosen_auth_mechanism == AUTH_MECHANISM_LEGACY_PASSWORD) { - m_password = translate_password(playername, m_password); based_on = 0; + auth_data = m_auth.getLegacyAuthData(); + } + else { + based_on = 1; + auth_data = m_auth.getSrpAuthData(); } - std::string playername_u = lowercase(playername); - m_auth_data = srp_user_new(SRP_SHA256, SRP_NG_2048, - playername.c_str(), playername_u.c_str(), - (const unsigned char *) m_password.c_str(), - m_password.length(), NULL, NULL); char *bytes_A = 0; size_t len_A = 0; SRP_Result res = srp_user_start_authentication( - (struct SRPUser *) m_auth_data, NULL, NULL, 0, - (unsigned char **) &bytes_A, &len_A); + static_cast(auth_data), NULL, NULL, 0, + reinterpret_cast(&bytes_A), &len_A); FATAL_ERROR_IF(res != SRP_OK, "Creating local SRP user failed."); NetworkPacket resp_pkt(TOSERVER_SRP_BYTES_A, 0); @@ -1357,8 +1342,9 @@ void Client::sendChangePassword(const std::string &oldpassword, return; // get into sudo mode and then send new password to server - m_password = oldpassword; - m_new_password = newpassword; + std::string playername = m_env.getLocalPlayer()->getName(); + m_auth.applyPassword(playername, oldpassword); + m_new_auth.applyPassword(playername, newpassword); startAuth(choseAuthMech(m_sudo_auth_methods)); } diff --git a/src/client/client.h b/src/client/client.h index b2ff9a0da..4db5f4b9b 100644 --- a/src/client/client.h +++ b/src/client/client.h @@ -36,6 +36,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "network/peerhandler.h" #include "gameparams.h" #include "clientdynamicinfo.h" +#include "clientauth.h" #include "util/numeric.h" #ifdef SERVER @@ -123,7 +124,7 @@ public: */ Client( - const char *playername, + const std::string &playername, const std::string &password, MapDrawControl &control, IWritableTextureSource *tsrc, @@ -533,12 +534,11 @@ private: // Auth data std::string m_playername; - std::string m_password; + ClientAuth m_auth; // If set, this will be sent (and cleared) upon a TOCLIENT_ACCEPT_SUDO_MODE - std::string m_new_password; + ClientAuth m_new_auth; // Usable by auth mechanisms. AuthMechanism m_chosen_auth_mech; - void *m_auth_data = nullptr; bool m_access_denied = false; bool m_access_denied_reconnect = false; diff --git a/src/client/clientauth.cpp b/src/client/clientauth.cpp new file mode 100644 index 000000000..b0ff76306 --- /dev/null +++ b/src/client/clientauth.cpp @@ -0,0 +1,106 @@ +/* +Minetest +Copyright (C) 2024 SFENCE, + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#include "clientauth.h" +#include "util/auth.h" +#include "util/srp.h" + +ClientAuth::ClientAuth() : + m_is_empty(true), + m_srp_verifier(""), + m_srp_salt("") +{ +} + +ClientAuth::ClientAuth(const std::string &player_name, const std::string &password) +{ + applyPassword(player_name, password); +} + +ClientAuth::~ClientAuth() +{ + clear(); +} + +ClientAuth &ClientAuth::operator=(ClientAuth &&other) +{ + clear(); + + m_is_empty = other.m_is_empty; + + m_srp_verifier = other.m_srp_verifier; + m_srp_salt = other.m_srp_salt; + + m_legacy_auth_data = other.m_legacy_auth_data; + m_srp_auth_data = other.m_srp_auth_data; + + other.m_legacy_auth_data = nullptr; + other.m_srp_auth_data = nullptr; + + other.clear(); + + return *this; +} + +void ClientAuth::applyPassword(const std::string &player_name, const std::string &password) +{ + clear(); + // AUTH_MECHANISM_FIRST_SRP + generate_srp_verifier_and_salt(player_name, password, &m_srp_verifier, &m_srp_salt); + m_is_empty = password.empty(); + + std::string player_name_u = lowercase(player_name); + // AUTH_MECHANISM_SRP + m_srp_auth_data = srp_user_new(SRP_SHA256, SRP_NG_2048, + player_name.c_str(), player_name_u.c_str(), + reinterpret_cast(password.c_str()), + password.length(), NULL, NULL); + // AUTH_MECHANISM_LEGACY_PASSWORD + std::string translated = translate_password(player_name, password); + m_legacy_auth_data = srp_user_new(SRP_SHA256, SRP_NG_2048, + player_name.c_str(), player_name_u.c_str(), + reinterpret_cast(translated.c_str()), + translated.length(), NULL, NULL); +} + +void * ClientAuth::getAuthData(AuthMechanism chosen_auth_mech) const +{ + switch (chosen_auth_mech) { + case AUTH_MECHANISM_LEGACY_PASSWORD: + return m_legacy_auth_data; + case AUTH_MECHANISM_SRP: + return m_srp_auth_data; + default: + return nullptr; + } +} + +void ClientAuth::clear() +{ + if (m_legacy_auth_data != nullptr) { + srp_user_delete(m_legacy_auth_data); + m_legacy_auth_data = nullptr; + } + if (m_srp_auth_data != nullptr) { + srp_user_delete(m_srp_auth_data); + m_srp_auth_data = nullptr; + } + m_srp_verifier.clear(); + m_srp_salt.clear(); +} diff --git a/src/client/clientauth.h b/src/client/clientauth.h new file mode 100644 index 000000000..6cc783df2 --- /dev/null +++ b/src/client/clientauth.h @@ -0,0 +1,58 @@ +/* +Minetest +Copyright (C) 2024 SFENCE, + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#pragma once + +#include +#include "network/networkprotocol.h" +#include "util/basic_macros.h" + +struct SRPUser; + +class ClientAuth +{ +public: + ClientAuth(); + ClientAuth(const std::string &player_name, const std::string &password); + + ~ClientAuth(); + DISABLE_CLASS_COPY(ClientAuth); + + ClientAuth(ClientAuth &&other) { *this = std::move(other); } + ClientAuth &operator=(ClientAuth &&other); + + void applyPassword(const std::string &player_name, const std::string &password); + + bool getIsEmpty() const { return m_is_empty; } + const std::string &getSrpVerifier() const { return m_srp_verifier; } + const std::string &getSrpSalt() const { return m_srp_salt; } + void * getLegacyAuthData() const { return m_legacy_auth_data; } + void * getSrpAuthData() const { return m_srp_auth_data; } + void * getAuthData(AuthMechanism chosen_auth_mech) const; + + void clear(); +private: + bool m_is_empty; + + std::string m_srp_verifier; + std::string m_srp_salt; + + SRPUser *m_legacy_auth_data = nullptr; + SRPUser *m_srp_auth_data = nullptr; +}; diff --git a/src/client/clientlauncher.cpp b/src/client/clientlauncher.cpp index 56beba927..7fca85672 100644 --- a/src/client/clientlauncher.cpp +++ b/src/client/clientlauncher.cpp @@ -449,6 +449,9 @@ bool ClientLauncher::launch_game(std::string &error_message, server_name = menudata.servername; server_description = menudata.serverdescription; + /* make sure that password will not stay somewhere in memory */ + clear_string(menudata.password); + start_data.local_server = !menudata.simple_singleplayer_mode && start_data.address.empty(); } else { diff --git a/src/client/game.cpp b/src/client/game.cpp index d89b6639c..f6d26ea2f 100644 --- a/src/client/game.cpp +++ b/src/client/game.cpp @@ -683,7 +683,7 @@ public: bool startup(bool *kill, InputHandler *input, RenderingEngine *rendering_engine, - const GameStartData &game_params, + GameStartData &game_params, std::string &error_message, bool *reconnect, ChatBackend *chat_backend); @@ -1072,7 +1072,7 @@ Game::~Game() bool Game::startup(bool *kill, InputHandler *input, RenderingEngine *rendering_engine, - const GameStartData &start_data, + GameStartData &start_data, std::string &error_message, bool *reconnect, ChatBackend *chat_backend) @@ -1114,8 +1114,11 @@ bool Game::startup(bool *kill, start_data.socket_port, start_data.game_spec)) return false; - if (!createClient(start_data)) + if (!createClient(start_data)) { + start_data.erasePassword(); return false; + } + start_data.erasePassword(); m_rendering_engine->initialize(client, hud); @@ -1618,7 +1621,7 @@ bool Game::connectToServer(const GameStartData &start_data, try { - client = new Client(start_data.name.c_str(), + client = new Client(start_data.name, start_data.password, *draw_control, texture_src, shader_src, itemdef_manager, nodedef_manager, sound_manager.get(), eventmgr, @@ -4526,7 +4529,7 @@ void Game::showPauseMenu() void the_game(bool *kill, InputHandler *input, RenderingEngine *rendering_engine, - const GameStartData &start_data, + GameStartData &start_data, std::string &error_message, ChatBackend &chat_backend, bool *reconnect_requested) // Used for local game diff --git a/src/client/game.h b/src/client/game.h index 0282e5ea9..76ba3a805 100644 --- a/src/client/game.h +++ b/src/client/game.h @@ -53,7 +53,7 @@ struct CameraOrientation { void the_game(bool *kill, InputHandler *input, RenderingEngine *rendering_engine, - const GameStartData &start_data, + GameStartData &start_data, std::string &error_message, ChatBackend &chat_backend, bool *reconnect_requested); diff --git a/src/gameparams.h b/src/gameparams.h index b138f8771..a0d14a1eb 100644 --- a/src/gameparams.h +++ b/src/gameparams.h @@ -21,6 +21,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "irrlichttypes.h" #include "content/subgames.h" +#include "util/auth.h" // Information provided from "main" struct GameParams @@ -55,4 +56,9 @@ struct GameStartData : GameParams // "world_path" must be kept in sync! WorldSpec world_spec; + + void erasePassword() { + /* make sure that password will not stay somewhere in memory */ + clear_string(password); + } }; diff --git a/src/network/clientpackethandler.cpp b/src/network/clientpackethandler.cpp index 90f2bed5b..220490589 100644 --- a/src/network/clientpackethandler.cpp +++ b/src/network/clientpackethandler.cpp @@ -97,8 +97,7 @@ void Client::handleCommand_Hello(NetworkPacket* pkt) << "(chosen_mech=" << m_chosen_auth_mech << ")." << std::endl; if (m_chosen_auth_mech == AUTH_MECHANISM_SRP || m_chosen_auth_mech == AUTH_MECHANISM_LEGACY_PASSWORD) { - srp_user_delete((SRPUser *) m_auth_data); - m_auth_data = 0; + m_auth.clear(); } } @@ -166,9 +165,7 @@ void Client::handleCommand_AuthAccept(NetworkPacket* pkt) void Client::handleCommand_AcceptSudoMode(NetworkPacket* pkt) { - deleteAuthData(); - - m_password = m_new_password; + m_auth = std::move(m_new_auth); verbosestream << "Client: Received TOCLIENT_ACCEPT_SUDO_MODE." << std::endl; @@ -195,6 +192,8 @@ void Client::handleCommand_AccessDenied(NetworkPacket* pkt) m_access_denied = true; m_access_denied_reason = "Unknown"; + deleteAuthData(); + if (pkt->getCommand() != TOCLIENT_ACCESS_DENIED) { // Legacy code from 0.4.12 and older but is still used // in some places of the server code @@ -1570,16 +1569,16 @@ void Client::handleCommand_SrpBytesSandB(NetworkPacket* pkt) char *bytes_M = 0; size_t len_M = 0; - SRPUser *usr = (SRPUser *) m_auth_data; + SRPUser *usr = static_cast(m_auth.getAuthData(m_chosen_auth_mech)); std::string s; std::string B; *pkt >> s >> B; infostream << "Client: Received TOCLIENT_SRP_BYTES_S_B." << std::endl; - srp_user_process_challenge(usr, (const unsigned char *) s.c_str(), s.size(), - (const unsigned char *) B.c_str(), B.size(), - (unsigned char **) &bytes_M, &len_M); + srp_user_process_challenge(usr, reinterpret_cast(s.c_str()), s.size(), + reinterpret_cast(B.c_str()), B.size(), + reinterpret_cast(&bytes_M), &len_M); if ( !bytes_M ) { errorstream << "Client: SRP-6a S_B safety check violation!" << std::endl; diff --git a/src/util/auth.cpp b/src/util/auth.cpp index d7d1da280..d139142f1 100644 --- a/src/util/auth.cpp +++ b/src/util/auth.cpp @@ -17,6 +17,9 @@ with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +// enable include of memset_s function +#define __STDC_WANT_LIB_EXT1__ 1 + #include #include #include "auth.h" @@ -26,6 +29,10 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "util/string.h" #include "debug.h" +#ifdef _WIN32 + #include +#endif + // Get an sha-1 hash of the player's name combined with // the password entered. That's what the server uses as // their password. (Exception : if the password field is @@ -134,3 +141,21 @@ bool decode_srp_verifier_and_salt(const std::string &encoded, return true; } + +/// Override every character before clearing +void clear_string(std::string &text) +{ + #ifdef __STDC_LIB_EXT1__ + memset_s((void *)text.data(), text.size(), '0', text.size()); + #elif _WIN32 + SecureZeroMemory((void *)text.data(), text.size()); + #else + volatile char *ch = (char *)text.data(); + size_t n = text.size(); + for (;n>0;n--) { + *ch = 0; + ch++; + } + #endif + text.clear(); +} diff --git a/src/util/auth.h b/src/util/auth.h index ba827f322..e6741a0f7 100644 --- a/src/util/auth.h +++ b/src/util/auth.h @@ -45,3 +45,6 @@ std::string encode_srp_verifier(const std::string &verifier, /// and salt components. bool decode_srp_verifier_and_salt(const std::string &encoded, std::string *verifier, std::string *salt); + +/// Override every character before clearing +void clear_string(std::string &text);