0001426: Make ConsoleCommand whitelist-based

(0006762)
Dusk (developer)
2013-07-23 00:00

I wonder, should this be a public ticket? Could get some help building the possible white-list that way.

(0006764)
Blzut3 (administrator)
2013-07-23 06:53

I see no reason this has to be private. In fact, I think it would be better that it isn't as it serves as a good example for all the people who think that consolecommand is harmless. :P

As you can probably guess, I'd be all for a white list or even making it only available as a compat flag.

(0006768)
Dusk (developer)
2013-07-23 12:07

Made public. I don't think that compatflagging is sufficient since a mod could just say it needs compat_acsconsolecommand true or something like that and people who have no idea what this even is would just activate it, and we have the same problem all over again. IMO it should be completely torn out.

So yeah. I think a list of things we should allow for now...
- set... maybe only if the cvar being set is mod-generated? Also AOW and WDI auto-adjust DMFlags and nobody complains.
- archivecvar, same rationale as above
- puke? Might be needed to get some info from clients to servers...
- kick? I don't know any legit uses off-hand though

(0006772)
ZzZombo (reporter)
2013-07-23 14:47

Kick a troll armed a C4/Ion/Nuke in his base.

(0006777)
Konar6 (reporter)
2013-07-23 16:59

The attached snippet is part of a wad which one user was trying to host on servers. The full wad is here ->'http://sickedwick.net/k6/9ab7f5118db665ff6e2cc7dea7d4e2e9/surprise_me_v1.pk3_ [^]' (WARNING: don't run it, it's a malware that will screw your config if you do; I've renamed it to prevent accidental execution).

This shows how dangerous ConsoleCommand still is, despite of removing specific reported commands one by one. It's simply not enough even in the short run to foresee and catch all possible misuse. I would also like to remind that a huge security issue arising from ConsoleCommand, which allowed a wad to delete arbitrary files from a remote server by modifying sv_*lists values, has been only recently brought to dev's attention and fixed.

I fully support the idea behind this ticket, I feel that ConsoleCommand is overrated and that the real legit use is minimal.

> - kick? I don't know any legit uses off-hand though
IMHO a wad shouldn't meddle with server actions like this. There is already KickFromGame, which I believe is enough to have.

(0006778)
Dusk (developer)
2013-07-23 17:55
edited on: 2013-07-23 18:38

Quote

> - kick? I don't know any legit uses off-hand though
IMHO a wad shouldn't meddle with server actions like this. There is already KickFromGame, which I believe is enough to have.

That's what I had in mind too. A mod shouldn't really have much a need anyway for the kick command; auto-kicks are just bad because they can greatly disrupt the game over a preset set of conditions which always has corner cases.

(0006780)
Catastrophe (reporter)
2013-07-23 20:33
edited on: 2013-07-23 20:35

"set", "archivecvar", and "puke" needs to be whitelisted if you guys are doing this.

(0006781)
Dusk (developer)
2013-07-23 20:41
edited on: 2013-07-23 20:41

That's kind of exactly what I outlined here.

(0006787)
Torr Samaho (administrator)
2013-07-24 19:33

I think this is one of the things we should probably also discuss on the forum since a lot more people are reading those than the tracker.

BTW: As I already stated on multiple occasions, I fully agree that ConsoleCommand is an abomination that needs to go.

(0006921)
Esum (reporter)
2013-08-07 09:25

I suggest to whitelist eval and test as well unless they trigger commands that are not whitelisted.

(0006944)
Dusk (developer)
2013-08-08 17:43

bump - just today we caught another case of abuse on the BE cluster. IMO we should hurry up with this and push out a security release.

I don't know what'd eval and test be useful for here - can't GetCVar work just as well?

(0006998)
Catastrophe (reporter)
2013-08-12 07:22

I also use +Attack to "wake up" monsters in a map; is there an alternative to this?

(0006999)
Dusk (developer)
2013-08-12 14:10

Quote
I also use +Attack to "wake up" monsters in a map; is there an alternative to this?

'http://zdoom.org/wiki/NoiseAlert [^]'

(0007635)
Hypnotoad (reporter)
2013-11-29 17:32

>I feel that ConsoleCommand [...] real legit use is minimal.

I strongly disagree. Due to some major obstacles with acs many, many mods are completely reliant on consolecommand at present. You'll be hard pressed to find any complex acs mod on zandronum that doesn't use consolecommand to some degree.

(0007642)
Gez (reporter)
2013-11-30 09:50
edited on: 2013-11-30 09:51

""set", "archivecvar", and "puke" needs to be whitelisted if you guys are doing this."

Why should "puke" be whitelisted? You can already use the ACS_*Execute* commands from within ACS. This means that puke is redundant, superfluous, unneeded, and extraneous.

(0007643)
Hypnotoad (reporter)
2013-11-30 12:00

Gez, puke is used because one of the many limitations of the engine is the inability to send information about variables etc changed clientside back to the server. Therefore one must puke a script from the client with arguments so that it can be executed on the server.

(0009392)
Watermelon (developer)
2014-06-15 14:46

While I support the idea a lot, we'd probably have to see what the major mods use this for and try to get a list from there so we'd have a good head start.

(0012772)
arkore (reporter)
2015-06-21 18:18
edited on: 2015-06-21 18:18

I don't know if BFH is major, but it uses consolecommand() for loading a new map (not changing map), since there's no other way to do it. The exit portal at the end will execute consolecommand("MAP START") to restart the mod.

Only registered users can voice their support. Click here to register, or here to log in.
Supporters:	ZzZombo Konar6 Dusk Esum Hypnotoad MazterQyoun-ae Monsterovich
Opponents:	No one explicitly opposes this issue yet.

Notes
(0006762) Dusk (developer) 2013-07-23 00:00	I wonder, should this be a public ticket? Could get some help building the possible white-list that way.

(0006764) Blzut3 (administrator) 2013-07-23 06:53	I see no reason this has to be private. In fact, I think it would be better that it isn't as it serves as a good example for all the people who think that consolecommand is harmless. :P As you can probably guess, I'd be all for a white list or even making it only available as a compat flag.

(0006768) Dusk (developer) 2013-07-23 12:07	Made public. I don't think that compatflagging is sufficient since a mod could just say it needs compat_acsconsolecommand true or something like that and people who have no idea what this even is would just activate it, and we have the same problem all over again. IMO it should be completely torn out. So yeah. I think a list of things we should allow for now... - set... maybe only if the cvar being set is mod-generated? Also AOW and WDI auto-adjust DMFlags and nobody complains. - archivecvar, same rationale as above - puke? Might be needed to get some info from clients to servers... - kick? I don't know any legit uses off-hand though

(0006772) ZzZombo (reporter) 2013-07-23 14:47	Kick a troll armed a C4/Ion/Nuke in his base.

(0006777) Konar6 (reporter) 2013-07-23 16:59	The attached snippet is part of a wad which one user was trying to host on servers. The full wad is here ->'http://sickedwick.net/k6/9ab7f5118db665ff6e2cc7dea7d4e2e9/surprise_me_v1.pk3_ [^]' (WARNING: don't run it, it's a malware that will screw your config if you do; I've renamed it to prevent accidental execution). This shows how dangerous ConsoleCommand still is, despite of removing specific reported commands one by one. It's simply not enough even in the short run to foresee and catch all possible misuse. I would also like to remind that a huge security issue arising from ConsoleCommand, which allowed a wad to delete arbitrary files from a remote server by modifying sv_*lists values, has been only recently brought to dev's attention and fixed. I fully support the idea behind this ticket, I feel that ConsoleCommand is overrated and that the real legit use is minimal. > - kick? I don't know any legit uses off-hand though IMHO a wad shouldn't meddle with server actions like this. There is already KickFromGame, which I believe is enough to have.

(0006778) Dusk (developer) 2013-07-23 17:55 edited on: 2013-07-23 18:38	Quote > - kick? I don't know any legit uses off-hand though IMHO a wad shouldn't meddle with server actions like this. There is already KickFromGame, which I believe is enough to have. That's what I had in mind too. A mod shouldn't really have much a need anyway for the kick command; auto-kicks are just bad because they can greatly disrupt the game over a preset set of conditions which always has corner cases.

(0006780) Catastrophe (reporter) 2013-07-23 20:33 edited on: 2013-07-23 20:35	"set", "archivecvar", and "puke" needs to be whitelisted if you guys are doing this.

(0006781) Dusk (developer) 2013-07-23 20:41 edited on: 2013-07-23 20:41	That's kind of exactly what I outlined here.

(0006787) Torr Samaho (administrator) 2013-07-24 19:33	I think this is one of the things we should probably also discuss on the forum since a lot more people are reading those than the tracker. BTW: As I already stated on multiple occasions, I fully agree that ConsoleCommand is an abomination that needs to go.

(0006921) Esum (reporter) 2013-08-07 09:25	I suggest to whitelist eval and test as well unless they trigger commands that are not whitelisted.

(0006944) Dusk (developer) 2013-08-08 17:43	bump - just today we caught another case of abuse on the BE cluster. IMO we should hurry up with this and push out a security release. I don't know what'd eval and test be useful for here - can't GetCVar work just as well?

(0006998) Catastrophe (reporter) 2013-08-12 07:22	I also use +Attack to "wake up" monsters in a map; is there an alternative to this?

(0006999) Dusk (developer) 2013-08-12 14:10	Quote I also use +Attack to "wake up" monsters in a map; is there an alternative to this? 'http://zdoom.org/wiki/NoiseAlert [^]'

(0007635) Hypnotoad (reporter) 2013-11-29 17:32	>I feel that ConsoleCommand [...] real legit use is minimal. I strongly disagree. Due to some major obstacles with acs many, many mods are completely reliant on consolecommand at present. You'll be hard pressed to find any complex acs mod on zandronum that doesn't use consolecommand to some degree.

(0007642) Gez (reporter) 2013-11-30 09:50 edited on: 2013-11-30 09:51	""set", "archivecvar", and "puke" needs to be whitelisted if you guys are doing this." Why should "puke" be whitelisted? You can already use the ACS_Execute commands from within ACS. This means that puke is redundant, superfluous, unneeded, and extraneous.

(0007643) Hypnotoad (reporter) 2013-11-30 12:00	Gez, puke is used because one of the many limitations of the engine is the inability to send information about variables etc changed clientside back to the server. Therefore one must puke a script from the client with arguments so that it can be executed on the server.

(0009392) Watermelon (developer) 2014-06-15 14:46	While I support the idea a lot, we'd probably have to see what the major mods use this for and try to get a list from there so we'd have a good head start.

(0012772) arkore (reporter) 2015-06-21 18:18 edited on: 2015-06-21 18:18	I don't know if BFH is major, but it uses consolecommand() for loading a new map (not changing map), since there's no other way to do it. The exit portal at the end will execute consolecommand("MAP START") to restart the mod.

Issue History
Date Modified	Username	Field	Change
2013-07-22 23:50	Dusk	New Issue
2013-07-22 23:51	Dusk	Description Updated	View Revisions
2013-07-22 23:53	Dusk	Relationship added	related to 0001070
2013-07-23 00:00	Dusk	Note Added: 0006762
2013-07-23 06:53	Blzut3	Note Added: 0006764
2013-07-23 12:02	Dusk	View Status	private => public
2013-07-23 12:07	Dusk	Note Added: 0006768
2013-07-23 12:09	Dusk	Description Updated	View Revisions
2013-07-23 14:47	ZzZombo	Note Added: 0006772
2013-07-23 16:59	Konar6	Note Added: 0006777
2013-07-23 17:55	Dusk	Note Added: 0006778
2013-07-23 18:38	Dusk	Note Edited: 0006778	View Revisions
2013-07-23 18:38	Dusk	Note Edited: 0006778	View Revisions
2013-07-23 20:33	Catastrophe	Note Added: 0006780
2013-07-23 20:35	Catastrophe	Note Edited: 0006780	View Revisions
2013-07-23 20:35	Catastrophe	Note Edited: 0006780	View Revisions
2013-07-23 20:41	Dusk	Note Added: 0006781
2013-07-23 20:41	Dusk	Note Edited: 0006781	View Revisions
2013-07-23 20:41	Dusk	Note Edited: 0006781	View Revisions
2013-07-24 19:33	Torr Samaho	Note Added: 0006787
2013-08-07 09:25	Esum	Note Added: 0006921
2013-08-08 17:43	Dusk	Note Added: 0006944
2013-08-12 07:22	Catastrophe	Note Added: 0006998
2013-08-12 14:10	Dusk	Note Added: 0006999
2013-11-29 17:32	Hypnotoad	Note Added: 0007635
2013-11-30 09:50	Gez	Note Added: 0007642
2013-11-30 09:51	Gez	Note Edited: 0007642	View Revisions
2013-11-30 12:00	Hypnotoad	Note Added: 0007643
2014-06-15 14:46	Watermelon	Note Added: 0009392
2015-06-21 18:18	arkore	Note Added: 0012772
2015-06-21 18:18	arkore	Note Edited: 0012772	View Revisions
2016-01-26 12:34	Dusk	Severity	major => exploit