From patchwork Wed Jan 13 08:09:58 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wolfgang Bumiller X-Patchwork-Id: 566851 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7ED2A140556 for ; Wed, 13 Jan 2016 19:11:08 +1100 (AEDT) Received: from localhost ([::1]:35805 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJGWI-0000gV-8j for incoming@patchwork.ozlabs.org; Wed, 13 Jan 2016 03:11:06 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJGVo-0008Hw-K5 for qemu-devel@nongnu.org; Wed, 13 Jan 2016 03:10:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJGVl-0006zk-TP for qemu-devel@nongnu.org; Wed, 13 Jan 2016 03:10:36 -0500 Received: from proxmox.maurer-it.com ([94.136.31.133]:35284) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJGVl-0006zX-IJ for qemu-devel@nongnu.org; Wed, 13 Jan 2016 03:10:33 -0500 Received: from proxmox.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox.maurer-it.com (Proxmox) with ESMTP id 7F9A2AD0904; Wed, 13 Jan 2016 09:10:02 +0100 (CET) Date: Wed, 13 Jan 2016 09:09:58 +0100 From: Wolfgang Bumiller To: Markus Armbruster Message-ID: <20160113080958.GA18934@olga> References: <1907860725.4.388b58e8-5b06-4844-be0c-df2778eb46fb.open-xchange@webmail.proxmox.com> <56920EC7.6090109@msgid.tls.msk.ru> <20160111075914.GA28466@olga> <87wprfqj8t.fsf@blackfin.pond.sub.org> <1786291473.38.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> <87h9iirdms.fsf@blackfin.pond.sub.org> <1388889179.80.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> <877fjepwo9.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <877fjepwo9.fsf@blackfin.pond.sub.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 94.136.31.133 Cc: Ling Liu , Michael Tokarev , qemu-devel@nongnu.org, P J P Subject: Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote: > Wolfgang Bumiller writes: > > >> On January 12, 2016 at 5:00 PM Markus Armbruster 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 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 Signed-off-by: Wolfgang Bumiller --- 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; } }