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

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

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 <w.bumiller@proxmox.com>
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
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 <liuling-it@360.cn>
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 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;
         pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
 
         /* Be compatible with old interface, convert user inputted "<" */
@@ -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;
 }
 
-- 
2.1.4