[Freeciv-Dev] Re: (PR#13262) pubserver-in-a-diff

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Per I. Mathisen wrote:

> Index: common/game.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/game.c,v
> retrieving revision 1.220
> diff -u -r1.220 game.c
> --- common/game.c     21 Jun 2005 16:21:01 -0000      1.220
> +++ common/game.c     22 Jun 2005 20:34:23 -0000
> @@ -233,7 +233,11 @@
>    game.info.cooling       = 0;
>    game.info.watchtower_extra_vision = GAME_DEFAULT_WATCHTOWER_EXTRA_VISION;
>    game.info.allowed_city_names = GAME_DEFAULT_ALLOWED_CITY_NAMES;
> -  game.info.save_nturns   = 10;
> +  if (game.info.pubserver > 0) {
> +    game.info.save_nturns   = 1;
> +  } else {
> +    game.info.save_nturns   = GAME_DEFAULT_SAVETURNS;
> +  }

This will work, but not perfectly.  The saveturns will be set but the
*default* saveturns will remain at ten.  If we introduce a /reset
command (which will be useful in conjunction with /ruleset) this will
stop working.

I don't see an easy way to fix this however.  If this change is
committed a new ticket for this issue should be created.

> +    fc_fprintf(stderr, _("  -P, --Pubserver NUM\tLaunch in pubserver "
> +                         "mode with game number NUM\n"));

This help is insufficient.  At a minimum it needs to mention that -e is
also required (when running a pubserver in daemon mode it would be hard
to debug a missing -e), and point to a URL or README file where more
info about pubservers can be found.

> +      /* If we load a game, some players may be assigned to user accounts,
> +       * in which case we should not start until they have joined too. 
> +       * Otherwise it would be easy to cheat. This will be problematic
> +       * for scenarios where creators forget to reset usernames. */
> +      if (pplayer->is_ready && pplayer->is_connected) {
> +     num_ready++;
> +      } else if (pplayer->is_connected 
> +                 || is_valid_username(pplayer->username)) {
> +     num_unready++;

Is this supposed to apply in non-pubserver mode too?

Perhaps the check should be changed so that all non-AI players must be
ready.  Then if you have (in new games or loaded games) a created player
you must either wait for someone to connect to him or aitoggle him.  And
this should apply in both modes.

>    /* Run server loop */
>    while (TRUE) {
> +    if (game.info.pubserver > 0) {
> +      DIR *dir;
> +      char path[20], gamelogfile[40], conlogfile[40];
> +
> +      /* Create directory */
> +      my_snprintf(path, sizeof(path), "%d", game.info.pubserver);
> +      dir = opendir(path);
> +      if (dir != NULL) {
> +        freelog(LOG_FATAL, "Game directory \"%s\" already exists!", path);
> +        exit(EXIT_FAILURE);
> +      }
> +      make_dir(path);
> +      dir = opendir(path);
> +      if (dir == NULL) {
> +        freelog(LOG_FATAL, "Could not create directory \"%s\".", path);
> +        exit(EXIT_FAILURE);
> +      }
> +      sz_strlcpy(srvarg.final_savepath, path);
> +      /* Redirect output to saves directory */
> +      my_snprintf(gamelogfile, sizeof(gamelogfile), "%s/gamelog.txt", 
> +                  srvarg.final_savepath);
> +      my_snprintf(conlogfile, sizeof(conlogfile), "%s/cmdline.txt", 
> +                  srvarg.final_savepath);
> +      con_log_init(conlogfile, srvarg.loglevel);
> +      gamelog_init(gamelogfile);
> +    }
> +

This should be moved into a helper function.

> Index: server/stdinhand.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/stdinhand.c,v
> retrieving revision 1.422
> diff -u -r1.422 stdinhand.c
> --- server/stdinhand.c        21 Jun 2005 10:17:03 -0000      1.422
> +++ server/stdinhand.c        22 Jun 2005 20:34:24 -0000
> @@ -116,6 +116,16 @@
>  Returns whether the specified server setting (option) should be
>  sent to the client.
>  *********************************************************************/
> +static bool restricted_filepaths(struct connection *caller)
> +{
> +  return (game.info.pubserver > 0
> +          || (caller && caller->access_level != ALLOW_HACK));
> +}

Should be access_level < ALLOW_HACK?  Or does this matter?

> @@ -755,6 +765,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;
> +  }

This check doesn't look useful.  Basically the only added effect is to
prevent admin saves on pubservers.  Why not just restrict /save to hack
users?


> +  if (restricted_filepaths(caller)) {
> +    if (!is_ascii_name(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);
> +  }

I think this check is insufficent or at least risky.  Rather than
excluding certain characters we need to only include the allowed ones
(alphanumerics).  is_ascii_name isn't intended to be used for security
and could be changed.

> @@ -1105,7 +1148,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) {

Again, why not just restrict /write to hack access?

> +  if (!is_ascii_name(filename) && restricted_filepaths(caller)) {
> +    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Name \"%s\" disallowed for "
> +              "security reasons."), filename);
> +    return FALSE;
> +  }

Same security problem here (and it occurrs a few places lower down too).

> -  if (!arg || arg[0] == '\0') {
> -    cmd_reply(CMD_LOAD, caller, C_FAIL, _("Usage: load <filename>"));
> -    send_load_game_info(FALSE);
> -    return FALSE;
> +    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;
> +          }
> +        }
> +      }
> +    }

Very ugly.  Isn't this already done somewhere else?  We shouldn't
duplicate it.

> -    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);

These contraction fixes seem unrelated (and there are some other
unrelated changes too).  The patch is big enough without them...

> Index: utility/shared.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/utility/shared.c,v
> retrieving revision 1.133
> diff -u -r1.133 shared.c
> --- utility/shared.c  15 Jun 2005 20:23:00 -0000      1.133
> +++ utility/shared.c  22 Jun 2005 20:34:24 -0000
> @@ -385,7 +385,8 @@
>  ***************************************************************/
>  bool is_ascii_name(const char *name)
>  {
> -  const char illegal_chars[] = {'|', '%', '"', ',', '*', '<', '>', '\0'};
> +  const char illegal_chars[] = { '|', '%', '"', ',', '*', '<', '>', 
> +                                 '/', '\\', '$', '`', '\0' };
>    int i, j;
>  
>    /* must not be NULL or empty */

Basically I think using is_ascii_name for security isn't a good idea.
Even if we make another identical function
is_safe_unpriviledged_filename that is identical it would be better.
Some future programmer may "fix" is_ascii_name and then the security
would break.

But is_safe_unpriviledged_filename shouldn't list disallowed characters
but should include only allowed characters.  a-zA-Z0-9_- are all fine.
Maybe . is okay too, not sure about that.

Finally, my review shouldn't be considered as good as an audit.  Before
a patch like this could be committed we'd need a very careful audit of
the commands that you've changed from hack to ctrl.

-jason