Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer

[Markus Armbruster]

Wolfgang Bumiller <address@hidden> writes:

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