widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Notabilis has proposed merging lp:~widelands-dev/widelands/net-pwd-security into lp:widelands.

Commit message:
Increasing password security by no longer storing and transmitting it in plaintext.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/net-pwd-security/+merge/340540

Increasing password security by using CHAP for transmitting the password instead of sending it in plaintext. With CHAP, a challenge (random value) is send to the connecting client and the client responds by sending back the cryptographic hash of the challenge concatenated with its secret password. A cryptographic hash function converts an input to a fixed length output while it is practically impossible to convert the output back to the input. Since the server also knows the password, it can also calculate the hash and can check the hashes whether they are equal. An attacker which captures the send hash can not do anything with it: Extracting the password is not possible and sending the hash to the server does not work since the server will send another challenge on next connect.

To reduce login time, moved TELL_IP functionality for transmitting the IPv4 address to the metaserver to an own thread.

Passwords are now stored as a cryptographic hash in the configuration file. For now, the plaintext password is left in the configuration file if it exists to allow parallel usage of build19 and trunk builds.

Also, removed "max. number of players" network command parameter when opening a game, which has long been obsolete.

This change should only affect new clients, build19 clients should still work with plaintext passwords. This branch should be merged parallel to the respective metaserver branch.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/net-pwd-security into lp:widelands.

=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2018-02-04 11:41:45 +0000
+++ src/network/internet_gaming.cc	2018-03-02 19:20:25 +0000
@@ -20,6 +20,7 @@
 #include "network/internet_gaming.h"
 
 #include <memory>
+#include <thread>
 
 #include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
@@ -159,7 +160,7 @@
 	s.string(clientname_);
 	s.string(build_id());
 	s.string(bool2str(reg_));
-	s.string(authenticator_);
+	s.string(reg_ ? "" : authenticator_);
 	net->send(s);
 
 	// Now let's see, whether the metaserver is answering
@@ -337,34 +338,72 @@
 }
 
 void InternetGaming::create_second_connection() {
-	NetAddress addr;
-	net->get_remote_address(&addr);
-	if (!addr.is_ipv6()) {
+	NetAddress addr_net;
+	net->get_remote_address(&addr_net);
+	if (!addr_net.is_ipv6()) {
 		// Primary connection already is IPv4, abort
 		return;
 	}
 
-	if (!NetAddress::resolve_to_v4(&addr, meta_, port_)) {
-		// Could not get the IPv4 address of the metaserver? Strange :-/
-		return;
-	}
-
-	std::unique_ptr<NetClient> tmpNet = NetClient::connect(addr);
-	if (!tmpNet || !tmpNet->is_connected()) {
-		// Connecting by IPv4 doesn't work? Well, nothing to do then
-		return;
-	}
-
-	// Okay, we have a connection. Send the login message and terminate the connection
-	SendPacket s;
-	s.string(IGPCMD_TELL_IP);
-	s.string(boost::lexical_cast<std::string>(kInternetGamingProtocolVersion));
-	s.string(clientname_);
-	s.string(authenticator_);
-	tmpNet->send(s);
-
-	// Close the connection
-	tmpNet->close();
+	// Do real work in thread to reduce freezing of GUI
+	// $this cannot become invalid since it is a global variable
+	// Member variables of $this might change their values but it does not really matter if the thread fails
+	std::thread([this]() {
+
+		NetAddress addr;
+		if (!NetAddress::resolve_to_v4(&addr, meta_, port_)) {
+			// Could not get the IPv4 address of the metaserver? Strange :-/
+			return;
+		}
+
+		std::unique_ptr<NetClient> tmpNet = NetClient::connect(addr);
+		if (!tmpNet || !tmpNet->is_connected()) {
+			// Connecting by IPv4 doesn't work? Well, nothing to do then
+			return;
+		}
+
+		// Okay, we have a connection. Send the login message and terminate the connection
+		SendPacket s;
+		s.string(IGPCMD_TELL_IP);
+		s.string(boost::lexical_cast<std::string>(kInternetGamingProtocolVersion));
+		s.string(clientname_);
+		s.string(reg_ ? "" : authenticator_);
+		tmpNet->send(s);
+
+		// Wait for the challenge
+		uint32_t const secs = time(nullptr);
+		try {
+			while (kInternetGamingTimeout > time(nullptr) - secs) {
+				// Check if the connection is still open
+				if (!tmpNet->is_connected()) {
+					return;
+				}
+				// Try to get a packet
+				std::unique_ptr<RecvPacket> packet = tmpNet->try_receive();
+				if (!packet) {
+					continue;
+				}
+				const std::string cmd = packet->string();
+				if (cmd != IGPCMD_PWD_CHALLENGE) {
+					// Wrong command, abort
+					return;
+				}
+				const std::string challenge = packet->string();
+				// Got a challenge. Calculate the response and send it
+				SendPacket s2;
+				s2.string(IGPCMD_PWD_CHALLENGE);
+				s2.string(crypto::sha1(challenge + authenticator_));
+				tmpNet->send(s2);
+				// Our work is done
+				return;
+			}
+		} catch (const std::exception& e) {
+			log("InternetGaming: Error when trying to transmit secondary IP.\n");
+			return;
+		}
+
+		log("InternetGaming: Timeout when trying to transmit secondary IP.\n");
+	}).detach();
 }
 
 /// Handle one packet received from the metaserver.
@@ -389,7 +428,15 @@
 
 	// Are we already online?
 	if (state_ == CONNECTING) {
-		if (cmd == IGPCMD_LOGIN) {
+		if (cmd == IGPCMD_PWD_CHALLENGE) {
+			const std::string nonce = packet.string();
+			SendPacket s;
+			s.string(IGPCMD_PWD_CHALLENGE);
+			s.string(crypto::sha1(nonce + authenticator_));
+			net->send(s);
+			return;
+
+		} else if (cmd == IGPCMD_LOGIN) {
 			// Clients request to login was granted
 			const std::string assigned_name = packet.string();
 			if (clientname_ != assigned_name) {
@@ -401,10 +448,11 @@
 			}
 			clientname_ = assigned_name;
 			clientrights_ = packet.string();
-			if (clientrights_ == INTERNET_CLIENT_UNREGISTERED) {
-				// Might happen that we log in with less rights than we wanted to.
+			if (reg_ && clientrights_ == INTERNET_CLIENT_UNREGISTERED) {
+				// Permission downgrade: We logged in with less rights than we wanted to.
 				// Happens when we are already logged in with another client.
 				reg_ = false;
+				authenticator_ = crypto::sha1(clientname_ + authenticator_);
 			}
 			state_ = LOBBY;
 			log("InternetGaming: Client %s logged in.\n", clientname_.c_str());
@@ -412,7 +460,7 @@
 
 		} else if (cmd == IGPCMD_ERROR) {
 			std::string errortype = packet.string();
-			if (errortype != "LOGIN" && errortype != "RELOGIN") {
+			if (errortype != IGPCMD_LOGIN && errortype != IGPCMD_PWD_CHALLENGE) {
 				log("InternetGaming: Strange ERROR in connecting state: %s\n", errortype.c_str());
 				throw WLWarning(
 				   _("Mixed up"), _("The metaserver sent a strange ERROR during connection"));
@@ -425,11 +473,12 @@
 		} else {
 			logout();
 			set_error();
+			log("InternetGaming: Expected a LOGIN, PWD_CHALLENGE or ERROR packet from server, but received "
+				"command %s. Maybe the metaserver is using a different protocol version?", cmd.c_str());
 			throw WLWarning(
 			   _("Unexpected packet"),
-			   _("Expected a LOGIN, RELOGIN or REJECTED packet from server, but received command "
-			     "%s. Maybe the metaserver is using a different protocol version ?"),
-			   cmd.c_str());
+			   _("Received an unexpected network packet from the metaserver. The metaserver could be "
+				 "using a different protocol version. If the error persists, try updating your game."));
 		}
 	}
 	try {
@@ -589,6 +638,9 @@
 			if (waitcmd_ == IGPCMD_GAME_OPEN) {
 				waitcmd_ = "";
 			}
+			// Get the challenge
+			std::string challenge = packet.string();
+			relay_password_ = crypto::sha1(challenge + authenticator_);
 			// Save the received IP(s), so the client can connect to the game
 			NetAddress::parse_ip(&gameips_.first, packet.string(), kInternetRelayPort);
 			// If the next value is true, a secondary IP follows
@@ -666,7 +718,7 @@
 }
 
 const std::string InternetGaming::relay_password() {
-	return authenticator_;
+	return relay_password_;
 }
 
 /// called by a client to join the game \arg gamename
@@ -701,7 +753,6 @@
 	SendPacket s;
 	s.string(IGPCMD_GAME_OPEN);
 	s.string(gamename_);
-	s.string("1024");  // Used to be maxclients, no longer used.
 	net->send(s);
 	log("InternetGaming: Client opened a game with the name %s.\n", gamename_.c_str());
 

=== modified file 'src/network/internet_gaming.h'
--- src/network/internet_gaming.h	2017-11-28 20:39:35 +0000
+++ src/network/internet_gaming.h	2018-03-02 19:20:25 +0000
@@ -218,6 +218,9 @@
 	std::string authenticator_;
 	bool reg_;
 
+	/// Password for connecting as host to a game on the relay server
+	std::string relay_password_;
+
 	std::string meta_;
 	uint16_t port_;
 

=== modified file 'src/network/internet_gaming_protocol.h'
--- src/network/internet_gaming_protocol.h	2017-11-29 20:43:35 +0000
+++ src/network/internet_gaming_protocol.h	2018-03-02 19:20:25 +0000
@@ -36,9 +36,10 @@
  * 2: Between build 19 and build 20 - Added UUID to allow reconnect with same username after
  *    crashes. When logging twice with a registered account, the second connection gets a free
  *     username assigned. Dropping RELOGIN command.
- * 3: Between build 19 and build 20 - Added network relay for internet games [supported]
+ * 3: Between build 19 and build 20 - Added network relay for internet games
+ * 4: Between build 19 and build 20 - Using CHAP for password authentication [supported]
  */
-constexpr unsigned int kInternetGamingProtocolVersion = 3;
+constexpr unsigned int kInternetGamingProtocolVersion = 4;
 
 /**
  * The default timeout time after which the client tries to resend a package or even finally closes
@@ -110,10 +111,8 @@
  * 1) Linking connections with IPv4 and IPv6
  * The UUID is used on the metaserver to link multiple connections by the same client. This
  * normally happens when the client supports IPv4 and IPv6 and connects with both protocol versions.
- * This
- * way, the metaserver knows that the client supports both versions and can show games / offer its
- * game
- * of/for clients with both protocol versions.
+ * This way, the metaserver knows that the client supports both versions and can show games / offer its
+ * game of/for clients with both protocol versions.
  *
  * When a network client connects to the metaserver with (RE)LOGIN it also sends the UUID.
  * When "another" netclient connects to the metaserver and sends TELL_IP containing the same UUID,
@@ -122,11 +121,9 @@
  *
  * 2) Reconnect after crash / network problems.
  * When Widelands breaks the connection without logging out, the server still assumes that the old
- * connection is active. So when the player reconnects, another name is chosen. Sending the UUID
- * allows
+ * connection is active. So when the player reconnects, another name is chosen. Sending the UUID allows
  * to reclaim the old name, since the server recognizes that there isn't a second player trying to
- * use
- * the same name.
+ * use the same name.
  */
 
 /**
@@ -144,8 +141,7 @@
  *
  * \note If you want to change the payload of this command, change it only by appending new items.
  *       The reason is that this is the only command that can be sent by the metaserver even when
- * the
- *       protocol versions differ.
+ *       the protocol versions differ.
  *
  */
 static const std::string IGPCMD_DISCONNECT = "DISCONNECT";
@@ -153,25 +149,30 @@
 /**
  * Initiate a connection.
  *
- * The first communication across the network stream is a LOGIN command
+ * The first communication across the network stream is a IGPCMD_LOGIN command
  * sent by the client, with the following payload:
- * \li string:    protocol version
+ * \li string:    protocol version (see kInternetGamingProtocolVersion)
  * \li string:    client name
  * \li string:    build_id of the client
  * \li string:    whether the client wants to login in to a registered account
  *                ("true" or "false" as string)
- * \li string:    for registered accounts: password in clear text
+ * \li string:    for registered accounts: string of length 0
  *                for unregistered users the UUID to recognize the matching IPv4 and IPv6
  *                connections or to reclaim the username after a unintended disconnect.
  *                For an explanation of the UUID, see above.
  *
- * If the metaserver accepts, it replies with a LOGIN command with the following payload:
- * \li string:    client name (might be different to the previously chosen one, if the client did
- *                NOT login to a registered account and either the chosen is registered or already
- *                used.)
- * \li string:    clients rights  (see client rights section above)
- *
- * If no answer is received in \ref kInternetGamingTimeout s the client will again try to login
+ * If the user tries to login to a registered account, a IGPCMD_PWD_CHALLENGE exchange follows before
+ * the server replies with a IGPCMD_LOGIN or IGPCMD_ERROR message.
+ *
+ * If the metaserver accepts, it replies with a IGPCMD_LOGIN command with the following payload:
+ * \li string:    client name. Might be different to the previously chosen one, if the chosen
+ *                name already used or is registered (and the connecting client is not registered).
+ * \li string:    clients rights (see client rights section above)
+ *
+ * When the client is downgraded to an unregistered user on login, a special UUID value
+ * of sha1(assignedName | passwordHash) has to be used on reconnects.
+ *
+ * If no answer is received in \ref kInternetGamingTimeout seconds the client will again try to login
  * \ref INTERNET_GAMING_RETRIES times until it finally bails out something like "server does not
  * answer"
  *
@@ -184,19 +185,37 @@
  *
  * Assuming the client already has a connection over IPv6 and tries to establish a secondary
  * connection over IPv4, this is the only message sent.
- * It should be sent as soon as a connection is established, immediately followed by closing
- * the connection. No answer from the server should be expected.
+ * It should be sent as soon as a connection is established.
+ * For unregistered users, the connection should be closed immediately following the command. No
+ * answer from the server should be expected.
+ * For registered users, an exchange of IGPCMD_PWD_CHALLENGE commands is done before closing the
+ * connection.
  *
  * Is sent by the client, with the following payload:
- * \li string:    protocol version
+ * \li string:    protocol version (see kInternetGamingProtocolVersion)
  * \li string:    client name - the one the metaserver replied at the first login
- * \li string:    for registered accounts: password in clear text
+ * \li string:    for registered accounts: string of length 0
  *                for unregistered users the UUID used on login
  *                for an explanation of the UUID, see above.
  */
 static const std::string IGPCMD_TELL_IP = "TELL_IP";
 
 /**
+ * This is sent by the metaserver after a IGPCMD_LOGIN or IGPCMD_TELL_IP by a registered client.
+ * This is the first message of the a protocol similar to the challenge handshake authentication
+ * protocol (CHAP) for secure transmission of the users password.
+ * The server sends the nonce for hashing:
+ * \li string:    a nonce for hashing
+ *
+ * The client should answer the message by an own IGPCMD_PWD_CHALLENGE containing the hashed password:
+ * \li string:    HASH_SHA1(nonce | HASH_SHA1(password))
+ *
+ * If the transmitted value is correct, the normal IGPCMD_LOGIN/IGPCMD_TELL_IP sequence continues. If the
+ * value is wrong (e.g., wrong password) the connection is terminated by the servers IGPCMD_DISCONNECT.
+ */
+static const std::string IGPCMD_PWD_CHALLENGE = "PWD_CHALLENGE";
+
+/**
  * This command is sent by the metaserver if something went wrong.
  * At least the following payload:
  * \li string:    IGPCMD code of the message that lead to the ERROR message or ERROR
@@ -321,11 +340,12 @@
 
 /**
  * Sent by the client to announce the startup of a game with following payload:
- * \li string:    name
- * \li string:    number of maximal clients
- * \note build_id is not necessary, as this is in every way the build_id of the hosting client.
+ * \li string:    name of the game
+ * \note build_id is not necessary, as this is the build_id of the hosting client anyway.
  *
  * Sent by the metaserver to acknowledge the startup of a new game with the following payload:
+ * \li string:    a challenge that has to be "solved" to work as host of the new game.
+ *                See IGPCMD_PWD_CHALLENGE. The response is send to the relay
  * \li string:    primary ip of relay server for the game.
  * \li string:    whether a secondary ip for the relay follows ("true" or "false" as string)
  * \li string:    secondary ip of the relay - only valid if previous was true

=== modified file 'src/network/relay_protocol.h'
--- src/network/relay_protocol.h	2017-12-17 14:45:23 +0000
+++ src/network/relay_protocol.h	2018-03-02 19:20:25 +0000
@@ -123,7 +123,8 @@
     * Has the following payload:
     * \li unsigned_8: Protocol version.
     * \li string:     Game name.
-    * \li string:     For the host: Password that was set on start of the relay.
+    * \li string:     For the host: Password that was set on start of the relay. Is the "solution"
+    *                 for the challenge send by the metaserver on IGPCMD_GAME_OPEN
     *                 For clients/observers: String "client".
     *
     * Is answered by kWelcome or kDisconnect (if a parameter is wrong/unknown).

=== modified file 'src/ui_fsmenu/multiplayer.cc'
--- src/ui_fsmenu/multiplayer.cc	2017-11-28 20:54:16 +0000
+++ src/ui_fsmenu/multiplayer.cc	2018-03-02 19:20:25 +0000
@@ -105,13 +105,22 @@
 	Section& s = g_options.pull_section("global");
 	if (auto_log_) {
 		nickname_ = s.get_string("nickname", _("nobody"));
-		password_ = s.get_string("password", "nobody");
+		password_ = s.get_string("password_sha1", "nobody");
 		register_ = s.get_bool("registered", false);
 	} else {
 		LoginBox lb(*this);
 		if (lb.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
 			nickname_ = lb.get_nickname();
-			password_ = lb.get_password();
+			/// NOTE: The password is only stored (in memory and on disk) and transmitted (over the network
+			/// to the metaserver) as cryptographic hash. This does NOT mean that the password is stored
+			/// securely on the local disk. While the password should be secure while transmitted to the
+			/// metaserver (no-one can use the transmitted data to log in as the user) this is not the case
+			/// for local storage. The stored hash of the password makes it hard to look at the configuration
+			/// file and figure out the plaintext password to, e.g., log in on the forum. However, the
+			/// stored hash can be copied to another system and used to log in as the user on the metaserver.
+			// Further note: SHA-1 is considered broken and shouldn't be used anymore. But since the
+			// passwords on the server are protected by SHA-1 we have to use it here, too
+			password_ = crypto::sha1(lb.get_password());
 			register_ = lb.registered();
 
 			s.set_bool("registered", lb.registered());
@@ -139,7 +148,7 @@
 
 		// Reset InternetGaming and passwort and show internet login again
 		InternetGaming::ref().reset();
-		s.set_string("password", "");
+		s.set_string("password_sha1", "");
 		show_internet_login();
 	}
 }

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2017-12-14 09:02:31 +0000
+++ src/wlapplication.cc	2018-03-02 19:20:25 +0000
@@ -71,6 +71,7 @@
 #include "logic/single_player_game_controller.h"
 #include "logic/single_player_game_settings_provider.h"
 #include "map_io/map_loader.h"
+#include "network/crypto.h"
 #include "network/gameclient.h"
 #include "network/gamehost.h"
 #include "network/internet_gaming.h"
@@ -747,7 +748,10 @@
 	s.get_string("uuid");
 	s.get_string("registered");
 	s.get_string("nickname");
+	// TODO(Notabilis): Remove next line after build 20.
+	// Currently left in to avoid removing stored passwords for users of both build 19 and trunk
 	s.get_string("password");
+	s.get_string("password_sha1");
 	s.get_string("emailadd");
 	s.get_string("auto_log");
 	s.get_string("lasthost");
@@ -765,6 +769,13 @@
 	}
 	s.set_int("last_start", time(nullptr));
 
+	// Replace the stored plaintext password with its SHA-1 hashed version
+	// Used to upgrade the stored password when upgrading widelands
+	if (s.get_string("password") != nullptr
+		&& (s.get_string("password_sha1") == nullptr || strlen(s.get_string("password_sha1")) == 0)) {
+		s.set_string("password_sha1", crypto::sha1(s.get_string("password")));
+	}
+
 	// Save configuration now. Otherwise, the UUID is not saved
 	// when the game crashes, loosing part of its advantage
 	try {
@@ -1175,8 +1186,9 @@
 			Section& s = g_options.pull_section("global");
 			s.set_string("nickname", playername);
 			// Only change the password if we use a registered account
-			if (registered)
-				s.set_string("password", password);
+			if (registered) {
+				s.set_string("password_sha1", password);
+			}
 
 			// reinitalise in every run, else graphics look strange
 			FullscreenMenuInternetLobby ns(playername.c_str(), password.c_str(), registered);

=== modified file 'src/wui/login_box.cc'
--- src/wui/login_box.cc	2017-02-26 12:16:09 +0000
+++ src/wui/login_box.cc	2018-03-02 19:20:25 +0000
@@ -63,7 +63,6 @@
 
 	Section& s = g_options.pull_section("global");
 	eb_nickname->set_text(s.get_string("nickname", _("nobody")));
-	eb_password->set_text(s.get_string("password", ""));
 	cb_register->set_state(s.get_bool("registered", false));
 	eb_nickname->focus();
 }