Page MenuHomeWildfire Games

Identify host session via being on the same process.
Needs ReviewPublic

Authored by wraitii on Mon, Nov 9, 11:57 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

rP18140 made it so that the Server identifies the "host" (able to kick...) by listening to clients telling it they're the host. Yes, this is as secure as it sounds.
elexis rightfully raised a concern with it, suggesting that client and server should instead share a secret that the client uses to authenticate itself.

I agree, and in the short term the simplest secret is "I'm in the same process as you are". #3953 was concerned with data races, but this is safe.
Here's the breakdown of things:

  • The client handshakes.
  • The server generates a GUID and sends it to the client (handshake response).
  • The client receives that response, sets m_GUID, then sends an Authenticate request to the server
  • The server receives the authenticate request, compares the GUID with that of g_NetClient, and assigns host-ness.

It is thus rather obvious that the server cannot read from m_GUID while it is being written to, nor before it is written to.

This reverts rP18140.


One might wonder how this interacts with dedicated servers (#3556). It doesn't. For one thing, dedicated servers don't start a client, so the point is somewhat moot, and dedicated servers don't really need a host able to ban players so much as a players being able to vote or something.
_should_ we want dedicated-servers-host to be able to start a "host" client, we probably should go the whole way to an actual 'i am host' secret, but this is un-necessary for now.

Test Plan

Notice that you cannot impersonate the host any longer.

Event Timeline

wraitii created this revision.Mon, Nov 9, 11:57 AM
wraitii edited the summary of this revision. (Show Details)Mon, Nov 9, 11:58 AM
wraitii edited the summary of this revision. (Show Details)

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3453/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2900/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1801/display/redirect

wraitii requested review of this revision.Mon, Nov 9, 12:46 PM
Angen added a subscriber: Angen.Mon, Nov 9, 1:03 PM
Angen added inline comments.
source/network/NetSession.cpp
273

lol

Angen added a comment.Mon, Nov 9, 1:05 PM

your locale copy or jenkins is outdated, it fails to apply patch

So it does auth correctly on if ps_generate_guid() will be the same for client and server? So it might mean that someone can relatively easily generate it? Why to not generate a GUID with some randomness before and pass it to both?

So it does auth correctly on if ps_generate_guid() will be the same for client and server? So it might mean that someone can relatively easily generate it?

I'm not sure I understand your comment? Server-side, this sets the "host GUID" if the authenticating session has the same GUID as the NetClient session running in the same process. I don't really see how you could spoof this efficiently unless there also were two sessions with the same GUID in the server, which is impossible. Even if you sent the same GUID as the host, it'd still only recognise the host as the host?

Why to not generate a GUID with some randomness before and pass it to both?

This is essentially what this diff does though. Except there's no "passing" since the server can 'peek' via being in the same process. Doesn't work for dedicated servers but I don't think it's a huge concern.

This is essentially what this diff does though. Except there's no "passing" since the server can 'peek' via being in the same process. Doesn't work for dedicated servers but I don't think it's a huge concern.

That sounds a bit unsafe. Since two parallel object creations depend on the global state. When you create NetClient and NetServer you know that they're on the same machine/in the same process. Why do you need to hide it and delay it?

That sounds a bit unsafe. Since two parallel object creations depend on the global state.

Well, yes, but as I noted in the diff summary it's not actually all that parallel, since we have a handshake process for setting up such things, which got slike this:

  • Client/server connect -> Server handshakes
  • Client handshakes back
  • Server responds to the handshake <- this is where the server create the GUID
  • Client receives and Authenticates <- this is where the client writes m_GUID
  • Server receives the authentication request <-- this is where the server checks GUID

So it's actually quite guaranteed right now that there won't be data races in this.

When you create NetClient and NetServer you know that they're on the same machine/in the same process. Why do you need to hide it and delay it?

Essentially, he 'trouble' is that GUIDs don't exist until the server handles the "client handshake" message. This means the client needs to send the secret as part of this handshake. That means both server and client need to store data, and we need the network protocol to be updated.
This is all very possible, agree is conceptually more expected, but I don't think it provides much of a tangible benefit over simply 'peeking' in the authenticate step, since we can.