Xen Security Advisory 137 (CVE-2015-3259) - xl command line config handling stack overflow

From 593fe52faa1b85567a7ec20c69d8cfbc7368ae5b Mon Sep 17 00:00:00 2001
From: Ian Jackson <[email protected]>
Date: Mon, 15 Jun 2015 14:50:42 +0100
Subject: [PATCH] xl: Sane handling of extra config file arguments

Various xl sub-commands take additional parameters containing = as
additional config fragments.

The handling of these config fragments has a number of bugs:

 1. Use of a static 1024-byte buffer.  (If truncation would occur,
    with semi-trusted input, a security risk arises due to quotes
    being lost.)

 2. Mishandling of the return value from snprintf, so that if
    truncation occurs, the to-write pointer is updated with the
    wanted-to-write length, resulting in stack corruption.  (This is
    XSA-137.)

 3. Clone-and-hack of the code for constructing the appended
    config file.

These are fixed here, by introducing a new function
`string_realloc_append' and using it everywhere.  The `extra_info'
buffers are replaced by pointers, which start off NULL and are
explicitly freed on all return paths.

The separate variable which will become dom_info.extra_config is
abolished (which involves moving the clearing of dom_info).

Additional bugs I observe, not fixed here:

 4. The functions which now call string_realloc_append use ad-hoc
    error returns, with multiple calls to `return'.  This currently
    necessitates multiple new calls to `free'.

 5. Many of the paths in xl call exit(-rc) where rc is a libxl status
    code.  This is a ridiculous exit status `convention'.

 6. The loops for handling extra config data are clone-and-hacks.

 7. Once the extra config buffer is accumulated, it must be combined
    with the appropriate main config file.  The code to do this
    combining is clone-and-hacked too.

Signed-off-by: Ian Jackson <[email protected]>
Tested-by: Ian Jackson <[email protected]>
Acked-by: Ian Campbell <ian,[email protected]>
---
v2: Use SSIZE_MAX, not INT_MAX.
    Check *accumulate for NULL, not accumulate.
    Move memset of dom_info.
---
 tools/libxl/xl_cmdimpl.c |   64 +++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c858068..c01a851 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -151,7 +151,7 @@ struct domain_create {
     int console_autoconnect;
     int checkpointed_stream;
     const char *config_file;
-    const char *extra_config; /* extra config string */
+    char *extra_config; /* extra config string */
     const char *restore_file;
     int migrate_fd; /* -1 means none */
     char **migration_domname_r; /* from malloc */
@@ -4805,11 +4805,25 @@ int main_vm_list(int argc, char **argv)
     return 0;
 }
 
+static void string_realloc_append(char **accumulate, const char *more)
+{
+    /* Appends more to accumulate.  Accumulate is either NULL, or
+     * points (always) to a malloc'd nul-terminated string. */
+
+    size_t oldlen = *accumulate ? strlen(*accumulate) : 0;
+    size_t morelen = strlen(more) + 1/*nul*/;
+    if (oldlen > SSIZE_MAX || morelen > SSIZE_MAX - oldlen) {
+        fprintf(stderr,"Additional config data far too large\n");
+        exit(-ERROR_FAIL);
+    }
+
+    *accumulate = xrealloc(*accumulate, oldlen + morelen);
+    memcpy(*accumulate + oldlen, more, morelen);
+}
+
 int main_create(int argc, char **argv)
 {
     const char *filename = NULL;
-    char *p;
-    char extra_config[1024];
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
         quiet = 0, monitor = 1, vnc = 0, vncautopass = 0;
@@ -4824,6 +4838,8 @@ int main_create(int argc, char **argv)
         {0, 0, 0, 0}
     };
 
+    dom_info.extra_config = NULL;
+
     if (argv[1] && argv[1][0] != '-' && !strchr(argv[1], '=')) {
         filename = argv[1];
         argc--; argv++;
@@ -4863,20 +4879,21 @@ int main_create(int argc, char **argv)
         break;
     }
 
-    extra_config[0] = '\0';
-    for (p = extra_config; optind < argc; optind++) {
+    memset(&dom_info, 0, sizeof(dom_info));
+
+    for (; optind < argc; optind++) {
         if (strchr(argv[optind], '=') != NULL) {
-            p += snprintf(p, sizeof(extra_config) - (p - extra_config),
-                "%s\n", argv[optind]);
+            string_realloc_append(&dom_info.extra_config, argv[optind]);
+            string_realloc_append(&dom_info.extra_config, "\n");
         } else if (!filename) {
             filename = argv[optind];
         } else {
             help("create");
+            free(dom_info.extra_config);
             return 2;
         }
     }
 
-    memset(&dom_info, 0, sizeof(dom_info));
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
@@ -4884,16 +4901,18 @@ int main_create(int argc, char **argv)
     dom_info.dryrun = dryrun_only;
     dom_info.quiet = quiet;
     dom_info.config_file = filename;
-    dom_info.extra_config = extra_config;
     dom_info.migrate_fd = -1;
     dom_info.vnc = vnc;
     dom_info.vncautopass = vncautopass;
     dom_info.console_autoconnect = console_autoconnect;
 
     rc = create_domain(&dom_info);
-    if (rc < 0)
+    if (rc < 0) {
+        free(dom_info.extra_config);
         return -rc;
+    }
 
+    free(dom_info.extra_config);
     return 0;
 }
 
@@ -4901,8 +4920,7 @@ int main_config_update(int argc, char **argv)
 {
     uint32_t domid;
     const char *filename = NULL;
-    char *p;
-    char extra_config[1024];
+    char *extra_config = NULL;
     void *config_data = 0;
     int config_len = 0;
     libxl_domain_config d_config;
@@ -4940,15 +4958,15 @@ int main_config_update(int argc, char **argv)
         break;
     }
 
-    extra_config[0] = '\0';
-    for (p = extra_config; optind < argc; optind++) {
+    for (; optind < argc; optind++) {
         if (strchr(argv[optind], '=') != NULL) {
-            p += snprintf(p, sizeof(extra_config) - (p - extra_config),
-                "%s\n", argv[optind]);
+            string_realloc_append(&extra_config, argv[optind]);
+            string_realloc_append(&extra_config, "\n");
         } else if (!filename) {
             filename = argv[optind];
         } else {
             help("create");
+            free(extra_config);
             return 2;
         }
     }
@@ -4957,7 +4975,8 @@ int main_config_update(int argc, char **argv)
         rc = libxl_read_file_contents(ctx, filename,
                                       &config_data, &config_len);
         if (rc) { fprintf(stderr, "Failed to read config file: %s: %s\n",
-                           filename, strerror(errno)); return ERROR_FAIL; }
+                           filename, strerror(errno));
+                  free(extra_config); return ERROR_FAIL; }
         if (strlen(extra_config)) {
             if (config_len > INT_MAX - (strlen(extra_config) + 2 + 1)) {
                 fprintf(stderr, "Failed to attach extra configration\n");
@@ -4998,7 +5017,7 @@ int main_config_update(int argc, char **argv)
     libxl_domain_config_dispose(&d_config);
 
     free(config_data);
-
+    free(extra_config);
     return 0;
 }
 
@@ -7255,7 +7274,7 @@ int main_cpupoolcreate(int argc, char **argv)
 {
     const char *filename = NULL, *config_src=NULL;
     const char *p;
-    char extra_config[1024];
+    char *extra_config = NULL;
     int opt;
     static struct option opts[] = {
         {"defconfig", 1, 0, 'f'},
@@ -7289,13 +7308,10 @@ int main_cpupoolcreate(int argc, char **argv)
         break;
     }
 
-    memset(extra_config, 0, sizeof(extra_config));
     while (optind < argc) {
         if ((p = strchr(argv[optind], '='))) {
-            if (strlen(extra_config) + 1 + strlen(argv[optind]) < sizeof(extra_config)) {
-                strcat(extra_config, "\n");
-                strcat(extra_config, argv[optind]);
-            }
+            string_realloc_append(&extra_config, "\n");
+            string_realloc_append(&extra_config, argv[optind]);
         } else if (!filename) {
             filename = argv[optind];
         } else {
-- 
1.7.10.4