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

[Markus Armbruster]

Wolfgang Bumiller <address@hidden> writes:

> On Sun, Jan 10, 2016 at 10:56:55AM +0300, Michael Tokarev wrote:
>> So, what's the status of this issue now?
>> (it is CVE-2015-8619 btw, maybe worth to mention this in the commit message)
>
> Seems we concluded it's best to keep keyname_len around and simply check
> it against the sizeof(keyname_buf).
>
> Here's a full new version as I haven't seen one yet. (With an adapted
> commit message and the CVE id added.)
>
> I did not include the proposed change to the pstrcpy() size parameter
> as it seemed more like a coding-style change and because the code also
> uses
>   pstrcpy(keyname_buf, sizeof(keyname_buf), "less")
> instead of a memcpy() (after all, the buffer size is known and the
> contents are constant in that line).
>
> Patch:
>
> ===
>>From 8da4a3bf8fb076314f986a0d58cb94f5458e3659 Mon Sep 17 00:00:00 2001
> From: Wolfgang Bumiller <address@hidden>
> Date: Mon, 11 Jan 2016 08:21:25 +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

Well, it technically doesn't terminate, 

> write issue, if 'keyname_len' was to fall outside of
> 'keyname_buf' array.
>
> Now checking the length against the buffer size before using
> it.
>
> Reported-by: Ling Liu <address@hidden>
> Signed-off-by: Wolfgang Bumiller <address@hidden>
> ---
>  hmp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hmp.c b/hmp.c
> index c2b2c16..0c7a04c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1749,6 +1749,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>      while (1) {
>          separator = strchr(keys, '-');
>          keyname_len = separator ? separator - keys : strlen(keys);
> +        if (keyname_len >= sizeof(keyname_buf))
> +            goto err_out;

Style nit: missing braces.

Works because the longest member of QKeyCode_lookup[] is 13 characters,
and therefore truncation implies "no match".  But it's not obvious.
No worse than before, but "you touch it, you own it".

Options:

* Add a comments

    char keyname_buf[16];       /* can hold any QKeyCode */

  and

        if (keyname_len >= sizeof(keyname_buf)) {
            /* too long to match any QKeyCode */
            goto err_out;
        }

* Make index_from_key() take pointer into string and size instead of a
  string.

* Get rid of the static buffer and malloc the sucker already.

>          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");
               keyname_len = 4;
           }
           keyname_buf[keyname_len] = 0;

Why keep this assignment?

> @@ -1800,7 +1802,7 @@ out:
>      return;
>  
>  err_out:
> -    monitor_printf(mon, "invalid parameter: %s\n", keyname_buf);
> +    monitor_printf(mon, "invalid parameter: %s\n", keys);
>      goto out;
>  }

Before your patch, the message shows the (possibly truncated) offending
key name.  With your patch, it shows the tail of the argument starting
with the offending key name.  Why is that an improvement?

Could use %.*s to print exactly the offending key name.

What's wrong with Prasad's simple fix?