From 593fe52faa1b85567a7ec20c69d8cfbc7368ae5b Mon Sep 17 00:00:00 2001 From: Ian Jackson 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 Tested-by: Ian Jackson Acked-by: Ian Campbell --- 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