From 6d37a670273d716e5c4db35083d09e4bf647c513 Mon Sep 17 00:00:00 2001 From: "alexey.lysiuk" Date: Sun, 7 Jan 2018 15:02:22 +0200 Subject: [PATCH 1/6] Added unsafe execution context for console commands Some console commands are insecure because they access user's file system Such commands cannot be executed from MENUDEF and KEYCONF aliases --- src/c_dispatch.cpp | 68 ++++++++++++++++++---------------------------- src/c_dispatch.h | 27 ++++++++++++++++++ 2 files changed, 54 insertions(+), 41 deletions(-) diff --git a/src/c_dispatch.cpp b/src/c_dispatch.cpp index 75779cc93..b8e221e38 100644 --- a/src/c_dispatch.cpp +++ b/src/c_dispatch.cpp @@ -126,7 +126,8 @@ FButtonStatus Button_Mlook, Button_Klook, Button_Use, Button_AltAttack, Button_AM_PanLeft, Button_AM_PanRight, Button_AM_PanDown, Button_AM_PanUp, Button_AM_ZoomIn, Button_AM_ZoomOut; -bool ParsingKeyConf, ParsingMenuDef = false; +bool ParsingKeyConf; +static bool UnsafeExecutionContext; // To add new actions, go to the console and type "key ". // This will give you the key value to use in the first column. Then @@ -187,24 +188,6 @@ static const char *KeyConfCommands[] = "clearplayerclasses" }; -static const char *MenuDefCommands[] = -{ - "snd_reset", - "reset2defaults", - "reset2saved", - "menuconsole", - "clearnodecache", - "am_restorecolors", - "undocolorpic", - "special", - "puke", - "fpuke", - "pukename", - "event", - "netevent", - "openmenu" -}; - // CODE -------------------------------------------------------------------- IMPLEMENT_CLASS(DWaitingCommand, false, false) @@ -602,25 +585,6 @@ void C_DoCommand (const char *cmd, int keynum) } } - if (ParsingMenuDef) - { - int i; - - for (i = countof(MenuDefCommands)-1; i >= 0; --i) - { - if (strnicmp (beg, MenuDefCommands[i], len) == 0 && - MenuDefCommands[i][len] == 0) - { - break; - } - } - if (i < 0) - { - Printf ("Invalid command for MENUDEF/ZScript: %s\n", beg); - return; - } - } - // Check if this is an action if (*beg == '+' || *beg == '-') { @@ -707,9 +671,9 @@ DEFINE_ACTION_FUNCTION(DOptionMenuItemCommand, DoCommand) if (CurrentMenu == nullptr) return 0; PARAM_PROLOGUE; PARAM_STRING(cmd); - ParsingMenuDef = true; + UnsafeExecutionContext = true; C_DoCommand(cmd); - ParsingMenuDef = false; + UnsafeExecutionContext = false; return 0; } @@ -1061,6 +1025,17 @@ void FConsoleCommand::Run (FCommandLine &argv, APlayerPawn *who, int key) m_RunFunc (argv, who, key); } +void FUnsafeConsoleCommand::Run (FCommandLine &args, APlayerPawn *instigator, int key) +{ + if (UnsafeExecutionContext) + { + Printf(TEXTCOLOR_RED "Cannot execute unsafe command " TEXTCOLOR_GOLD "%s\n", m_Name); + return; + } + + FConsoleCommand::Run (args, instigator, key); +} + FConsoleAlias::FConsoleAlias (const char *name, const char *command, bool noSave) : FConsoleCommand (name, NULL), bRunning(false), bKill(false) @@ -1381,9 +1356,13 @@ CCMD (alias) alias = NULL; } } + else if (ParsingKeyConf) + { + new FUnsafeConsoleAlias (argv[1], argv[2]); + } else { - new FConsoleAlias (argv[1], argv[2], ParsingKeyConf); + new FConsoleAlias (argv[1], argv[2], false); } } } @@ -1521,6 +1500,13 @@ void FConsoleAlias::SafeDelete () } } +void FUnsafeConsoleAlias::Run (FCommandLine &args, APlayerPawn *instigator, int key) +{ + UnsafeExecutionContext = true; + FConsoleAlias::Run(args, instigator, key); + UnsafeExecutionContext = false; +} + void FExecList::AddCommand(const char *cmd, const char *file) { // Pullins are special and need to be separated from general commands. diff --git a/src/c_dispatch.h b/src/c_dispatch.h index 24a7c42ec..dac14b818 100644 --- a/src/c_dispatch.h +++ b/src/c_dispatch.h @@ -127,6 +127,22 @@ protected: FConsoleCommand Cmd_##n##_Ref (#n, Cmd_##n); \ void Cmd_##n (FCommandLine &argv, APlayerPawn *who, int key) +class FUnsafeConsoleCommand : public FConsoleCommand +{ +public: + FUnsafeConsoleCommand (const char *name, CCmdRun RunFunc) + : FConsoleCommand (name, RunFunc) + { + } + + virtual void Run (FCommandLine &args, APlayerPawn *instigator, int key) override; +}; + +#define UNSAFE_CCMD(n) \ + static void Cmd_##n (FCommandLine &, APlayerPawn *, int key); \ + static FUnsafeConsoleCommand Cmd_##n##_Ref (#n, Cmd_##n); \ + void Cmd_##n (FCommandLine &argv, APlayerPawn *who, int key) + const int KEY_DBLCLICKED = 0x8000; class FConsoleAlias : public FConsoleCommand @@ -147,6 +163,17 @@ protected: bool bKill; }; +class FUnsafeConsoleAlias : public FConsoleAlias +{ +public: + FUnsafeConsoleAlias (const char *name, const char *command) + : FConsoleAlias (name, command, true) + { + } + + virtual void Run (FCommandLine &args, APlayerPawn *instigator, int key) override; +}; + // Actions struct FButtonStatus { From f25a5ea2bc4b42312a477ce261e95a93586379c5 Mon Sep 17 00:00:00 2001 From: "alexey.lysiuk" Date: Sun, 7 Jan 2018 15:03:49 +0200 Subject: [PATCH 2/6] Marked a few commands as unsafe This process of finding unsafe commands is not complete! --- src/c_cmds.cpp | 10 +++++----- src/c_console.cpp | 2 +- src/d_main.cpp | 4 ++-- src/g_level.cpp | 2 +- src/m_misc.cpp | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/c_cmds.cpp b/src/c_cmds.cpp index 748680e91..e3e87595d 100644 --- a/src/c_cmds.cpp +++ b/src/c_cmds.cpp @@ -467,7 +467,7 @@ CCMD (print) } } -CCMD (exec) +UNSAFE_CCMD (exec) { if (argv.argc() < 2) return; @@ -495,7 +495,7 @@ void execLogfile(const char *fn, bool append) } } -CCMD (logfile) +UNSAFE_CCMD (logfile) { if (Logfile) @@ -651,7 +651,7 @@ CCMD (error) } } -CCMD (error_fatal) +UNSAFE_CCMD (error_fatal) { if (argv.argc() > 1) { @@ -794,7 +794,7 @@ CCMD (warp) // //========================================================================== -CCMD (load) +UNSAFE_CCMD (load) { if (argv.argc() != 2) { @@ -819,7 +819,7 @@ CCMD (load) // //========================================================================== -CCMD (save) +UNSAFE_CCMD (save) { if (argv.argc() < 2 || argv.argc() > 3) { diff --git a/src/c_console.cpp b/src/c_console.cpp index c3ab76903..9cbb5623f 100644 --- a/src/c_console.cpp +++ b/src/c_console.cpp @@ -627,7 +627,7 @@ void C_InitConsole (int width, int height, bool ingame) // //========================================================================== -CCMD (atexit) +UNSAFE_CCMD (atexit) { if (argv.argc() == 1) { diff --git a/src/d_main.cpp b/src/d_main.cpp index 656a2ca3b..92e10903f 100644 --- a/src/d_main.cpp +++ b/src/d_main.cpp @@ -2821,7 +2821,7 @@ void D_DoomMain (void) // //========================================================================== -CCMD(restart) +UNSAFE_CCMD(restart) { // remove command line args that would get in the way during restart Args->RemoveArgs("-iwad"); @@ -2917,4 +2917,4 @@ CUSTOM_CVAR(Bool, I_FriendlyWindowTitle, true, CVAR_GLOBALCONFIG|CVAR_ARCHIVE|CV I_SetWindowTitle(DoomStartupInfo.Name.GetChars()); else I_SetWindowTitle(NULL); -} \ No newline at end of file +} diff --git a/src/g_level.cpp b/src/g_level.cpp index ce8ff0850..b427599f0 100644 --- a/src/g_level.cpp +++ b/src/g_level.cpp @@ -277,7 +277,7 @@ CCMD(recordmap) // //========================================================================== -CCMD (open) +UNSAFE_CCMD (open) { if (netgame) { diff --git a/src/m_misc.cpp b/src/m_misc.cpp index 8fcaf9e0c..eb51ec7b9 100644 --- a/src/m_misc.cpp +++ b/src/m_misc.cpp @@ -315,7 +315,7 @@ void M_SaveDefaultsFinal () GameConfig = NULL; } -CCMD (writeini) +UNSAFE_CCMD (writeini) { const char *filename = (argv.argc() == 1) ? NULL : argv[1]; if (!M_SaveDefaults (filename)) From 059e40e2d52cb201609f78c1520f14c53093f2bd Mon Sep 17 00:00:00 2001 From: "alexey.lysiuk" Date: Sun, 7 Jan 2018 15:04:50 +0200 Subject: [PATCH 3/6] Prohibited setting of non-mod CVARs from unsafe alias --- src/c_dispatch.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/c_dispatch.cpp b/src/c_dispatch.cpp index b8e221e38..73d0d5d3e 100644 --- a/src/c_dispatch.cpp +++ b/src/c_dispatch.cpp @@ -651,6 +651,12 @@ void C_DoCommand (const char *cmd, int keynum) if (args.argc() >= 2) { // Set the variable + if (UnsafeExecutionContext && !(var->GetFlags() & CVAR_MOD)) + { + Printf(TEXTCOLOR_RED "Cannot set console variable" TEXTCOLOR_GOLD " %s " TEXTCOLOR_RED "from unsafe command\n", var->GetName()); + return; + } + var->CmdSet (args[1]); } else From b008426ed7c6f24a8033111b24147b6822a7bd08 Mon Sep 17 00:00:00 2001 From: "alexey.lysiuk" Date: Mon, 8 Jan 2018 10:41:31 +0200 Subject: [PATCH 4/6] Added propagation of unsafe execution context to waiting command Thanks Edward-san for pointing this out --- src/c_dispatch.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/c_dispatch.cpp b/src/c_dispatch.cpp index 73d0d5d3e..fa786b271 100644 --- a/src/c_dispatch.cpp +++ b/src/c_dispatch.cpp @@ -65,7 +65,7 @@ class DWaitingCommand : public DThinker { DECLARE_CLASS (DWaitingCommand, DThinker) public: - DWaitingCommand (const char *cmd, int tics); + DWaitingCommand (const char *cmd, int tics, bool unsafe); ~DWaitingCommand (); void Serialize(FSerializer &arc); void Tick (); @@ -75,6 +75,7 @@ private: char *Command; int TicsLeft; + bool IsUnsafe; }; class DStoredCommand : public DThinker @@ -203,12 +204,14 @@ DWaitingCommand::DWaitingCommand () { Command = NULL; TicsLeft = 1; + IsUnsafe = false; } -DWaitingCommand::DWaitingCommand (const char *cmd, int tics) +DWaitingCommand::DWaitingCommand (const char *cmd, int tics, bool unsafe) { Command = copystring (cmd); TicsLeft = tics+1; + IsUnsafe = unsafe; } DWaitingCommand::~DWaitingCommand () @@ -223,7 +226,10 @@ void DWaitingCommand::Tick () { if (--TicsLeft == 0) { + const bool wasUnsafe = UnsafeExecutionContext; + UnsafeExecutionContext = IsUnsafe; AddCommandString (Command); + UnsafeExecutionContext = wasUnsafe; Destroy (); } } @@ -739,7 +745,7 @@ void AddCommandString (char *cmd, int keynum) // Note that deferred commands lose track of which key // (if any) they were pressed from. *brkpt = ';'; - Create (brkpt, tics); + Create (brkpt, tics, UnsafeExecutionContext); } return; } From 8667987cd65e709b8eb5c02e0b603d7aa515c3a5 Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Mon, 8 Jan 2018 18:13:02 +0100 Subject: [PATCH 5/6] - DWaitingCommand needs to serialize its unsafe state. --- src/c_dispatch.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/c_dispatch.cpp b/src/c_dispatch.cpp index fa786b271..ec4571b53 100644 --- a/src/c_dispatch.cpp +++ b/src/c_dispatch.cpp @@ -197,7 +197,8 @@ void DWaitingCommand::Serialize(FSerializer &arc) { Super::Serialize (arc); arc("command", Command) - ("ticsleft", TicsLeft); + ("ticsleft", TicsLeft) + ("unsafe", IsUnsafe); } DWaitingCommand::DWaitingCommand () From 2720e36a2c2298c84314092c7152c180e86cb0dc Mon Sep 17 00:00:00 2001 From: Christoph Oelckers Date: Sat, 20 Jan 2018 09:11:28 +0100 Subject: [PATCH 6/6] - marked a few more CCMDs unsafe. --- src/c_bind.cpp | 2 +- src/c_cmds.cpp | 2 +- src/g_game.cpp | 2 +- src/g_level.cpp | 2 +- src/sound/i_music.cpp | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/c_bind.cpp b/src/c_bind.cpp index 5098b9a95..62d497f80 100644 --- a/src/c_bind.cpp +++ b/src/c_bind.cpp @@ -558,7 +558,7 @@ void C_UnbindAll () AutomapBindings.UnbindAll(); } -CCMD (unbindall) +UNSAFE_CCMD (unbindall) { C_UnbindAll (); } diff --git a/src/c_cmds.cpp b/src/c_cmds.cpp index e3e87595d..efc11a3d2 100644 --- a/src/c_cmds.cpp +++ b/src/c_cmds.cpp @@ -674,7 +674,7 @@ UNSAFE_CCMD (error_fatal) //========================================================================== #if !defined(_WIN32) || !defined(_DEBUG) -CCMD (crashout) +UNSAFE_CCMD (crashout) { *(volatile int *)0 = 0; } diff --git a/src/g_game.cpp b/src/g_game.cpp index 82bff3f73..0e44e8301 100644 --- a/src/g_game.cpp +++ b/src/g_game.cpp @@ -2618,7 +2618,7 @@ CCMD (playdemo) } } -CCMD (timedemo) +UNSAFE_CCMD (timedemo) { if (argv.argc() > 1) { diff --git a/src/g_level.cpp b/src/g_level.cpp index b427599f0..0b211f3e6 100644 --- a/src/g_level.cpp +++ b/src/g_level.cpp @@ -224,7 +224,7 @@ CCMD (map) // //========================================================================== -CCMD(recordmap) +UNSAFE_CCMD(recordmap) { if (netgame) { diff --git a/src/sound/i_music.cpp b/src/sound/i_music.cpp index 28f839c6c..342fc5bf6 100644 --- a/src/sound/i_music.cpp +++ b/src/sound/i_music.cpp @@ -708,7 +708,7 @@ ADD_STAT(music) // //========================================================================== -CCMD (writeopl) +UNSAFE_CCMD (writeopl) { if (argv.argc() == 2) { @@ -746,7 +746,7 @@ CCMD (writeopl) // //========================================================================== -CCMD (writewave) +UNSAFE_CCMD (writewave) { if (argv.argc() >= 2 && argv.argc() <= 3) { @@ -784,7 +784,7 @@ CCMD (writewave) // //========================================================================== -CCMD (writemidi) +UNSAFE_CCMD (writemidi) { if (argv.argc() != 2) {