[PATCH] Ensure the unique client IDs

[elexis]

The current network code relies on GUIDs being unique, but doesn't ensure that this is actually the case.

Currently, the clients just pick a random GUID in the constructor of NetClient.cpp. If a player rejoins, he will have a new GUID too.

The server should disconnect clients with a duplicate GUID or even decide the GUID of the clients (rather than the clients).

[elexis]

(changed component to network)

[Andy Alt]

So if I understand this correctly, it would be very rare that GUIDs are duplicated, but you'd like to make sure it never happens?

You want to focus on the server disconnecting the client, as opposed to doing a check for a duplicate GUID, and if duplicate, generate another GUID?

Right now the client is deciding the GUID, but you'd like the server to decide upon it.

[rockytriton]

"The term GUID is generally used by developers working with Microsoft technologies, while UUID is used everywhere else. 128-bits is big enough and the generation algorithm is unique enough that if 1,000,000,000 GUIDs per second were generated for 1 year the probability of a duplicate would be only 50%."

Sounds like a wasted effort to me, unless you think you might have 1 billion people joining your game within 1 second.

[elexis]

Disconnecting a client that comes with a GUID that already exists shouldn't be too hard to do though. Furthermore the chance can become 100% if someone would modify the code.

[Vladislav Belov]

Replying to rockytriton:

Sounds like a wasted effort to me, unless you think you might have 1 billion people joining your game within 1 second.

It's only probability, not the real case, so you could get a duplication even on a first pair. Also you should know that GUID is generated by PRNG, not the real RNG, so probability is even more.

[elexis]

Yep, looks good. There is a typo though and I'd rather append the new disconnect reason to the end of the enum (so that the existing disconnect reasons keep their ID). Also noone but coders know what a GUID is, so better speak of "Player identifier" and mention that the player should retry.

Bonus question: What does actually happen if a malicious client was to use an existing GUID, (i.e. if the NetClient replaces that get-guid part with a fixed string)?

[sbirmi]

First attempt at adding a "unique" guid check when a new client is completing authentication

[sbirmi]

Updated the diff with the recommendations (also adding my name to credits in the new diff)

[elexis]

For the bonus question, changing m_GUID(ps_generate_guid()) to m_GUID("bla") in the NetClient is required.

In order to test the bonus question (or the patch itself), one has to open 0AD once to host a game and join in a second window. On Windows this is only possible as a late-observer (as the filesystem prohibits simultaenous file access when starting the game in two windows simultaneously)

[sbirmi]

Manually tested the fix works (screenshot)

[sbirmi]

Hi elexis,

Applied the following diff (as suggested):

Index: network/NetClient.cpp
===================================================================
--- network/NetClient.cpp       (revision 19084)
+++ network/NetClient.cpp       (working copy)
@@ -69,7 +69,7 @@
 CNetClient::CNetClient(CGame* game, bool isLocalClient) :
        m_Session(NULL),
        m_UserName(L"anonymous"),
-       m_GUID(ps_generate_guid()), m_HostID((u32)-1), m_ClientTurnManager(NULL), m_Game(game),
+       m_GUID("foo"), m_HostID((u32)-1), m_ClientTurnManager(NULL), m_Game(game),
        m_GameAttributes(game->GetSimulation2()->GetScriptInterface().GetContext()),
        m_IsLocalClient(isLocalClient),
        m_LastConnectionCheck(0),

If you had a unit test in mind for this, can you point me to an existing test that spawn a server and a client instance?

Thanks, Sharad

[elexis]

Adding a unit test is actually recommended. We do have test_Net.h that sets up a server and some clients. Since there is no use-case to change m_GUID of a client, there is no such function (f.e. SetGUID) to accomplish that. It would have to be added just for that test. But if it were added, then it would incentivize developers to use it outside of the tests, so I would actually argue against adding that function and the test (We shouldn't add functions that shouldn't be used).

I rather meant just compiling your code with the client patch and without the server patch. Then starting pyrogenesis, enabling "Late Observers" in the options, then clicking on multiplayer->host->start game. Then starting pyrogenesis again, clicking on multiplayer -> join to join the running game as observer. Thus either the server crashes somewhere or you got some impersonation thing going on I expect, giving reason to merge your proposed server patch.

[sbirmi]

Ah okay.

Tried it out (only the client patch, no server fix). When a duplicate guid user connects, there is no crash but the original (first) user isn't able to send any instructions anymore. I wasn't able to assign tasks to units, get buildings to do anything. The new "observer" also doesn't have any control over the units.

Closing the observer first didn't result in a crash. Control wasn't resumed to the first player.

Note that I tested this with a multiplayer game with 1 human and 1 AI originally (and added a spectator later).

[elexis]

And sending chat messages?

[sbirmi]

Hmm.. tried doing this again. Now, after the observer connects, within a few seconds, I end up getting a "out of sync in turn N" message.

Specifically about chat messages, I don't see a crash. They appear to originate from the same username (I had to provide different names otherwise the game wouldn't let the observer connect).

Messages sent from an observer to observers-only appears in the player's screen.

Messages from the player to allies or enemies don't appear on the observer's screen, messages sent to everyone or observers-only appear on the observer's screen.

[elexis]

Replying to sbirmi:

Hmm.. tried doing this again. Now, after the observer connects, within a few seconds, I end up getting a "out of sync in turn N" message.

Wouldn't be surprised if an out of sync is due to sending simulation commands.

(I had to provide different names otherwise the game wouldn't let the observer connect)

(That's intended)

Messages from the player to allies or enemies don't appear on the observer's screen, messages sent to everyone or observers-only appear on the observer's screen.

(As intended)

Specifically about chat messages, I don't see a crash. They appear to originate from the same username Messages sent from an observer to observers-only appears in the player's screen.

So the worst that could happen is someone joining as observer, grabbing the GUID of some player and then being able to chat with the allies, despite not being an assigned player?

[sbirmi]

No. The worst (and this happens everytime) is a denial of service. The original player isn't able to control his/her units anymore. S/he can try clicking on his/her units and passing instructions but none of the commands get followed.

[sbirmi]

Also, if the observer disconnects, control isn't restored to the original player. I didn't check if the original player could disconnect and reconnect to resume control but by that time, precious time may have been lost already.

[elexis]

Thanks for the patch and thorough testing, the patch receives my recommendation to be added! :-)

[elexis]

In 19225:

Ensure unique client GUIDs. Patch by sbirmi, fixes #3949.

Two clients chosing the same GUID is highly unlikely, yet possible.
A malicious client chosing an existing GUID would have resulted in unassigning the player with that GUID.

[elexis]

Thanks again and a sorry from the team for letting your patch rot!

[Imarok]

In ​Phab:rP20341:

The server should choose the guid

Reviewed by: elexis

Differential Revision: ​https://code.wildfiregames.com/D943