Page MenuHomeWildfire Games

Secure lobby authentication - prevent joins as a different player
ClosedPublic

Authored by Imarok on Sep 9 2017, 2:59 PM.

Details

Reviewers
Dunedan
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP21520: Secure lobby authentication - prevent joins as a different player
Trac Tickets
#3549
Summary

Prevent joins as different player (within the lobby).
To achieve that, several things have been changed:

  • The server and not the client chooses the GUID and sends it with the CSrvHandshakeMessage
  • Added PM support to XmppClient

When lobby authentification is enabled, the server sets the LOBBY_AUTH_FLAG in CSrvHandshakeMessage.
The client recieving that, proves his identity, by sending the assigned guid back to the server via a Lobby xmpp Private Message.
When the server receives that, he sends an empty CAuthenticateMessage to the client to request the normal authentication message.
After the server received that message, it compares the sent playername with the username of the xmpp account.
Here is the whole procedure:

Test Plan

Test it.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Mar 7 2018, 7:41 PM
binaries/data/config/default.cfg
404

should contain the word impersonation

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
240

The one you removed from this diff.

273

halfHeight, so that we don't have to conclude what h might be

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.xml
123

Use Title Case

source/lobby/StanzaExtensions.cpp
269

false comment

source/lobby/XmppClient.cpp
776

lobbyAuth would be more readable

785

These references to globals (g_NetServer) are not good, but we already have them. Should add a setter sometime.

source/network/NetMessages.h
34

SRVHSRES ? What's that supposed to mean? (PS meant prometheus). Maybe PS_FLAG_LOBBYAUTH?
(The flag should come first, so that it's nicely aligned if there are more flags)

source/network/NetServer.cpp
440

This function is becoming too long and complex, better move it to a helper function.

451

Either +1 tab or return on one line as in the other place. My \n request was posted when there were multiple statements within this function.

456

One place of the patch uses utf8_from_wstring here it uses FromUTF8, decide on one style, use the one that's most predominant.

953

Is that so? If so, why? See irc discussion today.

(17:24:19) Imarok: elexis: because xmpp just has lowercase

Could not confirm, maybe ejabberd does that. https://xmpp.org/extensions/xep-0029.html#sect-

comparisons will be made in case-normalized canonical form.

is the only thing I found.

When implementing a protocol and relying on some defined behavior, post a link to the specs defining that behavior.
Otherwise it could just be property of the server software / version / config we happen to use.

source/network/NetServer.h
140

(Proposing to use the same name that the hostName is renamed to, lobbyUsername or whatever)

366

(comment quadruplication)

Imarok added a comment.Mar 8 2018, 8:16 PM
In D897#55877, @elexis wrote:

I have rebased this patch 3 times manually now, would be / have been good to update the diff.

You could have just said something ;P

Imarok added a comment.Mar 8 2018, 9:00 PM
In D897#55877, @elexis wrote:

The idea is that one could change it ingame.
There's a small use case of allowing player replacements temporarily, but that only works when the host doesn't run STUN and we should formalize replacements.
Still would add the option until then if it works to change it in running games.

Looking at the code it seems it works to change it while running, but don't think there is any real usecase.

I'm a bit dubious about the secure lobby authentication, should that just be "lobby authentication"?

don't care, "secure lobby authentication" sounded more "enterprise-like" ;)

Imarok marked 18 inline comments as done.Mar 8 2018, 11:40 PM
Imarok added inline comments.
source/lobby/StanzaExtensions.cpp
246

Wondering if that shouldn't be labeled LobbyAuthQuery.

Contrary to the other ones it's no query.

260

Uh, shame

source/network/NetMessages.h
34

ServerHandshakeResponse

source/network/NetServer.cpp
952

yeah, we need to get rid of that sometimes...

973

It's changed, cause we need this && (*it) != sessionfor the patch (as said already on irc)

1567

good idea.

source/network/NetServer.h
140

(Proposing to use the same name that the hostName is renamed to, lobbyUsername or whatever)

That are two totally different things, so no.

Imarok updated this revision to Diff 6080.Mar 9 2018, 12:03 AM
Imarok marked 3 inline comments as done.

All the fixes

Vulcan added a comment.Mar 9 2018, 4:22 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 153| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 191| »   »   »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/190/display/redirect

elexis added a comment.Mar 9 2018, 1:49 PM

(As you might have noticed from the open concerns, I'm not too confident with the gloox types, especially memory leaks, so I have some reservations with evaluation there)

binaries/data/config/default.cfg
404

It sounds like it not only prevents players from impersonating other players but also players from joining, so I'd remove that joining and

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.xml
124

We had much better descriptions in irc last night.
Impersonation is a word which I would not leave out under any circumstance.
"Prevent players from impersonating others."?

source/lobby/StanzaExtensions.cpp
252

-1 space

285

return new LobbyAuth()?

source/network/NetClient.cpp
490

(Weird that we had the pw field in a message that was only sent from the server to the client until now)

source/network/NetMessages.h
34

Exactly the reason why we don't use abbreviations in names. It wastes the readers time guessing and often we don't even come to a conclusion what it's supposed to mean!

So I'd be for a DUABNKWTSF rule for the coding conventions (dont use abbreviations because noone knows what they stand for).

source/network/NetServer.cpp
411

Thanks, much better

440

thx

894

Whitespace comment from last time still applies. Either compress the last 3 lines in one, or something with tabs that looks similar to what we have in the other find_if place.

953

bump

955

Make it red = LOGERROR (Ask if you don't comprehend something and say no thank you if you disagree)

973

18:05 < Imarok> maybe because previously session had no username in this state, but may have one cause of lobby auth
18:05 < Imarok> elexis if that answer does not satisfy you, you need to wait until I get at my PC

Waiting.
session->SetUserName(username); occurs in L1077 below this code.

source/network/NetServer.h
140

It's the lobby playername of the joining client, right? IMO should still have the lobby, xmpp, jid or whatever in there so people aren't left clueless

236

\n above, -\n below I guess

source/network/scripting/JSInterface_Network.cpp
42

"hosting playername" is still a bad name because we have a second variable that holds a "hosting playername".
Distinguish it by adding the lobby word.

Imarok updated this revision to Diff 6101.Mar 10 2018, 12:59 AM
Imarok marked 17 inline comments as done.

More concerns addressed.

binaries/data/config/default.cfg
404

Yeah, sorry, I forgot to add our sentence...

source/network/NetClient.cpp
490

It's the other way around. (Look at my image in the top post)

source/network/NetServer.cpp
953

What do you want from me?

955

Sorry, looks like I forgot that.

973

(*it)->SetUserName(name.FromUTF8()); (L883)

source/network/NetServer.h
236

I think just +\n is better.

source/network/scripting/JSInterface_Network.cpp
42

Don't get your issue

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 153| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 191| »   »   »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/204/display/redirect

In D897#55877, @elexis wrote:

There is one in default.cfg, but you are right that we should have an according entry in options.json

You are asked every time you host a lobbied game, if joins from outside the lobby are allowed, so not sure, this additionally needs an option...

The idea is that one could change it ingame.
There's a small use case of allowing player replacements temporarily, but that only works when the host doesn't run STUN and we should formalize replacements.
Still would add the option until then if it works to change it in running games.
Consequently that useSecureAuth bool shouldn't be part of the constructor.

If we remove the useSecureAuth bool from the constructor and make it settable by a function:
How do we access that?
Doing it cleanly we'd need to create a netmessage for it, so that the gui can access it via g_NetClient.
But as that would just be something temporary, I don't think it makes sense, so just "dirtily" setting it via g_NetServer seems appropriate.

That said, what about: this alpha you need to either turn it on or turn it of, but you won't change it ingame and bringing the replace player thing for next alpha? (Would save us and translators work that will be reverted within the coming months)

Itms added a subscriber: Itms.Mar 11 2018, 6:30 PM

There could be an improvement that I wrote down, but apart from that I don't see any issue with the changes to the glooxwrapper.

source/lobby/glooxwrapper/glooxwrapper.h
465

This could return a const string.

Imarok updated this revision to Diff 6144.Mar 11 2018, 7:56 PM

const string

Imarok marked an inline comment as done.Mar 11 2018, 8:19 PM
Imarok updated this revision to Diff 6151.Mar 12 2018, 12:32 AM

Nicer comments for elexis

elexis accepted this revision.Mar 12 2018, 12:57 AM

Yes, it can remain in the constructor. Just saying that some players will run into that "cant replace player anymore" situation. But given that most host via STUN that's already become less probable.

Ok to recap what this patch does and Verification that it all makes sense:

  • Player 1 joins lobby and clicks host with the option checked
  • Player 2 joins lobby, selects that game and clicks join
  • Player 2 NetClient starts the UDP based handshake as usual
  • Player 1 enters CNetServerWorker::OnClientHandshake for that client. Due to your preparation in rP20341, the NetServer choses a clientGUID for that candidate.
  • Player 1 is still in CNetServerWorker::OnClientHandshake and notices the lobbyauth option being enabled, sends the PS_REQUIRE_LOBBYAUTH_FLAG flag and the GUID in the handshake response to the client of Player 2
  • Player 2 receives the HandshakeResponse packet, enters CNetClient::OnHandshakeResponse notices that flag and calls g_XmppClient->SendIqLobbyAuth(client->m_HostingPlayerName, client->m_GUID); where m_HostingPlayerName is the lobby JID node name of the hosting player (not the ingame playername of the host)
  • Player 2's XmppClient constructs new LobbyAuth stanza, sends it via the lobby server to the hosting client. The Stanza contains the GUID that the client just received and acts as an authentication token to the NetSever of player 1. The connection was kept alive meanwhile by enet by sending pings.
  • Player 1 receives this stanza in XmppClient::handleIq, passes it on to g_NetServer->OnLobbyAuth, it ends up in m_LobbyAuthQueue where it is processed by the NetServerWorker immediately in CNetServerWorker::RunStep, which calls ProcessLobbyAuth
  • Player 1 CNetServerWorker::ProcessLobbyAuth then stores the lobby JID node name of the sender of the stanza in the netclient session of the client that has the GUID that the stanza contains. So basically it reserves a seat. Then it sends the CAuthenticateMessage to that client as usual.

This story covers every line in the patch. So there's nothing in the patch that doesn't belong here.

Edge cases:

  • What if someone sents a wrong stanza, should that session be terminated? Should we add a timeout eventually? Not really needed now as that is most likely already possible by not completing the authentication or handshake.

Possible defects:

  • Is the GUID leaked in any message? The unauthenticated client isn't part of the player assignments yet, so the playerassignments message is safe. But maybe others, like the lag notifications?
  • It looks like the patch enforces that a client can only join a game with the JID node name in the lobby. But authentication could also work agnostic of the ingame name chosen. So the (*it)->SetUserName(name.FromUTF8()); call is kind of a workaround and could just use an isLobbyAuthenticated = true. Then we could maybe get rid of the (*it) != session part too.
  • Maybe something in gloox neither Itms or me have seen?

Thanks for the patch! It is really important with our great lobby community unfortuntely!

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.xml
124

before they can join?

source/lobby/StanzaExtensions.h
124

I have no clue why these four methods are all required by gloox or what that actually does and I didn't have time to learn this.

source/lobby/XmppClient.cpp
423

whitespace

787

We should split this function some day.

source/network/NetClient.h
264

Seems weird that one is called Username and the other Playername, one CStr and one CStrW, should be consistent.

source/network/NetMessages.h
34

Maybe PS_FLAG_LOBBYAUTH?
(The flag should come first, so that it's nicely aligned if there are more flags)

I would like a response to this comment.

Which one seems better?

PS_FLAG_SOMETHING
PS_FLAG_ELSE
PS_FLAG_LITTLE_BIT_DIFFERENT
PS_FLAG_VASTLY_DIFFERENT

or

PS_SOMETHING_FLAG
PS_ELSE_FLAG
PS_LITTLE_BIT_DIFFERENT_FLAG
PS_VASTLY_DIFFERENT_FLAG

Also we have many places where we flags could be used, so probably better to add a prefix like PS_SERVER_FLAG_, PS_NETMESSAGE_FLAG, PS_PROTOCOL_FLAG_ or PS_NETWORK_FLAG_.

source/network/NetServer.cpp
893

This means we can send this packet to other clients whose GUID we learnt after the join?

899

This comment cost me some time to understand.

We have just finished the lobby authentication, so it's a response to a successful authentication.

What the comment means is the UDP password based authentication that is not implemented yet!

926

|= to keep it easily extensible

953

This is not a matter between me and you but between the reader and the code.

"When implementing a protocol post a link to the specs defining that behavior."

972

I do read now that (*it) != session is indeed needed, as the OnAuthenticate occurs after the lobby auth processing which sets the username earlier than before!

source/network/scripting/JSInterface_Network.cpp
42

playerName is the name of the host too, so it's more confusing than it needs to be.
lobbyPlayername would inform the reader that this is the name in the lobby, not the name in the UDP / simulation area of the game.

This revision is now accepted and ready to land.Mar 12 2018, 12:57 AM
In D897#56801, @elexis wrote:
  • It looks like the patch enforces that a client can only join a game with the JID node name in the lobby. But authentication could also work agnostic of the ingame name chosen. So the (*it)->SetUserName(name.FromUTF8()); call is kind of a workaround and could just use an isLobbyAuthenticated = true. Then we could maybe get rid of the (*it) != session part too.

But how would we check if the jid matches the name the user tries to join with?

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 153| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 191| »   »   »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/229/display/redirect

Imarok marked 3 inline comments as done.Mar 12 2018, 1:21 AM
Imarok added inline comments.
source/network/NetServer.cpp
899

Not only that, just the Authenticate message in general.

Imarok updated this revision to Diff 6152.Mar 12 2018, 1:22 AM

Fix some concerns and make transfering the diff easier

This revision was automatically updated to reflect the committed changes.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 153| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 191| »   »   »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/235/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 153| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 191| »   »   »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/236/display/redirect