Libguestfs - May 2023 - [PATCH nbdkit v3 6/6] server: debug: Escape debug strings

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 | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/server/debug.c b/server/debug.c
index 177d9d5da..6cf1af316 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"
 
@@ -76,36 +77,43 @@ debug_common (bool in_server, const char *fs, va_list args)
 #ifndef WIN32
   int tty;
 #endif
-  CLEANUP_FREE char *str = NULL;
-  size_t len = 0;
-  FILE *fp;
+  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;
 
-  fp = open_memstream (&str, &len);
-  if (fp == NULL)
+  /* The "inner" string is the debug string before escaping. */
+  fp_inner = open_memstream (&str_inner, &len_inner);
+  if (fp_inner == NULL)
     goto fail;
+  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");
-  if (close_memstream (fp) == -1)
+  fprintf (fp_outer, "\n");
+  if (close_memstream (fp_outer) == -1)
     goto fail;
 
-  if (!str)
+  if (!str_outer)
     goto fail;
 
   /* Send it to stderr as atomically as possible. */
-  fputs (str, stderr);
+  fputs (str_outer, stderr);
 
   errno = err;
   return;
-- 
2.39.2

On Tue, May 09, 2023 at 03:51:21PM +0100, 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 | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/server/debug.c b/server/debug.c
> index 177d9d5da..6cf1af316 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"
>  
> @@ -76,36 +77,43 @@ debug_common (bool in_server, const char *fs, va_list
args)
>  #ifndef WIN32
>    int tty;
>  #endif
> -  CLEANUP_FREE char *str = NULL;
> -  size_t len = 0;
> -  FILE *fp;
> +  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;
>  
> -  fp = open_memstream (&str, &len);
> -  if (fp == NULL)
> +  /* The "inner" string is the debug string before escaping. */
> +  fp_inner = open_memstream (&str_inner, &len_inner);
> +  if (fp_inner == NULL)
>      goto fail;
> +  errno = err;
> +  vfprintf (fp_inner, fs, args);
> +  close_memstream (fp_inner);
Missing the check for failure.
> +
> +  /* 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");
> -  if (close_memstream (fp) == -1)
> +  fprintf (fp_outer, "\n");
> +  if (close_memstream (fp_outer) == -1)
Affects multiple patches: close_memstream() (on Linux) fails with EOF,
which happens to be -1 on all sane libc, but which POSIX says can be
any negative value.  '< 0' or at least '== EOF' is probably
safer than
'== -1'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org