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

[Wolfgang Bumiller]

On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote:
> Wolfgang Bumiller <address@hidden> writes:
> 
> >> On January 12, 2016 at 5:00 PM Markus Armbruster <address@hidden> 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 <address@hidden>
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 <address@hidden>
Signed-off-by: Wolfgang Bumiller <address@hidden>
---
 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);
-        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) {
+            keys = "less";
             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 != '-') {
                 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;
         }
     }
-- 
2.1.4