|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-changelog] [xen stable-4.2] xl: Sane handling of extra config file arguments
commit b20c28064c54d345f366528a0f452ad14911e146
Author: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
AuthorDate: Mon Jun 15 14:50:42 2015 +0100
Commit: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CommitDate: Wed Jul 29 16:40:17 2015 +0100
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 <Ian.Jackson@xxxxxxxxxxxxx>
Tested-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Acked-by: Ian Campbell <ian,campbell@xxxxxxxxxx>
(cherry picked from commit dd84604f35bd3855c57146eb8fe53924c10d3963)
(cherry picked from commit 6040b3aeb32b4bce2d9958ecbcbd020c46c35d61)
(cherry picked from commit 214fd40a20fa5988b4ea021c2d06e8aca8dda184)
(cherry picked from commit d7ab3a1c1cc245dc0683bb937467c27141754053)
---
tools/libxl/xl_cmdimpl.c | 64 ++++++++++++++++++++++++++++-----------------
1 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5eadb16..666b8cd 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -135,7 +135,7 @@ struct domain_create {
int vncautopass;
int console_autoconnect;
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 */
@@ -3815,11 +3815,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;
@@ -3835,6 +3849,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++;
@@ -3886,20 +3902,21 @@ int main_create(int argc, char **argv)
}
}
- 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;
@@ -3907,24 +3924,25 @@ 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;
}
int main_config_update(int argc, char **argv)
{
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;
@@ -3972,15 +3990,15 @@ int main_config_update(int argc, char **argv)
}
}
- 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;
}
}
@@ -3989,7 +4007,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");
@@ -4030,7 +4049,7 @@ int main_config_update(int argc, char **argv)
libxl_domain_config_dispose(&d_config);
free(config_data);
-
+ free(extra_config);
return 0;
}
@@ -6000,7 +6019,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;
int option_index = 0;
static struct option long_options[] = {
@@ -6047,13 +6066,10 @@ int main_cpupoolcreate(int argc, char **argv)
}
}
- 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 {
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.2
_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |