search

cBournhonesque / lightyear

A networking library to make multiplayer games for the Bevy game engine
https://cbournhonesque.github.io/lightyear/book
Apache License 2.0
490 stars 49 forks source link

Server doesn't check ConnectionToken's server address #692

Open MitchellMarinoDev opened 5 hours ago

MitchellMarinoDev commented 5 hours ago

What

When receiving a connection request packet, the server does not check the "server addresses" part of the connect token. The code that does so is commented out.

https://github.com/cBournhonesque/lightyear/blob/7d714079a540f3d0233a084de5a0603c7fc5caa9/lightyear/src/connection/netcode/server.rs#L567-L568

We cannot just use the UdpSocket address, as that would return the local ip, whereas we should be checking against the public ip of the server, as that is what the client would be connecting to. On LAN servers, the local ip would also play the role of the public ip. Therefore, in order to implement this, we would need to add a value in the server config for the public server address.

Renet has something like this: https://github.com/lucaspoffo/renet/blob/ffdcb1a2b0d509f73670aa0d46a9011ec9f61876/renetcode/src/server.rs#L109-L110

Why

This check is necessary for ensuring that the connection token that was generated was meant for this server. Otherwise someone could obtain a connection token for a specific server and use it on any server (not good). See the spec for more. Specifically the line,

If the dedicated server public address is not in the list of server addresses in the private connect token, ignore the packet.

Things to Consider

Right now the lightyear tries to keep things "transport-agnostic", so it might not be a good assumption that the transport will use IP addresses. For instance the LocalChannel enum varient of ClientTransport. However, in the Transport trait, there is a method for obtaining the address, so perhaps it is an okay assumption. See: https://github.com/cBournhonesque/lightyear/blob/7d714079a540f3d0233a084de5a0603c7fc5caa9/lightyear/src/transport/mod.rs#L73-L75

Perhaps we could still allow the user to bypass this check in the case of a transport that doesn't have ip addresses.

MitchellMarinoDev commented 4 hours ago

I can confirm this issue exists by hosting a server on port 6000, setting up a local relay for UDP traffic with the command socat UDP4-LISTEN:6001,fork UDP4:127.0.0.1:6000 and then connecting on the client to 6001. The server lets this connection through, even though the Auth specified the address 127.0.0.1:6001.

Implementing this would have breaking changes to the ServerConfig struct. I would be willing to try to take a stab at it, though I'm not sure if I understand the codebase enough to do so.