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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Per I. Mathisen wrote:


> -  /* 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 {

What's the purpose of this change?  In pubserver stderr is redirected to
/dev/null so writing LOG_ERRORs shouldn't hurt.  In logged games where
it's not redirected having the message written to stderr would probably
be desirable.

> -static char *gamelog_filename;
> +static char *gamelog_filename = NULL;

This change is unnecessary (but harmless).  However we shouldn't get
into the habit of going through the code making (or reversing) changes
like this unless we have an official policy on it.

>  /********************************************************************
> +  Are we operating under a restricted paths regime?
> +*********************************************************************/
> +static bool restricted_filepaths(struct connection *caller)
> +{
> +  return (caller && caller->access_level != ALLOW_HACK);
> +}

Is this really the right name?  What does this have to do with
filepaths?  And can you prepend an is_ to it?  Maybe
is_restricted_access?  (It's a local function so naming isn't that
crucial though.)

Callers of this generally should have a comment explaining why it's
used.  In fact the whole patch needs more comments since this is very
sensitive code that people need to understand well so as to not break.

> +  if (restricted_filepaths(caller) && !from_cmdline) {

Why the extra from_cmdline check here?  Doesn't is_restricted_filepaths
already cover this?

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

This is useful comment; why remove it?  IMO it would be more useful to
show the script_filename on pubserver however...

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

This means a script executes with the same permissions as the caller
right?  (It needs a comment explaining this.)  This is certainly much
safer but could be wrong in some cases.  One might want a script that
changes ctrl-level settings.  This allows users to change those settings
but only to new values specifically allowed by the script author/server
admin.  However one must be very careful then not to write dangerous
scripts.  So it could go either way...

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

Side note: maybe there should be a recursion check.  Limiting the
nesting levels of scripts should be sufficient in most cases.

>  /**************************************************************************
> @@ -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) {

Here's an example where restricted_filepaths seems like the wrong name.

> +  char arg[512 + strlen(filename)];

> +    char tmp[256 + strlen(filename)];

Please add a comment telling why 512 and 256 are chosen.

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

Side note: maybe datafilename should automatically look for suffixes...

Another side note: datafilename doesn't do any mallocing.  However it
does have one single static buffer which means you can't use it more
than once at a time.  This is dangerous and should probably be fixed.

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

Hmm, should we put something about contractions into the style guide?


> +  if (restricted_filepaths(caller)
> +      && (!is_safe_filename(str) || strchr(str, '.'))) {

. isn't allowed so the last check shouldn't be necessary.  Maybe this
should be an assertion or the check left in, just to be safe, however.

.....

Basically it looks good; most of the above is nit-picking.  But I do
think the patch needs a lot more explanatory comments.

-jason