Message ID | 20160113080958.GA18934@olga |
---|---|
State | New |
Headers | show |
Wolfgang Bumiller <w.bumiller@proxmox.com> writes: > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote: >> Wolfgang Bumiller <w.bumiller@proxmox.com> writes: >> >> >> On January 12, 2016 at 5:00 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Will you prepare a revised patch? >> > >> > Can do that tomorrow, but which option is the preferred one? If "%.*s" works >> > everywhere then changing index_from_key() and using "%.*s" would be the most >> > optimal I think. >> > >> > I don't want to bounce 5 more versions back and forth of something that's >> > supposed to be rather trivial. >> >> Understandable. >> >> If your patch works and is simple, I won't ask you to redo it using >> another method just because I might like that better. > > Less simple (or at least longer) but gets rid of the static buffer, > shows the exact keyname in the error message and gets rid of the copying > of the word "less", too, by adding a length to index_from_key() as per > your suggestion. Seemed like the cleanest option. > > Note that at the end of the loop (not visible in this patch's context > lines) 'keys' is reassigned to separator+1 or the loop ends if no > separator was there, which makes the `keys = "less"` assignment valid. > Though maybe adding an extra `const char *keyname` that becomes > `keyname = keys` at the beginning of the loop might be better? Not sure > which style you prefer, I can resend if you like. > > === >>From 136dd5ac96fc21654a31aff7fa88b86570c8fc72 Mon Sep 17 00:00:00 2001 > From: Wolfgang Bumiller <w.bumiller@proxmox.com> > Date: Wed, 13 Jan 2016 08:46:31 +0100 > Subject: [PATCH] hmp: fix sendkey out of bounds write (CVE-2015-8619) > > When processing 'sendkey' command, hmp_sendkey routine null > terminates the 'keyname_buf' array. This results in an OOB > write issue, if 'keyname_len' was to fall outside of > 'keyname_buf' array. > > Since the keyname's length is known the keyname_buf can be > removed altogether by adding a length parameter to > index_from_key() and using it for the error output as well. > > Reported-by: Ling Liu <liuling-it@360.cn> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> > --- > hmp.c | 17 +++++++---------- > include/ui/console.h | 2 +- > ui/input-legacy.c | 5 +++-- > 3 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/hmp.c b/hmp.c > index c2b2c16..066ccf8 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1742,21 +1742,18 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) > int has_hold_time = qdict_haskey(qdict, "hold-time"); > int hold_time = qdict_get_try_int(qdict, "hold-time", -1); > Error *err = NULL; > - char keyname_buf[16]; > char *separator; > int keyname_len; > > while (1) { > separator = strchr(keys, '-'); > keyname_len = separator ? separator - keys : strlen(keys); Preexisting: I wonder why the compiler doesn't warn here: separator - keys is ptrdiff_t, strlen() is size_t, and the left hand side is int. > - pstrcpy(keyname_buf, sizeof(keyname_buf), keys); > > /* Be compatible with old interface, convert user inputted "<" */ > - if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) { > - pstrcpy(keyname_buf, sizeof(keyname_buf), "less"); > + if (!strncmp(keys, "<", 1) && keyname_len == 1) { This strncmp() is a rather roundabout way to say keys[0] == '<'. I guess I'd dumb it down while touching it. Your choice. > + keys = "less"; Works because we're resetting keys to point into the argument string at the end of the loop. > keyname_len = 4; > } > - keyname_buf[keyname_len] = 0; > > keylist = g_malloc0(sizeof(*keylist)); > keylist->value = g_malloc0(sizeof(*keylist->value)); > @@ -1769,16 +1766,16 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) > } > tmp = keylist; > > - if (strstart(keyname_buf, "0x", NULL)) { > + if (strstart(keys, "0x", NULL)) { > char *endp; > - int value = strtoul(keyname_buf, &endp, 0); > - if (*endp != '\0') { > + int value = strtoul(keys, &endp, 0); > + if (*endp != '\0' && *endp != '-') { strtoul() will not parse beyond keyname_len, because it'll only accept hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop. I guess I'd throw in assert(endp <= keys + keyname_len), and test endp != keys + keyname_len. What do you think? > goto err_out; > } > keylist->value->type = KEY_VALUE_KIND_NUMBER; > keylist->value->u.number = value; > } else { > - int idx = index_from_key(keyname_buf); > + int idx = index_from_key(keys, keyname_len); > if (idx == Q_KEY_CODE__MAX) { > goto err_out; > } > @@ -1800,7 +1797,7 @@ out: > return; > > err_out: > - monitor_printf(mon, "invalid parameter: %s\n", keyname_buf); > + monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys); > goto out; > } > > diff --git a/include/ui/console.h b/include/ui/console.h > index adac36d..116bc2b 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -448,7 +448,7 @@ static inline int vnc_display_pw_expire(const char *id, time_t expires) > void curses_display_init(DisplayState *ds, int full_screen); > > /* input.c */ > -int index_from_key(const char *key); > +int index_from_key(const char *key, size_t key_length); > > /* gtk.c */ > void early_gtk_display_init(int opengl); > diff --git a/ui/input-legacy.c b/ui/input-legacy.c > index 35dfc27..3454055 100644 > --- a/ui/input-legacy.c > +++ b/ui/input-legacy.c > @@ -57,12 +57,13 @@ struct QEMUPutLEDEntry { > static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = > QTAILQ_HEAD_INITIALIZER(led_handlers); > > -int index_from_key(const char *key) > +int index_from_key(const char *key, size_t key_length) > { > int i; > > for (i = 0; QKeyCode_lookup[i] != NULL; i++) { > - if (!strcmp(key, QKeyCode_lookup[i])) { > + if (!strncmp(key, QKeyCode_lookup[i], key_length) && > + !QKeyCode_lookup[i][key_length]) { > break; > } > } Could !strncmp(key, QKeyCode_lookup[i], key_length + 1), but that's probably overly clever. Overall, this is more subtle than a simple g_strndup() solution. But it doesn't quite reach the threshold for me asking you to redo it differently. I can work in the two changes I proposed on commit, if you like them: dumb down the test for "<", and add the assertion.
On Mon, Jan 18, 2016 at 02:02:07PM +0100, Markus Armbruster wrote: > Wolfgang Bumiller <w.bumiller@proxmox.com> writes: > > > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote: > >> Wolfgang Bumiller <w.bumiller@proxmox.com> writes: > >> > > while (1) { > > separator = strchr(keys, '-'); > > keyname_len = separator ? separator - keys : strlen(keys); > > Preexisting: I wonder why the compiler doesn't warn here: separator - > keys is ptrdiff_t, strlen() is size_t, and the left hand side is int. I noticed and agree it should warn. We know that separator > keys (ie positive), but we also use keyname_len as a '.*' parameter to printf() which expects it to be an 'int', so when changing it to size_t we need to cast it there. Would have to pass a pretty long key name for this to be an issue... can this happen over any sane interface that doesn't already give you the power to just 'kill -9 $qemu'? > > - pstrcpy(keyname_buf, sizeof(keyname_buf), keys); > > > > /* Be compatible with old interface, convert user inputted "<" */ > > - if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) { > > - pstrcpy(keyname_buf, sizeof(keyname_buf), "less"); > > + if (!strncmp(keys, "<", 1) && keyname_len == 1) { > > This strncmp() is a rather roundabout way to say keys[0] == '<'. I > guess I'd dumb it down while touching it. Your choice. Yes, but with the previous pstrcpy() of "less" etc. I thought this was a style thing (and the compiler optimizes it out anyway last time I checked). > > + keys = "less"; > > Works because we're resetting keys to point into the argument string at > the end of the loop. > > > keyname_len = 4; > > } > > - keyname_buf[keyname_len] = 0; > > > > keylist = g_malloc0(sizeof(*keylist)); > > keylist->value = g_malloc0(sizeof(*keylist->value)); > > @@ -1769,16 +1766,16 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) > > } > > tmp = keylist; > > > > - if (strstart(keyname_buf, "0x", NULL)) { > > + if (strstart(keys, "0x", NULL)) { > > char *endp; > > - int value = strtoul(keyname_buf, &endp, 0); > > - if (*endp != '\0') { > > + int value = strtoul(keys, &endp, 0); > > + if (*endp != '\0' && *endp != '-') { > > strtoul() will not parse beyond keyname_len, because it'll only accept > hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop. > > I guess I'd throw in assert(endp <= keys + keyname_len), and test > endp != keys + keyname_len. What do you think? Makes sense, but I doubt it'll ever be hit with sane strtoul() implementations, but an assetion can't be harmful here either :-) > > goto err_out; > > } > > keylist->value->type = KEY_VALUE_KIND_NUMBER; > > keylist->value->u.number = value; > > } else { > > - int idx = index_from_key(keyname_buf); > > + int idx = index_from_key(keys, keyname_len); > > if (idx == Q_KEY_CODE__MAX) { > > goto err_out; > > } > > @@ -1800,7 +1797,7 @@ out: > > return; > > > > err_out: > > - monitor_printf(mon, "invalid parameter: %s\n", keyname_buf); > > + monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys); > > goto out; > > } > > > > diff --git a/include/ui/console.h b/include/ui/console.h > > index adac36d..116bc2b 100644 > > --- a/include/ui/console.h > > +++ b/include/ui/console.h > > @@ -448,7 +448,7 @@ static inline int vnc_display_pw_expire(const char *id, time_t expires) > > void curses_display_init(DisplayState *ds, int full_screen); > > > > /* input.c */ > > -int index_from_key(const char *key); > > +int index_from_key(const char *key, size_t key_length); > > > > /* gtk.c */ > > void early_gtk_display_init(int opengl); > > diff --git a/ui/input-legacy.c b/ui/input-legacy.c > > index 35dfc27..3454055 100644 > > --- a/ui/input-legacy.c > > +++ b/ui/input-legacy.c > > @@ -57,12 +57,13 @@ struct QEMUPutLEDEntry { > > static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = > > QTAILQ_HEAD_INITIALIZER(led_handlers); > > > > -int index_from_key(const char *key) > > +int index_from_key(const char *key, size_t key_length) > > { > > int i; > > > > for (i = 0; QKeyCode_lookup[i] != NULL; i++) { > > - if (!strcmp(key, QKeyCode_lookup[i])) { > > + if (!strncmp(key, QKeyCode_lookup[i], key_length) && > > + !QKeyCode_lookup[i][key_length]) { > > break; > > } > > } > > Could !strncmp(key, QKeyCode_lookup[i], key_length + 1), but that's > probably overly clever. That's assuming the key name ends with a \0, which is not the case coming from a combined key combination where key points to "ctrl-alt-f1" and should find "ctrl". > Overall, this is more subtle than a simple g_strndup() solution. But it > doesn't quite reach the threshold for me asking you to redo it > differently. > > I can work in the two changes I proposed on commit, if you like them: > dumb down the test for "<", and add the assertion. Sounds good.
=== From 136dd5ac96fc21654a31aff7fa88b86570c8fc72 Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller <w.bumiller@proxmox.com> Date: Wed, 13 Jan 2016 08:46:31 +0100 Subject: [PATCH] hmp: fix sendkey out of bounds write (CVE-2015-8619) When processing 'sendkey' command, hmp_sendkey routine null terminates the 'keyname_buf' array. This results in an OOB write issue, if 'keyname_len' was to fall outside of 'keyname_buf' array. Since the keyname's length is known the keyname_buf can be removed altogether by adding a length parameter to index_from_key() and using it for the error output as well. Reported-by: Ling Liu <liuling-it@360.cn> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> --- hmp.c | 17 +++++++---------- include/ui/console.h | 2 +- ui/input-legacy.c | 5 +++-- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/hmp.c b/hmp.c index c2b2c16..066ccf8 100644 --- a/hmp.c +++ b/hmp.c @@ -1742,21 +1742,18 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) int has_hold_time = qdict_haskey(qdict, "hold-time"); int hold_time = qdict_get_try_int(qdict, "hold-time", -1); Error *err = NULL; - char keyname_buf[16]; char *separator; int keyname_len; while (1) { separator = strchr(keys, '-'); keyname_len = separator ? separator - keys : strlen(keys); - pstrcpy(keyname_buf, sizeof(keyname_buf), keys); /* Be compatible with old interface, convert user inputted "<" */ - if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) { - pstrcpy(keyname_buf, sizeof(keyname_buf), "less"); + if (!strncmp(keys, "<", 1) && keyname_len == 1) { + keys = "less"; keyname_len = 4; } - keyname_buf[keyname_len] = 0; keylist = g_malloc0(sizeof(*keylist)); keylist->value = g_malloc0(sizeof(*keylist->value)); @@ -1769,16 +1766,16 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict) } tmp = keylist; - if (strstart(keyname_buf, "0x", NULL)) { + if (strstart(keys, "0x", NULL)) { char *endp; - int value = strtoul(keyname_buf, &endp, 0); - if (*endp != '\0') { + int value = strtoul(keys, &endp, 0); + if (*endp != '\0' && *endp != '-') { goto err_out; } keylist->value->type = KEY_VALUE_KIND_NUMBER; keylist->value->u.number = value; } else { - int idx = index_from_key(keyname_buf); + int idx = index_from_key(keys, keyname_len); if (idx == Q_KEY_CODE__MAX) { goto err_out; } @@ -1800,7 +1797,7 @@ out: return; err_out: - monitor_printf(mon, "invalid parameter: %s\n", keyname_buf); + monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys); goto out; } diff --git a/include/ui/console.h b/include/ui/console.h index adac36d..116bc2b 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -448,7 +448,7 @@ static inline int vnc_display_pw_expire(const char *id, time_t expires) void curses_display_init(DisplayState *ds, int full_screen); /* input.c */ -int index_from_key(const char *key); +int index_from_key(const char *key, size_t key_length); /* gtk.c */ void early_gtk_display_init(int opengl); diff --git a/ui/input-legacy.c b/ui/input-legacy.c index 35dfc27..3454055 100644 --- a/ui/input-legacy.c +++ b/ui/input-legacy.c @@ -57,12 +57,13 @@ struct QEMUPutLEDEntry { static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers); -int index_from_key(const char *key) +int index_from_key(const char *key, size_t key_length) { int i; for (i = 0; QKeyCode_lookup[i] != NULL; i++) { - if (!strcmp(key, QKeyCode_lookup[i])) { + if (!strncmp(key, QKeyCode_lookup[i], key_length) && + !QKeyCode_lookup[i][key_length]) { break; } }