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.

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

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

Open
MitchellMarinoDev opened this issue Nov 15, 2024 · 3 comments
Open

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

MitchellMarinoDev opened this issue Nov 15, 2024 · 3 comments

Comments

@MitchellMarinoDev
Copy link

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.

// TODO: this doesn't work with local hosts because the local bind_addr is often 0.0.0.0, even though
// the tokens contain 127.0.0.1

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:

pub(crate) trait Transport {
/// Return the local socket address for this transport
fn local_addr(&self) -> SocketAddr;

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

@MitchellMarinoDev
Copy link
Author

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.

@cBournhonesque
Copy link
Owner

Thanks for the issue, I think everything you said makes sense!

@RJ
Copy link
Contributor

RJ commented Nov 22, 2024

btw, when you deploy game servers to Edgegap you listen on 0.0.0.0 and you have to retrieve your actual public IP from an environment variable or api call. Edgegap maps a high numbered public port onto your listening port.

please consider this if you add a server ip check to the connect token logic.. as long as you can specify the IP that connect token uses to check, rather than just trying to enumerate addresses on the machine, it should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants