[Libguestfs] [PATCH nbdkit v2 4/4] server: debug: Escape debug strings

[Laszlo Ersek]

On 5/9/23 11:51, Richard W.M. Jones wrote:
> Debug strings contain all kinds of information including some under
> user control.  Previously we simply sent everything to stderr, but
> this is potentially insecure, as well as not dealing well with
> non-printable characters.  Escape these strings when printing.
> ---
>  server/debug.c | 52 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/server/debug.c b/server/debug.c
> index 2231d40a0..1fc56ec50 100644
> --- a/server/debug.c
> +++ b/server/debug.c
> @@ -42,6 +42,7 @@
>  
>  #include "ansi-colours.h"
>  #include "open_memstream.h"
> +#include "utils.h"
>  
>  #include "internal.h"
>  
> @@ -69,19 +70,22 @@ prologue (FILE *fp)
>  static void
>  debug_common (bool in_server, const char *fs, va_list args)
>  {
> -  int err = errno;
> -#ifndef WIN32
> -  int tty;
> -#endif
> -  CLEANUP_FREE char *str = NULL;
> -  size_t len = 0;
> -  FILE *fp;
> -
>    if (!verbose)
>      return;
>  
> -  fp = open_memstream (&str, &len);
> -  if (fp == NULL) {
> +  int err = errno;
> +
> +#ifndef WIN32
> +  int tty;
> +#endif
> +  CLEANUP_FREE char *str_inner = NULL;
> +  CLEANUP_FREE char *str_outer = NULL;
> +  FILE *fp_inner, *fp_outer;
> +  size_t len_inner = 0, len_outer = 0;
> +
> +  /* The "inner" string is the debug string before escaping. */
> +  fp_inner = open_memstream (&str_inner, &len_inner);
> +  if (fp_inner == NULL) {
>    fail:
>      /* Try to emit what we can. */
>      errno = err;
> @@ -89,28 +93,34 @@ debug_common (bool in_server, const char *fs, va_list args)
>      fprintf (stderr, "\n");
>      return;
>    }
> +  errno = err;
> +  vfprintf (fp_inner, fs, args);
> +  close_memstream (fp_inner);
> +
> +  /* The "outer" string contains the prologue, escaped debug string and \n. */
> +  fp_outer = open_memstream (&str_outer, &len_outer);
> +  if (fp_outer == NULL) goto fail;
>  
>  #ifndef WIN32
>    tty = isatty (fileno (stderr));
> -  if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK, fp);
> +  if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK, fp_outer);
>  #endif
>  
> -  prologue (fp);
> -
> -  errno = err;
> -  vfprintf (fp, fs, args);
> +  prologue (fp_outer);
> +  c_string_quote (str_inner, fp_outer);
>  
>  #ifndef WIN32
> -  if (!in_server && tty) ansi_force_restore (fp);
> +  if (!in_server && tty) ansi_force_restore (fp_outer);
>  #endif
> -  fprintf (fp, "\n");
> -  close_memstream (fp);
> +  fprintf (fp_outer, "\n");
> +  close_memstream (fp_outer);
>  
> -  if (str)
> -    fputs (str, stderr);
> -  else
> +  if (!str_outer)
>      goto fail;
>  
> +  /* Send it to stderr as atomically as possible. */
> +  fputs (str_outer, stderr);
> +
>    errno = err;
>  }
>  

In my opinion (now that I'm looking at it), debug_common() has several
issues *pre-patch* that I believe we should fix, before making further
changes (and duplicating some of those issues!) here:

(1) The top-level comment says "preserves the previous value of errno".
This is not guaranteed if open_memstream() fails, and we call vfprintf()
+ fprintf() pointed at "stderr".

(2) The "fail" label placed within the body of an "if", plus the
backwards jump to it, is just... truly bad.

In fact, I see problem (1) as a direct consequence of problem (2): lack
of a principled error handling epilogue (2) causes us to break the
interface contract (1), because we don't reach the end of the function
on one of the (nontrivial) error paths.

(3) The return value of close_memstream() is not checked. We modify the
memory stream in multiple steps, so it is theoretically possible that at
least one of those steps, but not all of them, succeeds. That way "str"
will be non-NULL in the end, but the contents will not be what we want
it to be. The proper way to deal with this is to let the stdio stream
error indicator trickle through to close_memstream() -- normally:
fclose() --, and then for us to check the return value. On Windows, the
replacement code does not modify "str" until the very end indeed, but
that's not something we should rely on Linux. And checking the retval of
close_memstream() works on both Linux and Windows (the Windows
replacement returns 0 only after setting "str").


Then, in the post-patch version, the following catches my eye:

  fp_inner = open_memstream (&str_inner, &len_inner);
  if (fp_inner == NULL) {
  fail:
    /* Try to emit what we can. */
    errno = err;
    vfprintf (stderr, fs, args);
    fprintf (stderr, "\n");
    return;
  }
  errno = err;
  vfprintf (fp_inner, fs, args);

Here a common "errno = err" assignment can be factored out, placed just
after the "fp_inner" assignment. But this isn't really relevant, as
fixing problem (2) -- i.e., moving "fail" at the end of the function --
might reorder the code such that this code extraction is no longer
needed or possible.

Thanks!
Laszlo