#2676 closed defect (fixed)
[PATCH] Prevent players in multiplayer games from sending commands for other players
Reported by: | historic_bruno | 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)
Change History (25)
by , 10 years ago
Attachment: | NetServer.cpp.patch added |
---|
comment:1 by , 10 years ago
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.
follow-up: 11 comment:2 by , 8 years ago
Keywords: | review added |
---|---|
Milestone: | Backlog → Alpha 19 |
Priority: | Should Have → Release 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.
follow-ups: 5 8 comment:3 by , 8 years ago
Priority: | Release Blocker → Should 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.
by , 8 years ago
Attachment: | 2676-patch-v2.patch added |
---|
comment:4 by , 8 years ago
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.
follow-up: 7 comment:5 by , 8 years ago
Priority: | Should Have → Release 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.
follow-up: 9 comment:7 by , 8 years ago
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.
follow-up: 13 comment:8 by , 8 years ago
Priority: | Release Blocker → Should Have |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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 by , 8 years ago
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 by , 8 years ago
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:12 by , 8 years ago
Milestone: | Alpha 19 → Alpha 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 by , 8 years ago
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 by , 8 years ago
Keywords: | patch added |
---|
comment:16 by , 8 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
comment:19 by , 7 years ago
Keywords: | beta added |
---|
comment:20 by , 7 years ago
Milestone: | Backlog → Alpha 22 |
---|
comment:21 by , 7 years ago
Priority: | Should Have → Release 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 by , 7 years ago
Keywords: | beta removed |
---|---|
Milestone: | Alpha 22 → Alpha 19 |
Resolution: | → fixed |
Status: | reopened → closed |
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
WIP patch