Security bugs with observer mode

[roncli]

Hopefully @nightwish75 can provide details, if not we'll have to go through netlogs.

The following security bugs have been observed and need to be fixed:

• [x] Being able to use F8 with a name other than their pilot name they joined as. (Need to check if this is possible for normal players as well.)
• [x] Being able to chat with the host.
• [x] End games prematurely.

I'm certain this isn't an exhaustive list, but anything we can add to the list is helpful.

[roncli]

From Icewolf/Nightwish:

These are not really bugs, or if we consider those as bugs then we should name entire netcode a bug.

• the game does not check validity of the incoming packets for example: player1 sends /killreactor packet, other clients wont verify if it came from player0 (preferably by ip address not just signature)
• restrictions (host rights) are enforced locally only in the client's game - removing these restrictions make everybody 'a host administrator'.

These two facts mean that if someone can build custom code (with several 'if' statements removed) then that person is able to do ANYTHING to screw up the game for everybody - now even as an observer.

This way its possible to send messages with false signature, kill reactor, end game, even produce weapon projectiles in front of a player killing him and posing as it was valid kill, screw up scoreboard for everybody (except the host)...etc. imagine...

Now as I recall roncli is planning to show shield amounts for observers, I don't remember if this information is being sent to all players now, but what if that will enable someone to use custom build to see these values when playing?

Tampering with code in a way as I mentioned above doesn't make much sense in organized games, but as anonymous observer...well it apparently does.

Quick solution would be 'restricted observers' - this way host has to 'let observer in' - same as restricted game host lets another player in.

Hope it helps

Nightwish75

[roncli]

"These are not really bugs, or if we consider those as bugs then we should name entire netcode a bug."

You'll get little argument from me. It seems every time I'm in the source I see something that makes me facepalm. :) However, if there's something we can do to reduce surface area for cases like this, the better. The community is stronger when we figure these things out and work together.

the game does not check validity of the incoming packets for example: player1 sends /killreactor packet, other clients wont verify if it came from player0 (preferably by ip address not just signature)

It appears that the code to check whether or not the UPID_MDATA packets only come from the IP expected is now only active when the Retro network protocol is NOT active, as of Retro 1.3X6. There's probably a good reason for this. @CDarrow, might you remember why this is the case?

https://github.com/CDarrow/DXX-Retro/blob/v1.4.0-x.6/d1/main/net_udp.c#L5580-L5599

restrictions (host rights) are enforced locally only in the client's game - removing these restrictions make everybody 'a host administrator'.

Hardening the code to restrict a potential malicious user from tampering with a game in progress is difficult because of the fact that we are open source. We've seen blarget show this off with his quad megas and other various hacks to the game. Unfortunately there's not a whole lot that we can do about this side of things. The most obvious is that someone could come along simply remove all Players[Player_num].shields -= lines and render themselves invincible.

With our small community, I don't think Retro should be expected to fix this. Generally, we like to trust that those within the community are going to follow the Wil Wheaton rule (DBAD) and not intentionally mess with games in this way.

Now as I recall roncli is planning to show shield amounts for observers, I don't remember if this information is being sent to all players now, but what if that will enable someone to use custom build to see these values when playing?

We already are. :) However, we are aware that this is a security issue, as the host has to process all the shield numbers. I do have an idea for a fix, which I've left as an open question for Drakona and Adam in the original Jinx mode issue #13.

Quick solution would be 'restricted observers' - this way host has to 'let observer in' - same as restricted game host lets another player in.

That's honestly not a bad idea. I'll add it as a enhancement request.

[CDarrow]

I don't specifically remember turning off the IP checks. That sounds like a bug. We had someone sending reactor kill packets to games in progress and I was quite concerned about the issue. Maybe something was wonky with the punchtrough algorithm? Or maybe the proxy fallback necessitated it? I dut. Sounds like a mistake to me. Maybe I'm forgetting something.

The most straightforward way to prevent security problems with observer mode is to drop packets from player 8 in an observed game. I'm surprised I didn't already do that. I sure thought I did. Observers should only be talking to the game host, and only to join and respond to heartbeats. Restricting observers by username seems silly to me - restriction now is a courtesy, not a security feature. I think password protected games are an obvious first step if needed, if we really have a persistent miscreant, potentially involving the tracking to avoid exposing the game's address to uninvited parties.

Once in the game, clients are and should be trusted. The federated nature of the game is pretty deep in the DNA of both the code and the community - it's not changing now. However, I have occasionally toyed with the idea of putting cheat detection of various degrees of sophistication in the clients. Mostly, though, I don't think there is a deep need for this sort of thing, and social solutions are more the way we do things.

I also stressed about the information relayed by sending shield info to the host, but I eventually gave up when I considered that position information is more valuable and we've sent that forever.

-Drak

On Feb 11, 2018 6:47 PM, "Ronald M. Clifford" notifications@github.com wrote:

"These are not really bugs, or if we consider those as bugs then we should name entire netcode a bug."

You'll get little argument from me. It seems every time I'm in the source I see something that makes me facepalm. :) However, if there's something we can do to reduce surface area for cases like this, the better. The community is stronger when we figure these things out and work together.

the game does not check validity of the incoming packets for example: player1 sends /killreactor packet, other clients wont verify if it came from player0 (preferably by ip address not just signature)

It appears that the code to check whether or not the UPID_MDATA packets only come from the IP expected is now only active when the Retro network protocol is NOT active, as of Retro 1.3X6. There's probably a good reason for this. @CDarrow https://github.com/cdarrow, might you remember why this is the case?

https://github.com/CDarrow/DXX-Retro/blob/v1.4.0-x.6/d1/ main/net_udp.c#L5580-L5599

restrictions (host rights) are enforced locally only in the client's game - removing these restrictions make everybody 'a host administrator'.

Hardening the code to restrict a potential malicious user from tampering with a game in progress is difficult because of the fact that we are open source. We've seen blarget show this off with his quad megas and other various hacks to the game. Unfortunately there's not a whole lot that we can do about this side of things. The most obvious is that someone could come along simply remove all Players[Player_num].shields -= lines and render themselves invincible.

With our small community, I don't think Retro should be expected to fix this. Generally, we like to trust that those within the community are going to follow the Wil Wheaton rule (DBAD) and not intentionally mess with games in this way.

Now as I recall roncli is planning to show shield amounts for observers, I don't remember if this information is being sent to all players now, but what if that will enable someone to use custom build to see these values when playing?

We already are. :) However, we are aware that this is a security issue, as the host has to process all the shield numbers. I do have an idea for a fix, which I've left as an open question for Drakona and Adam in the original Jinx mode issue #13 https://github.com/CDarrow/DXX-Retro/issues/13.

Quick solution would be 'restricted observers' - this way host has to 'let observer in' - same as restricted game host lets another player in.

That's honestly not a bad idea. I'll add it as a enhancement request.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/CDarrow/DXX-Retro/issues/100#issuecomment-364812103, or mute the thread https://github.com/notifications/unsubscribe-auth/AA1XBXz5Nv802UDYud6e_B7BcWe0cgAAks5tT5gtgaJpZM4R9kpZ .

[roncli]

Thanks for the reply! :) Based on this, I'd be willing to bet 1.3X6 was when the Retro protocol was introduced, and there was a mass "if away everything old-protocol-related". I'll test putting that back in and see what happens, that may be enough to put this issue to rest.

I can't find anything about the master only allowing certain packets from the observer via player ID. What the master won't do is forward packets to other players from the observer.

Both restricting observers like players in a restricted game and password protecting games pretty much gets you almost the same thing. The only difference is that the tech for restricting players is already in place. Depends on which method we'd like to take, perhaps more input from the community is needed. But yes, neither of these enhancements would be a network protocol security feature, which is what the issue is here.

And I 100% agree with trusting clients once connected.

Yeah, having location information is more valuable than shields, although knowing your opponents shields has its value too.

[nightwish75]

Disabling if statement checkin 'am i the master' thing does allow using /killreactor and other host commands on the client - checked in both retro 1.3 and 1.4x6. Trusting active game client is ok, trusting observer not really - this was my point unless observers are 'restricted' as I suggested. What if instead using these 'if's' in the client we use incoming packet verification? Instead of "If I'm the host so I can do this" use "oh killreactor packet arrived lets see if it was sent from host IP and if it was lets kill the reactor". With open source not much can be done to prevent host from cheating but, clients are another story - yet this can only be achieved with full host authority - I understand that. 'Community is small' argument doesn't sound good. "Think small - be small", unless we all want to be small? Just loose thoughts.

[roncli]

Disabling if statement checkin 'am i the master' thing does allow using /killreactor and other host commands on the client - checked in both retro 1.3 and 1.4x6.

Trusting active game client is ok, trusting observer not really - this was my point unless observers are 'restricted' as I suggested.

What if instead using these 'if's' in the client we use incoming packet verification?

Instead of "If I'm the host so I can do this" use "oh killreactor packet arrived lets see if it was sent from host IP and if it was lets kill the reactor".

Retro used to do packet verification. That code got accidentally short-circuited in 1.3X6, which means the packet verification is no longer present in 1.3X6 and after. 1.3 and 1.4X6 are both after 1.3X6. Remember that "X6" is basically "beta 6". I'll be testing reinstating that code soon.

As far as a client player goes, that's another issue entirely, which we've already decided to trust that player and handle any issues that come up outside of the game itself.

'Community is small' argument doesn't sound good. "Think small - be small", unless we all want to be small?

Want has little to do with it. :( The reality is that the community is small, and that's not going to change any time soon. 23 years on, I don't think we can expect thousands of people coming back to play competitively. D:U's Kickstarter was the best chance at a surge of new players we may ever see again, and that netted maybe a couple dozen players, and most of those only short term at that. The fact that this game is even being played competitively at all in 2018 is amazing. The fact that it's also the best competitive game in its genre is astoundingly silly, hopefully that changes with at least ONE of these newer games.

[roncli]

OK, so putting that code back in works for disallowing bad IPs, but it kicks out observers for not sending ACKs, because the host is filtering them out, because observers are not players. I'm going to have to change the code to also allow observers, and then write a filter that lets observers do only certain things.

[roncli]

I just spent some time to grind this one out, it is fixed and ready to go.

I think it's still possible for players with a self-compiled .exe to send messages as other players. I will look into changing how messages are sent at another time.