Libguestfs - Dec 2016 - Re: [PATCH 2/2] launch: Validate $TERM before passing it through to the kernel command line.

This is just code refactoring.
---
 src/launch.c | 172 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 109 insertions(+), 63 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index 46d7ab9..84d5e82 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -293,9 +293,7 @@ guestfs_impl_config (guestfs_h *g,
 #endif
 
 #if defined(__aarch64__)
-#define EARLYPRINTK " earlyprintk=pl011,0x9000000"
-#else
-#define EARLYPRINTK ""
+#define EARLYPRINTK "earlyprintk=pl011,0x9000000"
 #endif
 
 /**
@@ -327,75 +325,123 @@ char *
 guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev,
 				    int flags)
 {
-  char root[64] = "";
+  CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (argv);
   char *term = getenv ("TERM");
-  char *ret;
   bool tcg = flags & APPLIANCE_COMMAND_LINE_IS_TCG;
-  char lpj_s[64] = "";
 
-  if (appliance_dev)
-    snprintf (root, sizeof root, " root=%s", appliance_dev);
+  /* We assemble the kernel command line by simply joining the final
+   * list of strings with spaces.  This means (a) the strings are not
+   * quoted (it's not clear if the kernel can handle quoting in any
+   * case), and (b) we can append multiple parameters in a single
+   * argument, as we must do for the g->append parameter.
+   */
+
+  /* Force kernel to panic if daemon exits. */
+  guestfs_int_add_string (g, &argv, "panic=1");
+
+#ifdef __arm__
+  guestfs_int_add_sprintf (g, &argv, " mem=%dM", g->memsize);
+#endif
+
+#ifdef __i386__
+  /* Workaround for RHBZ#857026. */
+  guestfs_int_add_string (g, &argv, "noapic");
+#endif
+
+  /* Serial console. */
+  guestfs_int_add_string (g, &argv, SERIAL_CONSOLE);
+
+#ifdef EARLYPRINTK
+  /* Get messages from early boot. */
+  guestfs_int_add_string (g, &argv, EARLYPRINTK);
+#endif
+
+#ifdef __aarch64__
+  guestfs_int_add_string (g, &argv, "ignore_loglevel");
+
+  /* This option turns off the EFI RTC device.  QEMU VMs don't
+   * currently provide EFI, and if the device is compiled in it
+   * will try to call the EFI function GetTime unconditionally
+   * (causing a call to NULL).  However this option requires a
+   * non-upstream patch.
+   */
+  guestfs_int_add_string (g, &argv, "efi-rtc=noprobe");
+#endif
+
+  /* RHBZ#1404287 */
+  guestfs_int_add_string (g, &argv, "edd=off");
+
+  /* For slow systems (RHBZ#480319, RHBZ#1096579). */
+  guestfs_int_add_string (g, &argv, "udevtimeout=6000");
+
+  /* Same as above, for newer udevd. */
+  guestfs_int_add_string (g, &argv, "udev.event-timeout=6000");
+
+  /* Fix for RHBZ#502058. */
+  guestfs_int_add_string (g, &argv, "no_timer_check");
 
   if (tcg) {
     const int lpj = guestfs_int_get_lpj (g);
     if (lpj > 0)
-      snprintf (lpj_s, sizeof lpj_s, " lpj=%d", lpj);
+      guestfs_int_add_sprintf (g, &argv, "lpj=%d", lpj);
   }
 
-  ret = safe_asprintf
-    (g,
-     "panic=1"             /* force kernel to panic if daemon exits
*/
-#ifdef __arm__
-     " mem=%dM"
-#endif
-#ifdef __i386__
-     " noapic"                  /* workaround for RHBZ#857026 */
-#endif
-     " " SERIAL_CONSOLE         /* serial console */
-     EARLYPRINTK                /* get messages from early boot */
-#ifdef __aarch64__
-     " ignore_loglevel"
-     /* This option turns off the EFI RTC device.  QEMU VMs don't
-      * currently provide EFI, and if the device is compiled in it
-      * will try to call the EFI function GetTime unconditionally
-      * (causing a call to NULL).  However this option requires a
-      * non-upstream patch.
-      */
-     " efi-rtc=noprobe"
-#endif
-     " edd=off"                 /* RHBZ#1404287 */
-     " udevtimeout=6000"/* for slow systems (RHBZ#480319,
RHBZ#1096579) */
-     " udev.event-timeout=6000" /* for newer udevd */
-     " no_timer_check"  /* fix for RHBZ#502058 */
-     "%s"               /* lpj */
-     " printk.time=1"   /* display timestamp before kernel messages
*/
-     " cgroup_disable=memory"   /* saves us about 5 MB of RAM */
-     " usbcore.nousb"           /* disable USB, only saves about 1ms
*/
-     " cryptomgr.notests"       /* disable crypto tests, saves 28ms
*/
-     " tsc=reliable"            /* don't synch TSCs when using
SMP,
-                                   saves 21ms for each secondary vCPU */
-     " 8250.nr_uarts=1"         /* don't scan all 8250 UARTS */
-     "%s"                       /* root=appliance_dev */
-     " %s"                      /* selinux */
-     " %s"                      /* quiet/verbose */
-     "%s"                       /* network */
-     " TERM=%s"                 /* TERM environment variable */
-     "%s%s"                     /* handle identifier */
-     "%s%s",                    /* append */
-#ifdef __arm__
-     g->memsize,
-#endif
-     lpj_s,
-     root,
-     g->selinux ? "selinux=1 enforcing=0" : "selinux=0",
-     g->verbose ? "guestfs_verbose=1" : "quiet",
-     g->enable_network ? " guestfs_network=1" : "",
-     term ? term : "linux",
-     STRNEQ (g->identifier, "") ? " guestfs_identifier="
: "",
-     g->identifier,
-     g->append ? " " : "", g->append ? g->append :
"");
-
-  return ret;
+  /* Display timestamp before kernel messages. */
+  guestfs_int_add_string (g, &argv, "printk.time=1");
+
+  /* Saves us about 5 MB of RAM. */
+  guestfs_int_add_string (g, &argv, "cgroup_disable=memory");
+
+  /* Disable USB, only saves about 1ms. */
+  guestfs_int_add_string (g, &argv, "usbcore.nousb");
+
+  /* Disable crypto tests, saves 28ms. */
+  guestfs_int_add_string (g, &argv, "cryptomgr.notests");
+
+  /* Don't synch TSCs when using SMP.  Saves 21ms for each secondary vCPU.
*/
+  guestfs_int_add_string (g, &argv, "tsc=reliable");
+
+  /* Don't scan all 8250 UARTS. */
+  guestfs_int_add_string (g, &argv, "8250.nr_uarts=1");
+
+  /* Tell supermin about the appliance device. */
+  if (appliance_dev)
+    guestfs_int_add_sprintf (g, &argv, "root=%s", appliance_dev);
+
+  /* SELinux - deprecated setting, never worked and should not be enabled. */
+  if (g->selinux)
+    guestfs_int_add_string (g, &argv, "selinux=1 enforcing=0");
+  else
+    guestfs_int_add_string (g, &argv, "selinux=0");
+
+  /* Quiet/verbose. */
+  if (g->verbose)
+    guestfs_int_add_string (g, &argv, "guestfs_verbose=1");
+  else
+    guestfs_int_add_string (g, &argv, "quiet");
+
+  /* Network. */
+  if (g->enable_network)
+    guestfs_int_add_string (g, &argv, "guestfs_network=1");
+
+  /* TERM environment variable. */
+  if (term)
+    guestfs_int_add_sprintf (g, &argv, "TERM=%s", term);
+  else
+    guestfs_int_add_string (g, &argv, "TERM=linux");
+
+  /* Handle identifier. */
+  if (STRNEQ (g->identifier, ""))
+    guestfs_int_add_sprintf (g, &argv, "guestfs_identifier=%s",
g->identifier);
+
+  /* Append extra arguments. */
+  if (g->append)
+    guestfs_int_add_string (g, &argv, g->append);
+
+  guestfs_int_end_stringsbuf (g, &argv);
+
+  /* Caller frees. */
+  return guestfs_int_join_strings (" ", argv.argv);
 }
 
 /**
-- 
2.10.2

Make sure it is reasonable before we pass it through to the kernel
command line.  I don't believe this is exploitable, but it might cause
obscure bugs.
---
 src/launch.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/launch.c b/src/launch.c
index 84d5e82..ee2a23d 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -39,6 +39,8 @@
 #include <assert.h>
 #include <libintl.h>
 
+#include "c-ctype.h"
+
 #include "guestfs.h"
 #include "guestfs-internal.h"
 #include "guestfs-internal-actions.h"
@@ -284,6 +286,28 @@ guestfs_impl_config (guestfs_h *g,
   return 0;
 }
 
+/**
+ * Check that the $TERM environment variable is reasonable before
+ * we pass it through to the appliance.
+ */
+static int
+valid_term (const char *term)
+{
+  size_t len = strlen (term);
+
+  if (len == 0 || len > 16)
+    return 0;
+
+  while (len > 0) {
+    char c = *term++;
+    len--;
+    if (!c_isalnum (c) && c != '-' && c != '_')
+      return 0;
+  }
+
+  return 1;
+}
+
 #if defined(__powerpc64__)
 #define SERIAL_CONSOLE "console=hvc0 console=ttyS0"
 #elif defined(__arm__) || defined(__aarch64__)
@@ -425,7 +449,7 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char
*appliance_dev,
     guestfs_int_add_string (g, &argv, "guestfs_network=1");
 
   /* TERM environment variable. */
-  if (term)
+  if (term && valid_term (term))
     guestfs_int_add_sprintf (g, &argv, "TERM=%s", term);
   else
     guestfs_int_add_string (g, &argv, "TERM=linux");
-- 
2.10.2

On Sunday, 18 December 2016 20:09:28 CET Richard W.M. Jones
wrote:
> This is just code refactoring.
> ---
Mostly LGTM, just one note.
> +  return guestfs_int_join_strings (" ", argv.argv);
Currently, safe_asprintf aborts on ENOMEM errors, while
guestfs_int_join_strings just returns NULL on that situation -- most
probably the abort callback needs to be invoked manually:

  char *ret;
  ...
  ret = guestfs_int_join_strings (" ", argv.argv);
  if (ret == NULL)
    g->abort_cb ();

  return ret;

Thanks,
-- 
Pino Toscano

On Sunday, 18 December 2016 20:09:29 CET Richard W.M. Jones
wrote:
> Make sure it is reasonable before we pass it through to the kernel
> command line.  I don't believe this is exploitable, but it might cause
> obscure bugs.
> ---
>  src/launch.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/launch.c b/src/launch.c
> index 84d5e82..ee2a23d 100644
> --- a/src/launch.c
> +++ b/src/launch.c
> @@ -39,6 +39,8 @@
>  #include <assert.h>
>  #include <libintl.h>
>  
> +#include "c-ctype.h"
> +
>  #include "guestfs.h"
>  #include "guestfs-internal.h"
>  #include "guestfs-internal-actions.h"
> @@ -284,6 +286,28 @@ guestfs_impl_config (guestfs_h *g,
>    return 0;
>  }
>  
> +/**
> + * Check that the $TERM environment variable is reasonable before
> + * we pass it through to the appliance.
> + */
> +static int
> +valid_term (const char *term)
I guess the return value can be bool.
> +{
> +  size_t len = strlen (term);
> +
> +  if (len == 0 || len > 16)
> +    return 0;
> +
> +  while (len > 0) {
> +    char c = *term++;
> +    len--;
> +    if (!c_isalnum (c) && c != '-' && c !=
'_')
> +      return 0;
> +  }
The loop is fine already, maybe the need to use len can be dropped:

  for (; *term; ++term) {
    char c = *term;
    if (!c_isalnum (c) && c != '-' && c != '_')
      return 0;
  }

Thanks,
-- 
Pino Toscano