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

[P J P]

+-- On Mon, 11 Jan 2016, Wolfgang Bumiller wrote --+
| 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.)

  Sorry, i thought you were sending it.

| ===
| >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
| 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;
|          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;
|  }

  It looks good.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F