hmp: avoid redundant null termination of buffer

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.

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.