Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Renetcode: disconnect clients that are sending ConnectionRequests #136

Closed
wants to merge 1 commit into from
Closed

Renetcode: disconnect clients that are sending ConnectionRequests #136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 97 additions & 55 deletions renetcode/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ struct Connection {
user_data: [u8; NETCODE_USER_DATA_BYTES],
addr: SocketAddr,
last_packet_received_time: Duration,
connection_started_time: Duration,
last_packet_send_time: Duration,
timeout_seconds: i32,
sequence: u64,
Expand Down Expand Up @@ -338,6 +339,7 @@ impl NetcodeServer {
client_id: connect_token.client_id,
last_packet_received_time: self.current_time,
last_packet_send_time: self.current_time,
connection_started_time: self.current_time,
addr,
state: ConnectionState::PendingResponse,
send_key: connect_token.server_to_client_key,
Expand Down Expand Up @@ -406,38 +408,59 @@ impl NetcodeServer {
);

client.last_packet_received_time = self.current_time;
match client.state {
ConnectionState::Connected => match packet {
Packet::Disconnect => {
client.state = ConnectionState::Disconnected;
if client.state != ConnectionState::Connected {
return Ok(ServerResult::None);
}

match packet {
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,
});
}
Comment on lines +416 to +425
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was removed:
client.state = ConnectionState::Disconnected;

Packet::Payload(payload) => {
if !client.confirmed {
log::trace!("Confirmed connection for Client {}", client.client_id);
client.confirmed = true;
}
return Ok(ServerResult::Payload {
client_id: client.client_id,
payload,
});
}
Packet::KeepAlive { .. } => {
if !client.confirmed {
log::trace!("Confirmed connection for Client {}", client.client_id);
client.confirmed = true;
}
return Ok(ServerResult::None);
}
Packet::ConnectionRequest { .. } => {
// If a ConnectionRequest is received while connected,
// this might be a client trying to rejoin
Comment on lines +443 to +445
Copy link
Contributor

@UkoeHB UkoeHB Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct

Hmm, would decoding the private connect token and checking that it is valid and from the same client be enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@UkoeHB UkoeHB Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).


// We check the time when the connection started to detect rejoins
const REJOIN_TIMEOUT: Duration = Duration::from_secs(10);
if client.connection_started_time + REJOIN_TIMEOUT < self.current_time {
let client_id = client.client_id;
self.clients[slot] = None;
log::trace!("Client {} requested to disconnect", client_id);
log::trace!("Client {} is trying to rejoin", client_id);

// Disconnect client to restart the connection process
return Ok(ServerResult::ClientDisconnected {
client_id,
addr,
payload: None,
});
}
Packet::Payload(payload) => {
if !client.confirmed {
log::trace!("Confirmed connection for Client {}", client.client_id);
client.confirmed = true;
}
return Ok(ServerResult::Payload {
client_id: client.client_id,
payload,
});
}
Packet::KeepAlive { .. } => {
if !client.confirmed {
log::trace!("Confirmed connection for Client {}", client.client_id);
client.confirmed = true;
}
return Ok(ServerResult::None);
}
_ => return Ok(ServerResult::None),
},

return Ok(ServerResult::None);
}
_ => return Ok(ServerResult::None),
}
}
Expand Down Expand Up @@ -738,12 +761,44 @@ mod tests {

#[test]
fn server_connection() {
fn client_connect(client: &mut NetcodeClient, client_addr: SocketAddr, user_data: [u8; 256], server: &mut NetcodeServer) {
assert!(!client.is_connected());
let (client_packet, _) = client.update(NETCODE_SEND_RATE).unwrap();

let result = server.process_packet(client_addr, client_packet);
assert!(matches!(result, ServerResult::PacketToSend { .. }));
match result {
ServerResult::PacketToSend { payload, .. } => client.process_packet(payload),
_ => unreachable!(),
};

assert!(!client.is_connected());
let (client_packet, _) = client.update(Duration::ZERO).unwrap();
let result = server.process_packet(client_addr, client_packet);

match result {
ServerResult::ClientConnected {
client_id: r_id,
user_data: r_data,
payload,
..
} => {
assert_eq!(client.client_id(), r_id);
assert_eq!(user_data, *r_data);
client.process_packet(payload)
}
_ => unreachable!(),
};

assert!(client.is_connected());
}

let mut server = new_server();
let server_addresses: Vec<SocketAddr> = server.addresses();
let user_data = generate_random_bytes();
let expire_seconds = 3;
let client_id = 4;
let timeout_seconds = 5;
let expire_seconds = 500;
let client_id: u64 = 4;
let timeout_seconds = 30;
let client_addr: SocketAddr = "127.0.0.1:3000".parse().unwrap();
let connect_token = ConnectToken::generate(
Duration::ZERO,
Expand All @@ -757,35 +812,9 @@ mod tests {
)
.unwrap();
let client_auth = ClientAuthentication::Secure { connect_token };
let mut client = NetcodeClient::new(Duration::ZERO, client_auth).unwrap();
let (client_packet, _) = client.update(Duration::ZERO).unwrap();

let result = server.process_packet(client_addr, client_packet);
assert!(matches!(result, ServerResult::PacketToSend { .. }));
match result {
ServerResult::PacketToSend { payload, .. } => client.process_packet(payload),
_ => unreachable!(),
};

assert!(!client.is_connected());
let (client_packet, _) = client.update(Duration::ZERO).unwrap();
let result = server.process_packet(client_addr, client_packet);
let mut client = NetcodeClient::new(Duration::ZERO, client_auth.clone()).unwrap();

match result {
ServerResult::ClientConnected {
client_id: r_id,
user_data: r_data,
payload,
..
} => {
assert_eq!(client_id, r_id);
assert_eq!(user_data, *r_data);
client.process_packet(payload)
}
_ => unreachable!(),
};

assert!(client.is_connected());
client_connect(&mut client, client_addr, user_data, &mut server);

for _ in 0..3 {
let payload = [7u8; 300];
Expand Down Expand Up @@ -818,6 +847,19 @@ mod tests {
}

assert!(server.is_client_connected(client_id));

// Test rejoin
server.update(Duration::from_secs(15)); // Client should be timing out

// Create new client with same connect token
let mut client = NetcodeClient::new(Duration::ZERO, client_auth).unwrap();
let (client_packet, _) = client.update(Duration::ZERO).unwrap();
let result = server.process_packet(client_addr, client_packet);

// The client should be disconnected now and the connection workflow should restart
assert!(matches!(result, ServerResult::ClientDisconnected { payload: None, .. }));
client_connect(&mut client, client_addr, user_data, &mut server);

let result = server.disconnect(client_id);
match result {
ServerResult::ClientDisconnected {
Expand Down