changed Cmd_AddCommand to only work for console commands, not client commands execute...

this refactoring fixes a security vulnerability in the clcommand builtin provided by KRIMZON_SV_PARSECLIENTCOMMAND, which was able to execute many commands on the server console, and that put the burden on the QC code to validate command safety, which was not intended
in short: this fixes a remote console command execution vulnerability that affected a few games/mods

git-svn-id: svn://svn.icculus.org/twilight/trunk/darkplaces@6685 d7cf8633-e32d-0410-b094-e92efae38249

diff --git a/cl_demo.c b/cl_demo.c
index 627ccd12d68eaf4fbcd59355865512310a121f56..3e7f1c50aadb579c3e624fd8eb1b94b3676a112a 100644 (file)
--- a/cl_demo.c
+++ b/cl_demo.c
@@ -216,9 +216,6 @@ stop recording a demo
 */
 void CL_Stop_f (void)
 {
-       if (cmd_source != src_command)
-               return;
-
        if (!cls.demorecording)
        {
                Con_Print("Not recording a demo.\n");
@@ -249,9 +246,6 @@ void CL_Record_f (void)
        int c, track;
        char name[MAX_OSPATH];
 
-       if (cmd_source != src_command)
-               return;
-
        c = Cmd_Argc();
        if (c != 2 && c != 3 && c != 4)
        {
@@ -317,9 +311,6 @@ void CL_PlayDemo_f (void)
        int c;
        qboolean neg = false;
 
-       if (cmd_source != src_command)
-               return;
-
        if (Cmd_Argc() != 2)
        {
                Con_Print("play <demoname> : plays a demo\n");
@@ -398,9 +389,6 @@ timedemo [demoname]
 */
 void CL_TimeDemo_f (void)
 {
-       if (cmd_source != src_command)
-               return;
-
        if (Cmd_Argc() != 2)
        {
                Con_Print("timedemo <demoname> : gets demo speeds\n");

diff --git a/cmd.c b/cmd.c
index 9797e01f43fb80478f820b69e2e1bd405931b9e1..a59a46f6189e86fe3ed91c5ee282f31100ec107a 100644 (file)
--- a/cmd.c
+++ b/cmd.c
@@ -482,7 +482,8 @@ typedef struct cmd_function_s
        struct cmd_function_s *next;
        const char *name;
        const char *description;
-       xcommand_t function;
+       xcommand_t consolefunction;
+       xcommand_t clientfunction;
        qboolean csqcfunc;
 } cmd_function_t;
 
@@ -795,7 +796,7 @@ static void Cmd_TokenizeString (const char *text)
 Cmd_AddCommand
 ============
 */
-void Cmd_AddCommand (const char *cmd_name, xcommand_t function, const char *description)
+void Cmd_AddCommand_WithClientCommand (const char *cmd_name, xcommand_t consolefunction, xcommand_t clientfunction, const char *description)
 {
        cmd_function_t *cmd;
        cmd_function_t *prev, *current;
@@ -812,7 +813,7 @@ void Cmd_AddCommand (const char *cmd_name, xcommand_t function, const char *desc
        {
                if (!strcmp (cmd_name, cmd->name))
                {
-                       if (function)
+                       if (consolefunction || clientfunction)
                        {
                                Con_Printf("Cmd_AddCommand: %s already defined\n", cmd_name);
                                return;
@@ -827,9 +828,10 @@ void Cmd_AddCommand (const char *cmd_name, xcommand_t function, const char *desc
 
        cmd = (cmd_function_t *)Mem_Alloc(cmd_mempool, sizeof(cmd_function_t));
        cmd->name = cmd_name;
-       cmd->function = function;
+       cmd->consolefunction = consolefunction;
+       cmd->clientfunction = clientfunction;
        cmd->description = description;
-       if(!function)                   //[515]: csqc
+       if(!consolefunction && !clientfunction)                 //[515]: csqc
                cmd->csqcfunc = true;
        cmd->next = cmd_functions;
 
@@ -844,6 +846,11 @@ void Cmd_AddCommand (const char *cmd_name, xcommand_t function, const char *desc
        cmd->next = current;
 }
 
+void Cmd_AddCommand (const char *cmd_name, xcommand_t function, const char *description)
+{
+       Cmd_AddCommand_WithClientCommand (cmd_name, function, NULL, description);
+}
+
 /*
 ============
 Cmd_Exists
@@ -1088,19 +1095,47 @@ void Cmd_ExecuteString (const char *text, cmd_source_t src)
        {
                if (!strcasecmp (cmd_argv[0],cmd->name))
                {
-                       if(cmd->function && !cmd->csqcfunc)
-                               cmd->function ();
-                       else
-                               if(CL_VM_ConsoleCommand (text)) //[515]: csqc
-                                       return;
+                       if (cmd->csqcfunc && CL_VM_ConsoleCommand (text))       //[515]: csqc
+                               return;
+                       switch (src)
+                       {
+                       case src_command:
+                               if (cmd->consolefunction)
+                                       cmd->consolefunction ();
+                               else if (cmd->clientfunction)
+                               {
+                                       if (cls.state == ca_connected)
+                                       {
+                                               // forward remote commands to the server for execution
+                                               Cmd_ForwardToServer();
+                                       }
+                                       else
+                                               Con_Printf("Can not send command \"%s\", not connected.\n", Cmd_Argv(0));
+                               }
                                else
-                                       if(cmd->function)
-                                               cmd->function ();
-                       cmd_tokenizebufferpos = oldpos;
-                       return;
+                                       Con_Printf("Command \"%s\" can not be executed\n", Cmd_Argv(0));
+                               cmd_tokenizebufferpos = oldpos;
+                               return;
+                       case src_client:
+                               if (cmd->clientfunction)
+                               {
+                                       cmd->clientfunction ();
+                                       cmd_tokenizebufferpos = oldpos;
+                                       return;
+                               }
+                               break;
+                       }
+                       break;
                }
        }
 
+       // if it's a client command and no command was found, say so.
+       if (cmd_source == src_client)
+       {
+               Con_Printf("player \"%s\" tried to %s\n", host_client->name, text);
+               return;
+       }
+
 // check alias
        for (a=cmd_alias ; a ; a=a->next)
        {

diff --git a/cmd.h b/cmd.h
index f7d9d17c1a6f97ced9779902e6d3747733c5fbe4..130e3cde3107395ec99081529751ae806e4a8c0b 100644 (file)
--- a/cmd.h
+++ b/cmd.h
@@ -86,6 +86,7 @@ extern cmd_source_t cmd_source;
 void Cmd_Init (void);
 void Cmd_Shutdown (void);
 
+void Cmd_AddCommand_WithClientCommand (const char *cmd_name, xcommand_t consolefunction, xcommand_t clientfunction, const char *description);
 void Cmd_AddCommand (const char *cmd_name, xcommand_t function, const char *description);
 // called by the init functions of other parts of the program to
 // register commands and functions to call for them.

diff --git a/host_cmd.c b/host_cmd.c
index d3665ed63d39435b8868e228eca9c279f57f6e5a..5f93996d0734f199c6f2607757607459b3b2af2a 100644 (file)
--- a/host_cmd.c
+++ b/host_cmd.c
@@ -106,12 +106,6 @@ Sets client to godmode
 */
 void Host_God_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
-
        if (!allowcheats)
        {
                SV_ClientPrint("No cheats allowed, use sv_cheats 1 and restart level to enable.\n");
@@ -127,12 +121,6 @@ void Host_God_f (void)
 
 void Host_Notarget_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
-
        if (!allowcheats)
        {
                SV_ClientPrint("No cheats allowed, use sv_cheats 1 and restart level to enable.\n");
@@ -150,12 +138,6 @@ qboolean noclip_anglehack;
 
 void Host_Noclip_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
-
        if (!allowcheats)
        {
                SV_ClientPrint("No cheats allowed, use sv_cheats 1 and restart level to enable.\n");
@@ -185,12 +167,6 @@ Sets client to flymode
 */
 void Host_Fly_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
-
        if (!allowcheats)
        {
                SV_ClientPrint("No cheats allowed, use sv_cheats 1 and restart level to enable.\n");
@@ -278,9 +254,6 @@ void Host_Map_f (void)
                return;
        }
 
-       if (cmd_source != src_command)
-               return;
-
        cls.demonum = -1;               // stop demo loop in case this fails
 
        CL_Disconnect ();
@@ -341,8 +314,6 @@ void Host_Changelevel_f (void)
                Host_Map_f();
                return;
        }
-       if (cmd_source != src_command)
-               return;
 
        // remove menu
        key_dest = key_game;
@@ -378,8 +349,6 @@ void Host_Restart_f (void)
                Con_Print("Only the server may restart\n");
                return;
        }
-       if (cmd_source != src_command)
-               return;
 
        // remove menu
        key_dest = key_game;
@@ -512,9 +481,6 @@ void Host_Savegame_f (void)
        int             i;
        char    comment[SAVEGAME_COMMENT_LENGTH+1];
 
-       if (cmd_source != src_command)
-               return;
-
        if (cls.state != ca_connected || !sv.active)
        {
                Con_Print("Not playing a local game.\n");
@@ -618,9 +584,6 @@ void Host_Loadgame_f (void)
        int version;
        float spawn_parms[NUM_SPAWN_PARMS];
 
-       if (cmd_source != src_command)
-               return;
-
        if (Cmd_Argc() != 2)
        {
                Con_Print("load <savename> : load a game\n");
@@ -1256,12 +1219,6 @@ Host_Kill_f
 */
 void Host_Kill_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
-
        if (host_client->edict->fields.server->health <= 0)
        {
                SV_ClientPrint("Can't suicide -- already dead!\n");
@@ -1281,12 +1238,6 @@ Host_Pause_f
 */
 void Host_Pause_f (void)
 {
-
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
        if (!pausable.integer)
                SV_ClientPrint("Pause not allowed.\n");
        else
@@ -1343,12 +1294,6 @@ Host_PreSpawn_f
 */
 void Host_PreSpawn_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Con_Print("prespawn is not valid from the console\n");
-               return;
-       }
-
        if (host_client->spawned)
        {
                Con_Print("prespawn not valid -- already spawned\n");
@@ -1379,12 +1324,6 @@ void Host_Spawn_f (void)
        mfunction_t *f;
        int stats[MAX_CL_STATS];
 
-       if (cmd_source == src_command)
-       {
-               Con_Print("spawn is not valid from the console\n");
-               return;
-       }
-
        if (host_client->spawned)
        {
                Con_Print("Spawn not valid -- already spawned\n");
@@ -1521,12 +1460,6 @@ Host_Begin_f
 */
 void Host_Begin_f (void)
 {
-       if (cmd_source == src_command)
-       {
-               Con_Print("begin is not valid from the console\n");
-               return;
-       }
-
        host_client->spawned = true;
 }
 
@@ -1548,7 +1481,7 @@ void Host_Kick_f (void)
        int i;
        qboolean byNumber = false;
 
-       if (cmd_source != src_command || !sv.active)
+       if (!sv.active)
                return;
 
        SV_VM_Begin();
@@ -1632,12 +1565,6 @@ void Host_Give_f (void)
        int v;
        prvm_eval_t *val;
 
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
-
        if (!allowcheats)
        {
                SV_ClientPrint("No cheats allowed, use sv_cheats 1 and restart level to enable.\n");
@@ -2345,11 +2272,6 @@ void Host_Pings_f (void)
        int             i, j, ping, packetloss;
        char temp[128];
 
-       if (cmd_source == src_command)
-       {
-               Cmd_ForwardToServer ();
-               return;
-       }
        if (!host_client->netconnection)
                return;
 
@@ -2409,23 +2331,23 @@ void Host_InitCommands (void)
 {
        dpsnprintf(cls.userinfo, sizeof(cls.userinfo), "\\name\\player\\team\\none\\topcolor\\0\\bottomcolor\\0\\rate\\10000\\msg\\1\\noaim\\1\\*ver\\%s", engineversion);
 
-       Cmd_AddCommand ("status", Host_Status_f, "print server status information");
+       Cmd_AddCommand_WithClientCommand ("status", Host_Status_f, Host_Status_f, "print server status information");
        Cmd_AddCommand ("quit", Host_Quit_f, "quit the game");
        if (gamemode == GAME_NEHAHRA)
        {
-               Cmd_AddCommand ("max", Host_God_f, "god mode (invulnerability)");
-               Cmd_AddCommand ("monster", Host_Notarget_f, "notarget mode (monsters do not see you)");
-               Cmd_AddCommand ("scrag", Host_Fly_f, "fly mode (flight)");
-               Cmd_AddCommand ("wraith", Host_Noclip_f, "noclip mode (flight without collisions, move through walls)");
-               Cmd_AddCommand ("gimme", Host_Give_f, "alter inventory");
+               Cmd_AddCommand_WithClientCommand ("max", NULL, Host_God_f, "god mode (invulnerability)");
+               Cmd_AddCommand_WithClientCommand ("monster", NULL, Host_Notarget_f, "notarget mode (monsters do not see you)");
+               Cmd_AddCommand_WithClientCommand ("scrag", NULL, Host_Fly_f, "fly mode (flight)");
+               Cmd_AddCommand_WithClientCommand ("wraith", NULL, Host_Noclip_f, "noclip mode (flight without collisions, move through walls)");
+               Cmd_AddCommand_WithClientCommand ("gimme", NULL, Host_Give_f, "alter inventory");
        }
        else
        {
-               Cmd_AddCommand ("god", Host_God_f, "god mode (invulnerability)");
-               Cmd_AddCommand ("notarget", Host_Notarget_f, "notarget mode (monsters do not see you)");
-               Cmd_AddCommand ("fly", Host_Fly_f, "fly mode (flight)");
-               Cmd_AddCommand ("noclip", Host_Noclip_f, "noclip mode (flight without collisions, move through walls)");
-               Cmd_AddCommand ("give", Host_Give_f, "alter inventory");
+               Cmd_AddCommand_WithClientCommand ("god", NULL, Host_God_f, "god mode (invulnerability)");
+               Cmd_AddCommand_WithClientCommand ("notarget", NULL, Host_Notarget_f, "notarget mode (monsters do not see you)");
+               Cmd_AddCommand_WithClientCommand ("fly", NULL, Host_Fly_f, "fly mode (flight)");
+               Cmd_AddCommand_WithClientCommand ("noclip", NULL, Host_Noclip_f, "noclip mode (flight without collisions, move through walls)");
+               Cmd_AddCommand_WithClientCommand ("give", NULL, Host_Give_f, "alter inventory");
        }
        Cmd_AddCommand ("map", Host_Map_f, "kick everyone off the server and start a new level");
        Cmd_AddCommand ("restart", Host_Restart_f, "restart current level");
@@ -2433,13 +2355,13 @@ void Host_InitCommands (void)
        Cmd_AddCommand ("connect", Host_Connect_f, "connect to a server by IP address or hostname");
        Cmd_AddCommand ("reconnect", Host_Reconnect_f, "reset signon level in preparation for a new level (do not use)");
        Cmd_AddCommand ("version", Host_Version_f, "print engine version");
-       Cmd_AddCommand ("say", Host_Say_f, "send a chat message to everyone on the server");
-       Cmd_AddCommand ("say_team", Host_Say_Team_f, "send a chat message to your team on the server");
-       Cmd_AddCommand ("tell", Host_Tell_f, "send a chat message to only one person on the server");
-       Cmd_AddCommand ("kill", Host_Kill_f, "die instantly");
-       Cmd_AddCommand ("pause", Host_Pause_f, "pause the game (if the server allows pausing)");
+       Cmd_AddCommand_WithClientCommand ("say", Host_Say_f, Host_Say_f, "send a chat message to everyone on the server");
+       Cmd_AddCommand_WithClientCommand ("say_team", Host_Say_Team_f, Host_Say_Team_f, "send a chat message to your team on the server");
+       Cmd_AddCommand_WithClientCommand ("tell", Host_Tell_f, Host_Tell_f, "send a chat message to only one person on the server");
+       Cmd_AddCommand_WithClientCommand ("kill", NULL, Host_Kill_f, "die instantly");
+       Cmd_AddCommand_WithClientCommand ("pause", NULL, Host_Pause_f, "pause the game (if the server allows pausing)");
        Cmd_AddCommand ("kick", Host_Kick_f, "kick a player off the server by number or name");
-       Cmd_AddCommand ("ping", Host_Ping_f, "print ping times of all players on the server");
+       Cmd_AddCommand_WithClientCommand ("ping", Host_Ping_f, Host_Ping_f, "print ping times of all players on the server");
        Cmd_AddCommand ("load", Host_Loadgame_f, "load a saved game file");
        Cmd_AddCommand ("save", Host_Savegame_f, "save the game to a file");
 
@@ -2453,26 +2375,26 @@ void Host_InitCommands (void)
        Cmd_AddCommand ("viewprev", Host_Viewprev_f, "change to previous animation frame of viewthing entity in current level");
 
        Cvar_RegisterVariable (&cl_name);
-       Cmd_AddCommand ("name", Host_Name_f, "change your player name");
+       Cmd_AddCommand_WithClientCommand ("name", Host_Name_f, Host_Name_f, "change your player name");
        Cvar_RegisterVariable (&cl_color);
-       Cmd_AddCommand ("color", Host_Color_f, "change your player shirt and pants colors");
+       Cmd_AddCommand_WithClientCommand ("color", Host_Color_f, Host_Color_f, "change your player shirt and pants colors");
        Cvar_RegisterVariable (&cl_rate);
-       Cmd_AddCommand ("rate", Host_Rate_f, "change your network connection speed");
+       Cmd_AddCommand_WithClientCommand ("rate", Host_Rate_f, Host_Rate_f, "change your network connection speed");
        if (gamemode == GAME_NEHAHRA)
        {
                Cvar_RegisterVariable (&cl_pmodel);
-               Cmd_AddCommand ("pmodel", Host_PModel_f, "change your player model choice (Nehahra specific)");
+               Cmd_AddCommand_WithClientCommand ("pmodel", Host_PModel_f, Host_PModel_f, "change your player model choice (Nehahra specific)");
        }
 
        // BLACK: This isnt game specific anymore (it was GAME_NEXUIZ at first)
        Cvar_RegisterVariable (&cl_playermodel);
-       Cmd_AddCommand ("playermodel", Host_Playermodel_f, "change your player model");
+       Cmd_AddCommand_WithClientCommand ("playermodel", Host_Playermodel_f, Host_Playermodel_f, "change your player model");
        Cvar_RegisterVariable (&cl_playerskin);
-       Cmd_AddCommand ("playerskin", Host_Playerskin_f, "change your player skin number");
+       Cmd_AddCommand_WithClientCommand ("playerskin", Host_Playerskin_f, Host_Playerskin_f, "change your player skin number");
 
-       Cmd_AddCommand ("prespawn", Host_PreSpawn_f, "signon 1 (client acknowledges that server information has been received)");
-       Cmd_AddCommand ("spawn", Host_Spawn_f, "signon 2 (client has sent player information, and is asking server to send scoreboard rankings)");
-       Cmd_AddCommand ("begin", Host_Begin_f, "signon 3 (client asks server to start sending entities, and will go to signon 4 (playing) when the first entity update is received)");
+       Cmd_AddCommand_WithClientCommand ("prespawn", NULL, Host_PreSpawn_f, "signon 1 (client acknowledges that server information has been received)");
+       Cmd_AddCommand_WithClientCommand ("spawn", NULL, Host_Spawn_f, "signon 2 (client has sent player information, and is asking server to send scoreboard rankings)");
+       Cmd_AddCommand_WithClientCommand ("begin", NULL, Host_Begin_f, "signon 3 (client asks server to start sending entities, and will go to signon 4 (playing) when the first entity update is received)");
        Cmd_AddCommand ("maxplayers", MaxPlayers_f, "sets limit on how many players (or bots) may be connected to the server at once");
 
        Cmd_AddCommand ("sendcvar", Host_SendCvar_f, "sends the value of a cvar to the server as a sentcvar command, for use by QuakeC");       // By [515]
@@ -2489,7 +2411,7 @@ void Host_InitCommands (void)
        Cmd_AddCommand ("topcolor", Host_TopColor_f, "QW command to set top color without changing bottom color");
        Cmd_AddCommand ("bottomcolor", Host_BottomColor_f, "QW command to set bottom color without changing top color");
 
-       Cmd_AddCommand ("pings", Host_Pings_f, "command sent by clients to request updated ping and packetloss of players on scoreboard (originally from QW, but also used on NQ servers)");
+       Cmd_AddCommand_WithClientCommand ("pings", NULL, Host_Pings_f, "command sent by clients to request updated ping and packetloss of players on scoreboard (originally from QW, but also used on NQ servers)");
        Cmd_AddCommand ("pingplreport", Host_PingPLReport_f, "command sent by server containing client ping and packet loss values for scoreboard, triggered by pings command from client (not used by QW servers)");
 
        Cvar_RegisterVariable (&team);

diff --git a/sv_user.c b/sv_user.c
index e8998013f46fc9468e3cb08d4cbd009f86566ed5..3d2e78c5cce0c5e993ec9174077be69d349d888d 100644 (file)
--- a/sv_user.c
+++ b/sv_user.c
@@ -696,27 +696,8 @@ void SV_ReadClientMessage(void)
                                prog->globals.server->self = PRVM_EDICT_TO_PROG(host_client->edict);
                                PRVM_ExecuteProgram ((func_t)(SV_ParseClientCommandQC - prog->functions), "QC function SV_ParseClientCommand is missing");
                        }
-                       else if (strncasecmp(s, "status", 6) == 0
-                        || strncasecmp(s, "name", 4) == 0
-                        || strncasecmp(s, "say", 3) == 0
-                        || strncasecmp(s, "say_team", 8) == 0
-                        || strncasecmp(s, "tell", 4) == 0
-                        || strncasecmp(s, "color", 5) == 0
-                        || strncasecmp(s, "kill", 4) == 0
-                        || strncasecmp(s, "pause", 5) == 0
-                        || strncasecmp(s, "kick", 4) == 0
-                        || strncasecmp(s, "ping", 4) == 0
-                        || strncasecmp(s, "pings", 5) == 0
-                        || strncasecmp(s, "ban", 3) == 0
-                        || strncasecmp(s, "pmodel", 6) == 0
-                        || strncasecmp(s, "rate", 4) == 0
-                        || strncasecmp(s, "playermodel", 11) == 0
-                        || strncasecmp(s, "playerskin", 10) == 0
-                        || (gamemode == GAME_NEHAHRA && (strncasecmp(s, "max", 3) == 0 || strncasecmp(s, "monster", 7) == 0 || strncasecmp(s, "scrag", 5) == 0 || strncasecmp(s, "gimme", 5) == 0 || strncasecmp(s, "wraith", 6) == 0))
-                        || (gamemode != GAME_NEHAHRA && (strncasecmp(s, "god", 3) == 0 || strncasecmp(s, "notarget", 8) == 0 || strncasecmp(s, "fly", 3) == 0 || strncasecmp(s, "give", 4) == 0 || strncasecmp(s, "noclip", 6) == 0)))
-                               Cmd_ExecuteString (s, src_client);
                        else
-                               Con_Printf("%s tried to %s\n", host_client->name, s);
+                               Cmd_ExecuteString (s, src_client);
                        break;
 
                case clc_disconnect: