Renetcode: disconnect clients that are sending ConnectionRequests #136
Conversation
If ConnectionRequest is received while a client is connected, this might be a rejoin. If the connection has started more than 10 seconds (could be a late packet) we force a disconnect so the connection workflow can restart.
| Packet::Disconnect => { | ||
| let client_id = client.client_id; | ||
| self.clients[slot] = None; | ||
| log::trace!("Client {} requested to disconnect", client_id); | ||
| return Ok(ServerResult::ClientDisconnected { | ||
| client_id, | ||
| addr, | ||
| payload: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This line was removed:
client.state = ConnectionState::Disconnected;
| Packet::ConnectionRequest { .. } => { | ||
| // If a ConnectionRequest is received while connected, | ||
| // this might be a client trying to rejoin |
There was a problem hiding this comment.
I don't think this branch can actually run in practice for an honest client, since find_client_mut_by_addr() will fail for the probably-new client address (assuming clients typically generate a new port when reconnecting).
However, this branch can be exploited by a malicious party to force-disconnect clients using packet-sniffed client connection requests.
The function find_client_mut_by_addr() is an important security check in the netcode protocol that stands between connection requests and packet decryption + replay protection. Note that in Packet::decode() the PacketType::ConnectionRequest is not encrypted and doesn't use replay protection, while the other packets have both. Reference: https://gafferongames.com/post/client_server_connection/
There was a problem hiding this comment.
Yes, you are correct
Hmm, would decoding the private connect token and checking that it is valid and from the same client be enough?
There was a problem hiding this comment.
No, because the attacker is free to use an old connection request from the user.
You could perhaps include a counter inside the connect token (authenticated as part of the private connect token), and auto-disconnect if a higher-generation token shows up for the same client.
There was a problem hiding this comment.
@lucaspoffo an alternative to a counter would be to use the token's create_timestamp (or expire_timestamp, since only the expiration is currently encoded in the private connect token). If a fresher connect token is received, then the server can safely disconnect the client and reconnect with the new token.
One important detail: the server must emit a disconnect event for a reconnecting client so the server can properly reinitialize them.
This general approach is not strictly as secure as the default netcode protocol. If clients request multiple new connect tokens from the backend, e.g. on a timer while disconnected, then they may reconnect with an older connect token while a newer connect token is only transmitted and cached. If an attacker somehow acquires the newer token then they can use it to force-disconnect the client. However it is better than blindly disconnecting clients, because usually clients will receive connect tokens over a secure channel (and only the renet client -> renet server initial connection is insecure).
If ConnectionRequest is received while a client is connected, this might be a rejoin. If the connection has started more than 10 seconds (could be a late packet) we force a disconnect so the connection workflow can restart.