Wolfgang Bumiller <w.bumil...@proxmox.com> writes: > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote: >> Wolfgang Bumiller <w.bumil...@proxmox.com> writes: >> >> >> On January 12, 2016 at 5:00 PM Markus Armbruster <arm...@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.bumil...@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...@360.cn> > Signed-off-by: Wolfgang Bumiller <w.bumil...@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.