Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#2676 closed defect (fixed)

[PATCH] Prevent players in multiplayer games from sending commands for other players

Reported by: Ben Brian Owned by: JoshuaJB
Priority: Release Blocker Milestone: Alpha 19
Component: Network Keywords: patch
Cc: Patch:

Description

It shouldn't be possible for players to send commands on behalf of other players, unless there is a cheat/developer option enabled to allow it.

Attachments (2)

NetServer.cpp.patch (1.0 KB) - added by Ben Brian 5 years ago.
WIP patch
2676-patch-v2.patch (1.5 KB) - added by Ben Brian 4 years ago.

Download all attachments as: .zip

Change History (25)

Changed 5 years ago by Ben Brian

Attachment: NetServer.cpp.patch added

WIP patch

comment:1 Changed 5 years ago by Ben Brian

There's a WIP patch, it's quite simple, but it blocks all commands sent for other players, so it breaks the "change view" developer option. There should be some game setting where such options are allowed or not, and the server needs to be aware of that.

comment:2 Changed 4 years ago by elexis

Keywords: review added
Milestone: BacklogAlpha 19
Priority: Should HaveRelease Blocker

See #3552:

  • The developer overlay has been abused often for:
    • Defeating other players
    • Send fake chat
    • Quit the application of other players
  • Spectators can use it too and ruin one game after another without participating.
  • At least for rated games it must be prohibited (server-side as people hack their clients)
  • attachment:t3552_prevent_commands_for_others_in_rated_games_v1.patch:ticket:3552 ready for review
  • According to #3155, the developer overlay must always be enabled in non-rated games.
  • According to the code and nature of rated games, it should not be allowed in rated games.
  • Of all exploits, this one is the most prevalent and critical so far, so it must be fixed.

comment:3 Changed 4 years ago by Ben Brian

Priority: Release BlockerShould Have

It's not a new bug, so I don't see how it suddenly becomes a release blocker.

That said, I think there should be a game option to allow the developer overlay in multiplayer games. Like cheats, it will always be set false for rated games, otherwise it should default to true to match the current behavior (and help in debugging). Unfortunately I don't have the time to modify the UI and gamesetup to add that option, but I will attach a patch that uses the existing CheatsEnabled option, which I think addresses the above concerns.

Changed 4 years ago by Ben Brian

Attachment: 2676-patch-v2.patch added

comment:4 Changed 4 years ago by Ben Brian

FWIW I thought it would be a quick change to add a new option, unfortunately with the dynamic resizing of the options dialog and stored match settings, it's no longer so straightforward. But someone familiar with that code might be able to finish it for A19.

comment:5 in reply to:  3 ; Changed 4 years ago by elexis

Priority: Should HaveRelease Blocker

Replying to historic_bruno:

I don't see how it suddenly becomes a release blocker

This has been explained as referenced (in the dupe of this ticket and the other recent exploit tickets).

Again: I set #3552 to release blocker since we have multiple daily attacks since weeks that can't be stopped by banning. This is the most critical of the resulting recent exploit tickets (see also #3551).

Especially sending chat and closing 0ad for other players has never occured before.

In order to execute this exploit, the attacker doesn't even need to participate. Using observer mode he killed multiple games with an effort of some seconds.

We can do a more elaborate solution for a20/backlog, but hotfixing this for a19 is a must, especially since it can be fixed by changing one line (no matter which of the available patches we use).


dynamic resizing of the options dialog and stored match settings

See gamesetup.js/xml in r16945 for a simple example. Thanks for working on this.

comment:6 Changed 4 years ago by JoshuaJB

Owner: set to JoshuaJB
Resolution: fixed
Status: newclosed

In 17170:

Fix #2676 by only acknowledging commands matching the sender's name if cheats are disabled. Based on patch by historic_bruno.

comment:7 in reply to:  5 ; Changed 4 years ago by Josh

Keywords: patch review removed

Replying to elexis:

Replying to historic_bruno:

I don't see how it suddenly becomes a release blocker

This has been explained as referenced (in the dupe of this ticket and the other recent exploit tickets).

Again: I set #3552 to release blocker since we have multiple daily attacks since weeks that can't be stopped by banning. This is the most critical of the resulting recent exploit tickets (see also #3551).

Especially sending chat and closing 0ad for other players has never occured before.

In order to execute this exploit, the attacker doesn't even need to participate. Using observer mode he killed multiple games with an effort of some seconds.

We can do a more elaborate solution for a20/backlog, but hotfixing this for a19 is a must, especially since it can be fixed by changing one line (no matter which of the available patches we use).


dynamic resizing of the options dialog and stored match settings

See gamesetup.js/xml in r16945 for a simple example. Thanks for working on this.

The lobby is not considered an essential feature. Hence exploits of an auxiliary feature on a nonessential component are not alpha release blockers. I appreciate that you care about getting these issues fixed, but I would recommend taking a slightly broader perspective.

Regardless, I committed Ben's patch because it makes sense to just reuse the cheat setting. An additional setting would be clutter for our users and the majority of our developers. It also would have broken the string freeze.

Ben: Thank you for taking the time to patch this.

comment:8 in reply to:  3 ; Changed 4 years ago by leper

Priority: Release BlockerShould Have
Resolution: fixed
Status: closedreopened

Replying to historic_bruno:

That said, I think there should be a game option to allow the developer overlay in multiplayer games. Like cheats, it will always be set false for rated games, otherwise it should default to true to match the current behavior (and help in debugging).

As it is now this does not help in debugging since it is linked to cheats being enabled. Thus using it for debugging is not possible.

comment:9 in reply to:  7 Changed 4 years ago by elexis

Replying to Josh:

The lobby is not considered an essential feature.

To many players it is :P Non-humans are boring.

Replying to leper:

As it is now this does not help in debugging since it is linked to cheats being enabled. Thus using it for debugging is not possible.

How about enabling cheats if you want to debug a game? If not, why don't we allow the other cheats as well as they definitely are definitely helpful in debugging if you can phase quickly or do fast-actions.

Can making a new game-attribute be pushed to A20? Should we change this to the rating-setting instead of the cheat-setting? (The server will always correctly remember if it is a rated game).

Considering gamesetup.js/xml in r16945, making a new option should be quite easy. My concern is more having too many options.

comment:10 Changed 4 years ago by Lionkanzen

Elexis have right, the lobby is a huge feature, is the Multiplayer face. And multiplayer in most of cases even is more important than a campanig o storyline campaign.

comment:11 in reply to:  2 Changed 4 years ago by Josh

Blocked on #3579.

comment:12 Changed 4 years ago by elexis

Milestone: Alpha 19Alpha 20

If you want to change this back to rated games instead of cheats for a19, this could be committed: attachment:t3552_prevent_commands_for_others_in_rated_games_v1.patch:ticket:3552 instead of r17170.

String freeze means we can't have a new gamesetup option for a19.

comment:13 in reply to:  8 Changed 4 years ago by elexis

Replying to leper:

As it is now this does not help in debugging since it is linked to cheats being enabled. Thus using it for debugging is not possible.

That was the intent of the commit (as debugging and cheating are indistinguishable for that matter). Do you insist on having the dev-overlay enabled in non-cheated games? Also notice you can't debug rated games, so should we just allow it in any case?

We need a decision on this to continue with this ticket and #3551.

Introducing a new gamesetup option to enable/disable the dev-overlay doesn't make any sense as we could also just enable cheats in that case.

comment:14 Changed 3 years ago by elexis

Keywords: patch added

comment:15 Changed 3 years ago by elexis

In 17796:

Translate some untranslated strings, refs #3665.
Add a chat message if the control-all units cheat was used, refs #2676.

comment:16 Changed 3 years ago by Itms

Milestone: Alpha 20Alpha 21

comment:17 Changed 3 years ago by elexis

Component: Core engineNetwork

(set component to network)

comment:18 Changed 3 years ago by elexis

Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:19 Changed 3 years ago by scythetwirler

Keywords: beta added

comment:20 Changed 3 years ago by scythetwirler

Milestone: BacklogAlpha 22

comment:21 Changed 2 years ago by elexis

Priority: Should HaveRelease Blocker

Controling of enemy entities is entirely inacceptable to multiplayer games and I've never seen a developer that could use the feature to debug while actually playing. In order to find a bug, in every single case we had to use the replay to see what happened.

Revealing the map is the only thing that I've ever used constructively. Not for debugging but to find the last remaining units. For debugging we can still change the perspective in the GUI, even in rated games. Promotion should likely become a regular cheat, not a developer cheat (even if it means having to type more).

comment:22 Changed 2 years ago by elexis

Keywords: beta removed
Milestone: Alpha 22Alpha 19
Resolution: fixed
Status: reopenedclosed

Pointless to implement a gamesetup option to enable the developer overlay, since the developer can just enable cheats to enable cheats.

If the developer needs cheats to debug, he will likely need the other cheats as well (especially the phase, quick actions and resources ones), so it doesn't make any sense to have only N out of M cheats always available and others strictly forbidden.

Noone has ever missed this debug feature. If units are bugging in a game, the dev can still use the dev overlay to see the enemy units (change perspective). But it will be more important to watch the replay, since the events that caused the bug are crucial.

In todays staff meeting we have decided to link the control-all-units cheat to the cheats setting: #3551

Last edited 2 years ago by elexis (previous) (diff)

comment:23 Changed 2 years ago by elexis

In 19557:

Enable cheats in autostarted games because the JS gamesetup enables them in singleplayer and because the autostarted multiplayer mode relies on the change-perspective feature to control players.

Differential Revision: https://code.wildfiregames.com/D453
Refs #2676
Reviewed By: echotangoecho

Note: See TracTickets for help on using tickets.