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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Dec 17, 2015 at 06:10:59PM +0530, P J P wrote:
>   Hello,
> 
> An OOB write issue was reported by Mr Ling Liu, CC'd here. It occurs while
> processing the 'sendkey' command, if the command argument was longer than
> the 'keyname_buf[16]' buffer.
> 
> ===
> From b0363f4c0e91671064dd7ffece8a6923c8dcaf20 Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit <pjp@fedoraproject.org>
> Date: Thu, 17 Dec 2015 17:47:15 +0530
> Subject: [PATCH] hmp: avoid redundant null termination of buffer
> 
> 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.
> Removed the redundant null termination, as pstrcpy routine already
> null terminates the target buffer.
> 
> Reported-by: Ling Liu <liuling-it@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hmp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 2140605..e530c9c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1746,9 +1746,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>          /* 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 is a char[16] so 4 will not overflow it.

>          }
> -        keyname_buf[keyname_len] = 0;

This last write is also used to separate combined keys, so removing
this write breaks commands such as `sendkeys ctrl-f1`.
Better add a -1 to the sizeof()s?

Come to think of it, when is this really an OOB write?
Given where keyname_len comes from:

| separator = strchr(keys, '-');
| keyname_len = separator ? separator - keys : strlen(keys);

If separator is found the assignment replaces the '-', which is valid,
and if not then it'll point to the '\0' byte and simply writes to that,
which is still inside the string. What might be useful to do is see if
(separator != NULL && separator - keys >= sizeof(keyname_buf)) you can
error out since it means there's a single key-name longer than
keyname_buf used as parameter.

The buffer used here is static, the pointer that is advanced through the
key names is the 'keys' variable which is only read from.

| const char *keys = qdict_get_str(qdict, "keys");

and

| keys = separator + 1;

So I'm not sure this is really a bug here?
And if it is, this patch still breaks combined 'sendkey' commands.

>          keylist = g_malloc0(sizeof(*keylist));
>          keylist->value = g_malloc0(sizeof(*keylist->value));
> -- 
> 2.4.3
> ===
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
> 
>