#3549 closed defect (fixed)
Secure authentication - prevent joins as a different player
Reported by: | elexis | Owned by: | Imarok |
---|---|---|---|
Priority: | Release Blocker | Milestone: | Alpha 23 |
Component: | Multiplayer lobby | Keywords: | beta |
Cc: | Patch: | Phab:D897 |
Description (last modified by )
The current implementation of the NetServer
allows users to join as a different player.
Reproduce:
- Alice, bob and carol start a game, (no matter if lobby or rated)
- Bob disconnects
- Eve notices that there are less players connected than the list displays
- Eve tries to join the game, but can't connect as the game already started
- Eve clicks on multiplayer -> join. The IP address is the one of alice' game
- Eve tries to connect with one of the names (including the rating as that's part of the name)
- After some tries, eve successfuly joins as Bob. Carol doesn't know its eve, as even the chat messages indicate it's bob.
The code we need to change for this can be found in CNetServerWorker::AddPlayer
:
// Try to match username next for (PlayerAssignmentMap::iterator it = m_PlayerAssignments.begin(); it != m_PlayerAssignments.end(); ++it) { if (!it->second.m_Enabled && it->second.m_Name == name && usedIDs.find(it->second.m_PlayerID) == usedIDs.end()) { playerID = it->second.m_PlayerID; m_PlayerAssignments.erase(it); // delete the old mapping, since we've got a new one now goto found; } }
But notice we have this other TODO in CNetServerWorker::OnAuthenticate
:
// Search for an old disconnected player of the same name // (TODO: if GUIDs were stable, we should use them instead)
We might need some secure authentication token, like the jabber ID for lobbied games.
Attachments (1)
Change History (19)
comment:1 by , 8 years ago
Milestone: | Backlog → Alpha 19 |
---|---|
Priority: | Must Have → Release Blocker |
follow-up: 5 comment:2 by , 8 years ago
Quick fix for A19: Don't save lobby IPs. It's bad practice to expose them like that anyway.
by , 8 years ago
Attachment: | no-save-lobby-ips.diff added |
---|
Don't save the host ip or player name if joining through the lobby
comment:3 by , 8 years ago
Keywords: | patch review added |
---|---|
Owner: | set to |
Summary: | Secure authentication - prevent joins as a different player → [PATCH] Secure authentication - prevent joins as a different player |
follow-up: 7 comment:5 by , 8 years ago
Notice the rating is part of the playername and can be faked too by joining by IP.
Exposing IPs:
Replying to Josh:
Quick fix for A19: Don't save lobby IPs. It's bad practice to expose them like that anyway.
Yes, that works for less sophisticated attackers and I agree it can be commited. The ticket should not be closed as the actual issue is not addressd.
The problem of exposing user IPs can be solved if we have only dedicated wfg-owned servers hosting. Then attackers will only get IPs of those servers instead of the player ones. Only the host sees the client IPs.
Proposed Solution for this ticket: My idea for solving this problem is to let the lobby server authenticate the user in game-join:
- Prohibit rejoins by IP for lobby games.
- If a player wants to join a lobby game, it sends that info to the lobby server
- The lobby server sends the info (including the players IP) to the host
- The host assigns the playername + rating to that client as provided by the lobby server
I see no other possiblity.
comment:6 by , 8 years ago
Keywords: | patch review removed |
---|---|
Milestone: | Alpha 19 → Alpha 20 |
Owner: | removed |
Summary: | [PATCH] Secure authentication - prevent joins as a different player → Secure authentication - prevent joins as a different player |
Patched in r17147. Doesn't fix the root issue, but repairs a tangential one. Should reduce the abuse enough for A19.
comment:7 by , 8 years ago
Extending comment:5:
Prohibit rejoins by IP for lobby games
Therefore users without accounts can't participate in lobbied games
As every gamejoin is registered with the lobby server
- can improve the prevention, clarification of abuse and "proving the point" a bit
- more troll accounts will have to be banned though
comment:8 by , 8 years ago
Priority: | Release Blocker → Must Have |
---|
comment:10 by , 7 years ago
Keywords: | essential added |
---|
comment:11 by , 7 years ago
Keywords: | essential removed |
---|
comment:12 by , 7 years ago
Keywords: | beta added |
---|
comment:13 by , 7 years ago
Description: | modified (diff) |
---|---|
Milestone: | Backlog → Alpha 23 |
Owner: | set to |
comment:14 by , 7 years ago
Component: | Core engine → Multiplayer lobby |
---|
comment:15 by , 7 years ago
Patch: | → Phab:D897 |
---|
comment:16 by , 6 years ago
Priority: | Must Have → Release Blocker |
---|
I would recommend to not release without Imaroks patch. The trolls and smurfs are annoying and this is an effective means to mitigate the impact. Incidentally also helps with the "unbannable nickname" hack.
We need to have more host-side validation; simple redefining a couple variables in session.js will allow one to impersonate other players.
Raising this to release blocker for Alpha 19 (for attention, doesn't have to stay there).
Abuse of this in the multiplayer lobby has been increasing.