raduprv / Eternal-Lands

http://www.eternal-lands.com
Other
157 stars 57 forks source link

Encrypted connections #132

Closed gvissers closed 3 years ago

gvissers commented 3 years ago

This is not so much a pull request, but more of a way to open the discussion on encrypting the communication between the client and the server. I'd like to explicitly invite @raduprv to participate, as there is a server side to consider. I have discussed encrypting the network traffic in-game with multiple people, and most, if not all, seem to support the idea. A request for encrypted communication with the server has also been posted previously on the forums.

So, as a starter for discussion, I have implemented client-side support for how I envision this could be implemented in this branch. Note that this code is not ready for merging: it definitely needs documentation and perhaps some polishing, we need to not blindly send login details on reconnect, and we need to figure out how to detect whether the server supports encryption, but the basics are there. I have tested it with a simple toy server, that does not really do anything but set up the connection and send an error message when you try to log in, but at least that error message cannot be read by anyone but the client :wink:

I have tried to make the encryption optional, both on the client side and on the server side, using the following simple scheme:

To implement it the way I have, I have had to do away with SDL_net[2], as I need access to the underlying file descriptor which SDL_net does not provide. So the client now has its own Socket class. I have also moved the networking part out of multiplayer.c into a new Connection class to keep the associated state better contained.

I have tested this code with the current server, and it works (though unencrypted, of course). As I wrote before, I have tested the encryption with a simple toy server, and that works too, though obviously I do not yet know how well it works in real game play. There are still some things to think about:

[1] I would have increased the major number, to get 11.0, but the current server implementation is buggy in that it checks both parts of the version separately, so that you are disconnected if either part of the number is too low. [2] SDL_net is still used in other parts of the client, e.g. the update code. If this goes anywhere, we might want to ditch SDL_net altogether and use the clients own Socket code everywhere.

raduprv commented 3 years ago

Changing to SDl_net 2 alone is more trouble than it's worth. Not only this requires everything else to be changed to sdl2, but some other networking code, such as the implementation of freebsd kqueue and other things I haven't even thought of. And I am not sure what the benefit would be, after all, if some malicious 3rd party wants to do something they would most likely gain local access to the client via social engineering.

EL has been on for 18 years now, and we never had a problem with a man in the middle attack.

gvissers commented 3 years ago

I'm not sure I understand your post correctly. I'm not proposing to upgrade to SDL_net 2, I propose to remove the dependency on SDL_net altogether and use Berkeley sockets directly. They are widely supported, and as far as I can see, SDL_net itself is nothing more than a thin abstraction layer around them. A nice abstraction layer, but it does get in the way if you want to do anything with the underlying socket. Besides, if the world ever upgrades to IPv6, you'd have to upgrade or ditch SDL_net anyway.

As for the benefits: I certainly would feel better if I knew the game traffic could not be snooped upon. And not only my game password, but also in game chat. E.g. I would like to keep my PMs just that: private. Sure, I know that it's easier to obtain a password by social engineering than setting up a MITM attack. But why keep the possibility open? Heck, you wouldn't even have to do all that much, I would do the coding for you.

EL has been on for 18 years now, and we never had a problem with a man in the middle attack.

Good! Let's keep it that way!

pjbroad commented 3 years ago

I'd just like to add that I'd really like this feature to and would be willing to help client/server in any way. I've always considered this an issue and it has stopped me playing on public wifi in hotels etc, where it would be very easy to snoop the traffic.

raduprv commented 3 years ago

The proposed implementation is nearly worthless. If there is a man in the middle, he can just intercept the original packet, and then replace the version number with a lower one, so the server would not send a let's encrypt message. The only way to make it secure would be for the encryption to be mandatory. Which would at the very least require all the bots to have changes in their code (not to mention an important chunk of the server).

gvissers commented 3 years ago

Thanks for taking a serious look at this proposal, and poitning out the (in hindsight rather obvious) flaw! (BTW, I think the changes to the server code would be less intrusive than you think, but that's beside the point).

So here's anoher idea: how about we add a simple proxy server on the server machine, that sets up an encrypted connection with the client (mandatory, from the first byte on), and tunnels traffic to the game server program? That way unencrypted traffic never needs to leave the server machine. We would only have to add an extra socket to listen on localhost to the game server code (or a pipe, but an extra TCP socket might be easier to implement), bots can keep on working the way they do now, and people can opt out of encryption when they want to.

raduprv commented 3 years ago

That's a better idea. I'll think about it :)

raduprv commented 3 years ago

Ok, so I talked to Learner a bit. We could host the proxy on one of his servers first, as a test (they are located in the same building with the EL server). He can give you access to his proxy server, so you can extend it. You'd also have to coordinate with Bluap about the client side, see what he thinks about dropping SDL_net and such.

raduprv commented 3 years ago

Btw, if you want to switch to raw sockets, you have to provide the implementation for all the supported OSes. I think the UNIX based ones (Linux, BSD, Android and OSX) should use the same code, or small variations, but the Windows one is a bit more different.

gvissers commented 3 years ago

Ok, great! I'll contact Learner for the proxy server, then.

You'd also have to coordinate with Bluap about the client side, see what he thinks about dropping SDL_net and such.

Yes, of course bluap will be involved, he's been successfully guarding the client code for years.

Btw, if you want to switch to raw sockets, you have to provide the implementation for all the supported OSes. I think the UNIX based ones (Linux, BSD, Android and OSX) should use the same code, or small variations, but the Windows one is a bit more different.

Looking at SDL_net, it looks like for windows they use Berkeley sockets as well, perhaps with some small variations for things that are not supported on certain OSs (TCP_NODELAY comes to mind, there may be others). Anyway, we'll make sure everything works well on Windows.

gvissers commented 3 years ago

Okay, Learner has graciously given me access to the proxy machine and source code, and I was able to adapt the SDL_net library used by Learner's proxy to use encrypted connections. I was able to log in on the EL test server through the encrypted proxy, so that part is looking good. Of course that needs further testing, but I need to beat the client side into shape a bit further.

Which brings me to the next point: how should we determine if the connection should be encrypted? I see a few options:

None of these appeal very much to me, though the first is probably the easiest. If anyone has a better idea, I'd love to hear it.

And finally:

The proposed implementation is nearly worthless. If there is a man in the middle, he can just intercept the original packet, and then replace the version number with a lower one, so the server would not send a let's encrypt message.

That's not entirely true. The client would see that either the connection to the MITM is not encrypted, or that the certificate is not valid.

raduprv commented 3 years ago

That's not entirely true. The client would see that either the connection to the MITM is not encrypted, or that the certificate is not valid.

Yes, the client could signal in some way that the connection is encrypted, but unless if it gives a big warning stating that the connection is not encrypted it will be easy to miss or completely disregard if you haven't used encryption before.

We add a field in servers.lst. Unfortunately we can't just add a field at the end as the current last field is the description which is parsed as everything from the start of the last field until the end of the line. So distinguishing between old and new servers.lst is a bit difficult. However, we need new data files for the encryption anyway (accepted certificates).

Maybe something added in the description, such as [encrypted].