widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve

2 little nits, LGTM otherwise.

Which algorithm would sou recommend instead of sha-1? If it's available on Django, we should consider changing the website too.

Diff comments:

> === 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
> @@ -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();

This is no longer called in the thread below. On purpose?

> +	// 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.
> 
> === 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
> @@ -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)) {

char const* Section::get_string(char const* const name, char const* const def) allows specifying a fallback, so you can reduce this line to:

    && (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 {


-- 
https://code.launchpad.net/~widelands-dev/widelands/net-pwd-security/+merge/340540
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-pwd-security.