26

Re: Patching current vulnerabilities and moving on to 0.7.0

Thanks, Oy & Magnet, for working on this.

If there is anything know-nothings like me can do to help, please say so. For now I will continue flailing my arms helplessly.

"I'm in the bathroom!" ---Albert Einstein

27

Re: Patching current vulnerabilities and moving on to 0.7.0

Oy wrote:

Probably best to just release 0.7 and then go on from there. Will check/fix crucial stuff and then release it this weekend. Anyone is invited to test and report current bugs.

I'll push some map updates in the official repo then

28

Re: Patching current vulnerabilities and moving on to 0.7.0

Stitch626 wrote:

And what about slopes? There was a demo code which actually worked. There arent any big improvements over the past years (except the new skin system).. I'd like to see slopes! It would definitely improve the game!

I'm sure there are a lot of features we would like to see in 0.7.0, but surely this is by no means a final release. A release as-is has been long due and would never get done if we expect everything to be complete by the next milestone.

Not Luck, Just Magic.

29

Re: Patching current vulnerabilities and moving on to 0.7.0

@lucid.dreaming So you've left the dark side?

OI2 - Official Infclass 2.0: https://discord.gg/Sxk5ssv

30

Re: Patching current vulnerabilities and moving on to 0.7.0

Stitch626 wrote:

@Magnet / @ heinrich5991: Don't you think it would be better to finally release 0.7 with better standards instead of bugfixing a now 7 year old release? (0.6 was released in 2011...)

I welcome efforts to finally release 0.7, but realistically, 0.6 stuff will keep running for a year or so at least, so it's important to also fix 0.6.

31

Re: Patching current vulnerabilities and moving on to 0.7.0

Stitch626 wrote:

And what about slopes? There was a demo code which actually worked. There arent any big improvements over the past years (except the new skin system).. I'd like to see slopes! It would definitely improve the game!

Edit: What about 0.5/0.4? Thats now definitely dead, will tw itself drop the support for those old versions? I mean, if you really improve the master server security, you should/could drop the support for older versions (it would otherwise create a new hole).

0.7 will be what's already there in the master branch - so no slopes.
And the master just supports 0.7 and no older versions.

Sonix wrote:

I'll push some map updates in the official repo then

That would be great!

Remember the 80s - good times smile

32

Re: Patching current vulnerabilities and moving on to 0.7.0

@Stitch626 Okay hmm i understand.

@WaterMalone LOL, yes i have left the dark side, it is a waste of time and energy. I am glad teeworlds will be better.

33

Re: Patching current vulnerabilities and moving on to 0.7.0

hype

34

Re: Patching current vulnerabilities and moving on to 0.7.0

@Oy okay, but currently you are spending one master for 0.7 only, 2 or 3 for 0.6 and one is able to manage 0.4/0.5... thats why I wrote that ^^

35

Re: Patching current vulnerabilities and moving on to 0.7.0

Are you sure all issues are fixed in 0.7?

Besides,the correct way to fix something like that isn't always a 3 way handshake, just ensuring that no confirmed ip addresses can only get smaller responses than requests would do it as well (which one is the right one depends entirely on the use case)

Oy, if you allow it, I would like to review the 0.7 protocol code for some basic vulns like those before an initial release. (at least look for design errors that would need breaking changes).

Having troubles finding servers in the serverlist? Go to Pastebin (its a referer cause there is daily a new pastebin) and add the lines to your settings.cfg (in %APPDATA%\teeworlds). Then open teeworlds and go to the favorites tab. (Note however, standard teeworlds client can only show 256 favorites, use ddnet instead)

36 (edited by Schwertspize 2018-10-11 18:27:32)

Re: Patching current vulnerabilities and moving on to 0.7.0

So....

Version server is vulnerable, to what extent is not clear right now, however this is a minor issue as it is not working anyway (version.teeworlds.com for example doesn't even resolve to an address). Hence i mostly skipped this part.

master server has a few obvious vulnerabilities that can be fairly easily resolved.
  -- SERVERBROWSE_GETCOUNT is vulnerable to amplification. simple fix: enlarge SERVERBROWSE_GETCOUNT by at least 2 bytes. (or make it a struct with two empty bytes whose existence are checked but not the values (only size is checked)
  -- Heartbeat checking is prone to amplification. i'd suggest making the try count amount of tries a constant and enforcing SERVERBROWSE_HEARTBEAT size of MAX_TRIES*sizeof(SERVERBROWSE_FWCHECK). Alternatively, enforce a size of any*sizeof(SERVERBROWSE_FWCHECK) and use it like "credits", if the request is 4*8 bytes large, you allow up to 4 check packets, if its 32*8 bytes large, you allow for 32 checks. (however "used" credits must not be re-used, additional heartbeat checks must only be performed if enough "traffic credits" are there. just an example. (as you see, the basic idea is never sending out more bytes than you received from a given address)
  -- UPDATE: Probably a better alternative would be a three way handshake here as connections to the master server are long-lived so once a connection is secured with a token, it can stay that way and you don't waste traffic.
  -- SERVERBROWSE_GETLIST is vulnerable. As the client determines the server count anyway, a simple approach would be to enlarge the GETLIST packet to SERVERCOUNT*SERVERINFO bytes (and headers are equal size, however note multiple packets would be sent in response, so (SERVERCOUNT*sizeof(SERVERINFO))+sizeof(HEADER)*num_packets_for_SERVERCOUNT. the master server would store this value and cut off the response if GETLIST was unsufficiently large.

Additionally i would like to re-think the anti spoof tokens. game traffic should ideally be authenticated and encrypted if this is possible with low ping penalties. i'd prefer using either authenticated encryption (AES-GCM) or (packet+hmac(token,packet)) instead of just (packet+token). However, this is not so much a problem for the master server.

Thats my thoughts about the master server. The reasons i picked simply enlarging packets instead of a complicated handshake is that a handshake is a bit much overhead for short lived "connections" such as fetching a serverlist once.
UPDATE: for server registration and heartbeat i recommend an actual handshake. this connection is long-lived.

updates on game protocol will follow.

Having troubles finding servers in the serverlist? Go to Pastebin (its a referer cause there is daily a new pastebin) and add the lines to your settings.cfg (in %APPDATA%\teeworlds). Then open teeworlds and go to the favorites tab. (Note however, standard teeworlds client can only show 256 favorites, use ddnet instead)

37

Re: Patching current vulnerabilities and moving on to 0.7.0

Schwertspize wrote:

-- SERVERBROWSE_GETCOUNT is vulnerable to amplification. simple fix: enlarge SERVERBROWSE_GETCOUNT by at least 2 bytes. (or make it a struct with two empty bytes whose existence are checked but not the values (only size is checked)

Would result in an equal size for incoming and outgoing packets. But whether the COUNT packet of the size is really used sensibly for attacks remains questionable. You don't get a lot of traffic generated with it to make it worthwhile for amplification attacks.


Schwertspize wrote:

Additionally i would like to re-think the anti spoof tokens. game traffic should ideally be authenticated and encrypted if this is possible with low ping penalties. i'd prefer using either authenticated encryption (AES-GCM) or (packet+hmac(token,packet)) instead of just (packet+token). However, this is not so much a problem for the master server.

AES is a little overhead. With the exception of the rcon password or the server password, Teeworlds does not provide any critical data, which must be encrypted. Besides, I wonder what the key should be for this encryption? The key cannot be transferred unencrypted.

The hmac would prevent a packet manipulation. However, the token is also transferred unencrypted and you could calculate a new hmac with this token.

Would be good if you would explain this further.

38 (edited by Schwertspize 2018-10-12 18:51:24)

Re: Patching current vulnerabilities and moving on to 0.7.0

Yes, it would be unlikely to be abused, but still, simple fix, just to make sure.

Encryption for the sake of doing it. If it does not cost a lot of time (especially compared to hmac, since aes gcm is authenticated encryption). Hmac would prevent the token to be actually transferred (if you ignore key exchange). And finally, key exchange can be done securely using (ephemeral) keys.

Thinking ahead, this could allow for the client checking server integrity (using the server public key for example), if wanted. It could surely allow for securely identifying and recognizing individual clients, making better rcon access control as well as maybe server passwords possible (or: use password once, server recognizes you). Additionally, if not wanted, ephemeral one time keys can be used for key exchange.

As this all happens during initial connection, overhead wouldn't be so much of a problem. For me, I would discuss those things a lot, but no matter what you choose, I vote for added security. The main reason for packet authentication here is by the way to prevent address spoofing, if every packet is signed (or authenticated by other means) you are sure it's from its original owner.

EDIT: just a side note after i took a quick look at the current token code. MD5 is unsafe, however that isn't even the limiting factor as the protocol only reserves 20 bits for a token. yes, there is only 1 million different tokens, which can be brute forced in very short time (as there is for example no penalty on sending wrong tokens), especially as this could be prone to timing attacks, reducing the necessary attempts a lot.

Having troubles finding servers in the serverlist? Go to Pastebin (its a referer cause there is daily a new pastebin) and add the lines to your settings.cfg (in %APPDATA%\teeworlds). Then open teeworlds and go to the favorites tab. (Note however, standard teeworlds client can only show 256 favorites, use ddnet instead)

39

Re: Patching current vulnerabilities and moving on to 0.7.0

I basically agree with most of what Schwertspize said.

My proposed solution to most of the problems:

Initial connect/get token packet must be at least 512 bytes (e.g. as implemented in the fix-0.6 branch), so as to completely make reflection worthless.

HTTPS (definitely with S) for the master server list, preferably with all the server infos already (would also make the refresh faster)

Tiny nit: I agree that there's little reason to use md5 for anything, but I don't think it's unsafe in the presence of a sufficiently random input, against preimage attacks.

40

Re: Patching current vulnerabilities and moving on to 0.7.0

For that tiny nit. I don't regard md5 as the biggest problem. Packets are relatively small, and reserving only 20 bits for packet authentication makes it entirely possible to send all the million packets within 10 seconds until token rotation. Or, if that's just barely not possible, you retain a high percentage of win and will probably crack the next seed. As for https,as much as I despise it (it adds _a lot_ of unnecessary bloat instead of just using DTLS or TLS for example), i see that it would allow far better load balancing and reliability.

As for protocol, I would favor a simple approach. No heartbeat checking as that can be worked around quite easily. No server info as it would greatly limit teeworld's openness. Alternate serverlists? Must be supported by the individual server. Favorite list just storing ip addresse (or domains)? Nop. Custom server info for custom clients? Nop.
I would suggest a protocol that consists of a simple post request to put your server in the masterlist for say 1 or 5 minutes. The master server checks if the given ip matches the source ip, or if a domain was given, that domain resolved to the server. Get provides a list of addresses.

Having troubles finding servers in the serverlist? Go to Pastebin (its a referer cause there is daily a new pastebin) and add the lines to your settings.cfg (in %APPDATA%\teeworlds). Then open teeworlds and go to the favorites tab. (Note however, standard teeworlds client can only show 256 favorites, use ddnet instead)

41

Re: Patching current vulnerabilities and moving on to 0.7.0

So far ctf5, ctf8, sur1, dm7, dm3 are ready for release, I'll do a general checkup tonight before adding them to the maps repository.
The rest of the grass tiled maps were converted to 0.7 tileset with some possible tiny aesthetic changes (doodads mostly).

42

Re: Patching current vulnerabilities and moving on to 0.7.0

Posts relevant to patching the vulnerability and forward development were split and moved here.

Not Luck, Just Magic.

43

Re: Patching current vulnerabilities and moving on to 0.7.0

Rip. Most vulnerabilities were _not_ fixed.

Having troubles finding servers in the serverlist? Go to Pastebin (its a referer cause there is daily a new pastebin) and add the lines to your settings.cfg (in %APPDATA%\teeworlds). Then open teeworlds and go to the favorites tab. (Note however, standard teeworlds client can only show 256 favorites, use ddnet instead)

44

Re: Patching current vulnerabilities and moving on to 0.7.0

Schwertspize wrote:

Rip. Most vulnerabilities were _not_ fixed.

From my understanding, they cannot be fixed without breaking compatibility/moving to 0.7, right?

Not Luck, Just Magic.

45

Re: Patching current vulnerabilities and moving on to 0.7.0

Schwertspize wrote:

Rip. Most vulnerabilities were _not_ fixed.

That doesn’t mean they cant be fixed anymore :thinking:

46

Re: Patching current vulnerabilities and moving on to 0.7.0

Dune wrote:

From my understanding, they cannot be fixed without breaking compatibility/moving to 0.7, right?

I believe Schwertspize comments on how they weren't fixed in 0.7, so they cannot be fixed without breaking compatibility/moving to 0.8.

Sonix wrote:

That doesn’t mean they cant be fixed anymore :thinking:

See above.

47

Re: Patching current vulnerabilities and moving on to 0.7.0

Is there already a date for 0.8?

I will be banned if I troll again ...

48

Re: Patching current vulnerabilities and moving on to 0.7.0

Thanks Heinrich. The whole point of this rushed 0.7 release was to fix the security problems. Apparently he just fixed (or Idk if fixed) the status packet, but what about all the rest? We lived with those attacks for a long time, I'd rather have waited a week, maybe two or three if it involves serious discussion about the actual next protocol to have a stable, secure and maybe even feature enhanced version. Not a rushed hot fix this now, don't care about anything else.

Having troubles finding servers in the serverlist? Go to Pastebin (its a referer cause there is daily a new pastebin) and add the lines to your settings.cfg (in %APPDATA%\teeworlds). Then open teeworlds and go to the favorites tab. (Note however, standard teeworlds client can only show 256 favorites, use ddnet instead)