[Freeciv-Dev] (PR#13262) Security changes

["Per I. Mathisen" <per@xxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=13262 >

This is the security improvements / permissions changes from the pubserver
patch put in a separate patch. Most notable is the making of 'load' and
'read' options into 'ctrl' instead of 'hack' cmdlevel.

I intend to commit this patch soon, so please read carefully.

  - Per

Index: server/commands.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/commands.c,v
retrieving revision 1.12
diff -u -r1.12 commands.c
--- server/commands.c   21 May 2005 19:40:25 -0000      1.12
+++ server/commands.c   25 Jul 2005 11:41:14 -0000
@@ -337,7 +337,7 @@
       "    --file <filename>\n"
       "and use the 'start' command once players have reconnected.")
   },
-  {"load",      ALLOW_HACK,
+  {"load",      ALLOW_CTRL,
    /* TRANS: translate text between <> only */
    N_("load\n"
       "load <file-name>"),
@@ -345,7 +345,7 @@
    N_("Load a game from <file-name>. Any current data including players, "
       "rulesets and server options are lost.\n")
   },
-  {"read",     ALLOW_HACK,
+  {"read",     ALLOW_CTRL,
    /* TRANS: translate text between <> only */
    N_("read <file-name>"),
    N_("Process server commands from file."), NULL
Index: server/console.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/console.c,v
retrieving revision 1.25
diff -u -r1.25 console.c
--- server/console.c    21 Mar 2005 23:01:12 -0000      1.25
+++ server/console.c    25 Jul 2005 11:41:15 -0000
@@ -47,9 +47,8 @@
 ************************************************************************/
 static void con_handle_log(int level, const char *message, bool file_too)
 {
-  /* Write to console only when not written to file.
-     LOG_FATAL messages always to console too. */
-  if (! file_too || level <= LOG_FATAL) {
+  /* Write debug/verbose message to console only when not written to file. */
+  if (!file_too || level <= LOG_NORMAL) {
     if (console_rfcstyle) {
       con_write(C_LOG_BASE + level, "%s", message);
     } else {
Index: server/gamelog.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gamelog.c,v
retrieving revision 1.52
diff -u -r1.52 gamelog.c
--- server/gamelog.c    23 Jul 2005 18:02:44 -0000      1.52
+++ server/gamelog.c    25 Jul 2005 11:41:15 -0000
@@ -33,7 +33,7 @@
 #include "stdinhand.h"
 
 int gamelog_level;             /* also accessed from stdinhand.c */
-static char *gamelog_filename;
+static char *gamelog_filename = NULL;
 
 /* Stuff for gamelog_status() */
 struct player_score_entry {
@@ -75,6 +75,7 @@
   gamelog_level = GAMELOG_FULL;
   if (gamelog_filename) {
     free(gamelog_filename);
+    gamelog_filename = NULL;
   }
   if (filename && strlen(filename) > 0) {
     gamelog_filename = strdup(filename);
Index: server/stdinhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/stdinhand.c,v
retrieving revision 1.427
diff -u -r1.427 stdinhand.c
--- server/stdinhand.c  18 Jul 2005 22:46:29 -0000      1.427
+++ server/stdinhand.c  25 Jul 2005 11:41:18 -0000
@@ -112,6 +112,14 @@
 
"------------------------------------------------------------------------------";
 
 /********************************************************************
+  Are we operating under a restricted paths regime?
+*********************************************************************/
+static bool restricted_filepaths(struct connection *caller)
+{
+  return (caller && caller->access_level != ALLOW_HACK);
+}
+
+/********************************************************************
 Returns whether the specified server setting (option) should be
 sent to the client.
 *********************************************************************/
@@ -744,6 +752,11 @@
 **************************************************************************/
 static bool save_command(struct connection *caller, char *arg, bool check)
 {
+  if (restricted_filepaths(caller)) {
+    cmd_reply(CMD_SAVE, caller, C_FAIL,
+              _("You cannot save games manually on this server."));
+    return FALSE;
+  }
   if (!check) {
     save_game(arg, "User request");
   }
@@ -959,22 +972,51 @@
 
 /**************************************************************************
   Returns FALSE iff there was an error.
+
+  Security: We will look for a file with mandatory extension '.serv',
+  and on public servers we will not look outside the data directories.
+  As long as the user cannot create files with arbitrary names in the
+  root of the data directories, this should ensure that we will not be 
+  tricked into loading non-approved content. The script is read with the 
+  permissions of the caller, so it will in any case not lead to elevated
+  permissions unless there are other bugs.
 **************************************************************************/
-bool read_init_script(struct connection *caller, char *script_filename)
+bool read_init_script(struct connection *caller, char *script_filename,
+                      bool from_cmdline)
 {
   FILE *script_file;
+  const char extension[] = ".serv";
+  char serv_filename[strlen(extension) + strlen(script_filename) + 2];
   char tilde_filename[4096];
   char *real_filename;
 
-  interpret_tilde(tilde_filename, sizeof(tilde_filename), script_filename);
+  my_snprintf(serv_filename, sizeof(serv_filename), "%s%s", 
+              script_filename, extension);
+
+  if (restricted_filepaths(caller) && !from_cmdline) {
+    if (!is_safe_filename(serv_filename)) {
+      cmd_reply(CMD_READ_SCRIPT, caller, C_FAIL,
+                _("Name \"%s\" disallowed for security reasons."), 
+                serv_filename);
+      return FALSE;
+    }
+    sz_strlcpy(tilde_filename, serv_filename);
+  } else {
+    interpret_tilde(tilde_filename, sizeof(tilde_filename), serv_filename);
+  }
 
   real_filename = datafilename(tilde_filename);
   if (!real_filename) {
+    if (restricted_filepaths(caller) && !from_cmdline) {
+      cmd_reply(CMD_READ_SCRIPT, caller, C_FAIL,
+                _("No command script found by the name \"%s\"."), 
+                serv_filename);
+      return FALSE;
+    }
+    /* File is outside data directories */
     real_filename = tilde_filename;
   }
 
-  /* This used to print out the script_filename, but it seems more useful
-   * to show the real_filename. */
   freelog(LOG_NORMAL, _("Loading script file: %s"), real_filename);
 
   if (is_reg_file_for_access(real_filename, FALSE)
@@ -982,7 +1024,7 @@
     char buffer[MAX_LEN_CONSOLE_LINE];
     /* the size is set as to not overflow buffer in handle_stdin_input */
     while(fgets(buffer,MAX_LEN_CONSOLE_LINE-1,script_file))
-      handle_stdin_input((struct connection *)NULL, buffer, FALSE);
+      handle_stdin_input(caller, buffer, FALSE);
     fclose(script_file);
     return TRUE;
   } else {
@@ -1003,7 +1045,7 @@
     return TRUE; /* FIXME: no actual checks done */
   }
   /* warning: there is no recursion check! */
-  return read_init_script(caller, arg);
+  return read_init_script(caller, arg, FALSE);
 }
 
 /**************************************************************************
@@ -1094,7 +1136,11 @@
 **************************************************************************/
 static bool write_command(struct connection *caller, char *arg, bool check)
 {
-  if (!check) {
+  if (restricted_filepaths(caller)) {
+    cmd_reply(CMD_WRITE_SCRIPT, caller, C_OK, _("You cannot use the write "
+              "command on this server for security reasons."));
+    return FALSE;
+  } else if (!check) {
     write_init_script(arg);
   }
   return TRUE;
@@ -3143,20 +3189,45 @@
 {
   struct timer *loadtimer, *uloadtimer;  
   struct section_file file;
-  char arg[strlen(filename) + 1];
-
-  /* We make a local copy because the parameter might be a pointer to 
-   * srvarg.load_filename, which we edit down below. */
-  sz_strlcpy(arg, filename);
+  char arg[512 + strlen(filename)];
 
-  if (!arg || arg[0] == '\0') {
-    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Usage: load <filename>"));
-    send_load_game_info(FALSE);
+  if (!filename || filename[0] == '\0') {
+    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Usage: load <game name>"));
+    return FALSE;
+  }
+  if (!is_safe_filename(filename) && restricted_filepaths(caller)) {
+    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Name \"%s\" disallowed for "
+              "security reasons."), filename);
     return FALSE;
   }
+  {
+    /* it is a normal savegame or maybe a scenario */
+    char tmp[256 + strlen(filename)];
+
+    my_snprintf(tmp, sizeof(tmp), "%s.sav", filename);
+    if (!datafilename(tmp)) {
+      my_snprintf(tmp, sizeof(tmp), "%s.sav.gz", filename);
+      if (!datafilename(tmp)) {
+        my_snprintf(tmp, sizeof(tmp), "scenario/%s.sav", filename);
+        if (!datafilename(tmp)) {
+          my_snprintf(tmp, sizeof(tmp), "scenario/%s.sav.gz", filename);
+          if (restricted_filepaths(caller) && !datafilename(tmp)) {
+            cmd_reply(CMD_LOAD, caller, C_FAIL, _("Cannot find savegame or "
+                      "scenario with the name \"%s\"."), filename);
+            return FALSE;
+          }
+        }
+      }
+    }
+    if (datafilename(tmp)) {
+      sz_strlcpy(arg, datafilename(tmp));
+    } else {
+      sz_strlcpy(arg, filename);
+    }
+  }
 
   if (server_state != PRE_GAME_STATE) {
-    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Can't load a game while another "
+    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Cannot load a game while another "
                                           "is running."));
     send_load_game_info(FALSE);
     return FALSE;
@@ -3165,7 +3236,7 @@
   /* attempt to parse the file */
 
   if (!section_file_load_nodup(&file, arg)) {
-    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Couldn't load savefile: %s"), arg);
+    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Could not load savefile: %s"), arg);
     send_load_game_info(FALSE);
     return FALSE;
   }
@@ -3216,7 +3287,13 @@
 }
 
 /**************************************************************************
-  ...
+  Load rulesets from a given ruleset directory.
+
+  Security: There are some rudimentary checks in load_rulesets() to see
+  if this directory realls is a viable ruleset directory. For public
+  servers, we check against directory redirection (is_safe_filename) and
+  other bad stuff in the directory name, and will only use directories
+  inside the data directories.
 **************************************************************************/
 static bool set_rulesetdir(struct connection *caller, char *str, bool check)
 {
@@ -3226,6 +3303,13 @@
              _("Current ruleset directory is \"%s\""), game.rulesetdir);
     return FALSE;
   }
+  if (restricted_filepaths(caller)
+      && (!is_safe_filename(str) || strchr(str, '.'))) {
+    cmd_reply(CMD_RULESETDIR, caller, C_SYNTAX,
+             _("Ruleset directory name \"%s\" disallowed for security "
+               "reasons."), str);
+    return FALSE;
+  }  
   my_snprintf(filename, sizeof(filename), "%s", str);
   pfilename = datafilename(filename);
   if (!pfilename) {
Index: server/stdinhand.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/stdinhand.h,v
retrieving revision 1.30
diff -u -r1.30 stdinhand.h
--- server/stdinhand.h  9 Dec 2004 16:38:35 -0000       1.30
+++ server/stdinhand.h  25 Jul 2005 11:41:19 -0000
@@ -28,7 +28,8 @@
 void report_settable_server_options(struct connection *dest, int which);
 void set_ai_level_direct(struct player *pplayer, int level);
 void set_ai_level_directer(struct player *pplayer, int level);
-bool read_init_script(struct connection *caller, char *script_filename);
+bool read_init_script(struct connection *caller, char *script_filename,
+                      bool from_cmdline);
 void show_players(struct connection *caller);
 
 bool load_command(struct connection *caller, char *arg, bool check);
Index: utility/log.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/utility/log.c,v
retrieving revision 1.48
diff -u -r1.48 log.c
--- utility/log.c       12 Jul 2005 21:25:48 -0000      1.48
+++ utility/log.c       25 Jul 2005 11:41:19 -0000
@@ -180,6 +180,7 @@
   fc_log_level = initial_level;
   if (log_filename) {
     free(log_filename);
+    log_filename = NULL;
   }
   if (filename && strlen(filename) > 0) {
     log_filename = strdup(filename);