Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Michael Tokarev writes:
> Ping?
Sorry for the delay, I've been focusing on the QAPI queue to the
exclusion of pretty much everything else.
> 26.01.2016 12:36, Michael Tokarev wrote:
>> 18.01.2016 17:23, Markus Armbruster wrote:
>> [...]
>>> Applied to my monitor-next with these tweaks:
>>>
>>> diff --git a/hmp.c b/hmp.c
>>> index 8be03df..9c571f5 100644
>>> --- a/hmp.c
>>> +++ b/hmp.c
>>> @@ -1739,7 +1739,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>> keyname_len = separator ? separator - keys : strlen(keys);
>>>
>>> /* Be compatible with old interface, convert user inputted "<" */
>>> -if (!strncmp(keys, "<", 1) && keyname_len == 1) {
>>> +if (keys[0] == '<' && keyname_len == 1) {
>>> keys = "less";
>>> keyname_len = 4;
>>> }
>>> @@ -1758,7 +1758,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>> if (strstart(keys, "0x", NULL)) {
>>> char *endp;
>>> int value = strtoul(keys, &endp, 0);
>>> -if (*endp != '\0' && *endp != '-') {
>>> +assert(endp <= keys + keyname_len);
>>> +if (endp != keys + keyname_len) {
>>> goto err_out;
>>> }
>>> keylist->value->type = KEY_VALUE_KIND_NUMBER;
>>
>> Marcus, where's your monitor-next branch? Repository at
>> git://repo.or.cz/qemu/armbru.git , monitor-next branch does
>> not contain this change, last commit to hmp.c dated Sep-8.
I forgot to push. Should be there now, but needs a rebase to current
master.
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Ping?
26.01.2016 12:36, Michael Tokarev wrote:
> 18.01.2016 17:23, Markus Armbruster wrote:
> [...]
>> Applied to my monitor-next with these tweaks:
>>
>> diff --git a/hmp.c b/hmp.c
>> index 8be03df..9c571f5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1739,7 +1739,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>> keyname_len = separator ? separator - keys : strlen(keys);
>>
>> /* Be compatible with old interface, convert user inputted "<" */
>> -if (!strncmp(keys, "<", 1) && keyname_len == 1) {
>> +if (keys[0] == '<' && keyname_len == 1) {
>> keys = "less";
>> keyname_len = 4;
>> }
>> @@ -1758,7 +1758,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>> if (strstart(keys, "0x", NULL)) {
>> char *endp;
>> int value = strtoul(keys, &endp, 0);
>> -if (*endp != '\0' && *endp != '-') {
>> +assert(endp <= keys + keyname_len);
>> +if (endp != keys + keyname_len) {
>> goto err_out;
>> }
>> keylist->value->type = KEY_VALUE_KIND_NUMBER;
>
> Marcus, where's your monitor-next branch? Repository at
> git://repo.or.cz/qemu/armbru.git , monitor-next branch does
> not contain this change, last commit to hmp.c dated Sep-8.
>
> Thanks,
>
> /mjt
>
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
18.01.2016 17:23, Markus Armbruster wrote:
[...]
> Applied to my monitor-next with these tweaks:
>
> diff --git a/hmp.c b/hmp.c
> index 8be03df..9c571f5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1739,7 +1739,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
> keyname_len = separator ? separator - keys : strlen(keys);
>
> /* Be compatible with old interface, convert user inputted "<" */
> -if (!strncmp(keys, "<", 1) && keyname_len == 1) {
> +if (keys[0] == '<' && keyname_len == 1) {
> keys = "less";
> keyname_len = 4;
> }
> @@ -1758,7 +1758,8 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
> if (strstart(keys, "0x", NULL)) {
> char *endp;
> int value = strtoul(keys, &endp, 0);
> -if (*endp != '\0' && *endp != '-') {
> +assert(endp <= keys + keyname_len);
> +if (endp != keys + keyname_len) {
> goto err_out;
> }
> keylist->value->type = KEY_VALUE_KIND_NUMBER;
Marcus, where's your monitor-next branch? Repository at
git://repo.or.cz/qemu/armbru.git , monitor-next branch does
not contain this change, last commit to hmp.c dated Sep-8.
Thanks,
/mjt
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Wolfgang Bumiller writes:
> On Mon, Jan 18, 2016 at 02:02:07PM +0100, Markus Armbruster wrote:
>> Wolfgang Bumiller writes:
>>
>> > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote:
>> >> Wolfgang Bumiller writes:
>> >>
>> > while (1) {
>> > separator = strchr(keys, '-');
>> > keyname_len = separator ? separator - keys : strlen(keys);
>>
>> Preexisting: I wonder why the compiler doesn't warn here: separator -
>> keys is ptrdiff_t, strlen() is size_t, and the left hand side is int.
>
> I noticed and agree it should warn. We know that separator > keys (ie
> positive), but we also use keyname_len as a '.*' parameter to printf()
> which expects it to be an 'int', so when changing it to size_t we need
> to cast it there. Would have to pass a pretty long key name for this to
> be an issue... can this happen over any sane interface that doesn't
> already give you the power to just 'kill -9 $qemu'?
Merely unclean, not actually broken in a practical sense.
>> > -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) {
>>
>> This strncmp() is a rather roundabout way to say keys[0] == '<'. I
>> guess I'd dumb it down while touching it. Your choice.
>
> Yes, but with the previous pstrcpy() of "less" etc. I thought this was a
> style thing (and the compiler optimizes it out anyway last time I
> checked).
>
>> > +keys = "less";
>>
>> Works because we're resetting keys to point into the argument string at
>> the end of the loop.
>>
>> > 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 != '-') {
>>
>> strtoul() will not parse beyond keyname_len, because it'll only accept
>> hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop.
>>
>> I guess I'd throw in assert(endp <= keys + keyname_len), and test
>> endp != keys + keyname_len. What do you think?
>
> Makes sense, but I doubt it'll ever be hit with sane strtoul()
> implementations, but an assetion can't be harmful here either :-)
The assertion primarily documents we've considered strtoul() reading
beyond the bound. It might also protects us from hasty, incorrect
changes in the future, but I guess that's secondary in this case.
Preexisting: we don't check errno. Out of scope for this patch.
>> > 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
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
On Mon, Jan 18, 2016 at 02:02:07PM +0100, Markus Armbruster wrote:
> Wolfgang Bumiller writes:
>
> > On Tue, Jan 12, 2016 at 05:52:38PM +0100, Markus Armbruster wrote:
> >> Wolfgang Bumiller writes:
> >>
> > while (1) {
> > separator = strchr(keys, '-');
> > keyname_len = separator ? separator - keys : strlen(keys);
>
> Preexisting: I wonder why the compiler doesn't warn here: separator -
> keys is ptrdiff_t, strlen() is size_t, and the left hand side is int.
I noticed and agree it should warn. We know that separator > keys (ie
positive), but we also use keyname_len as a '.*' parameter to printf()
which expects it to be an 'int', so when changing it to size_t we need
to cast it there. Would have to pass a pretty long key name for this to
be an issue... can this happen over any sane interface that doesn't
already give you the power to just 'kill -9 $qemu'?
> > -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) {
>
> This strncmp() is a rather roundabout way to say keys[0] == '<'. I
> guess I'd dumb it down while touching it. Your choice.
Yes, but with the previous pstrcpy() of "less" etc. I thought this was a
style thing (and the compiler optimizes it out anyway last time I
checked).
> > +keys = "less";
>
> Works because we're resetting keys to point into the argument string at
> the end of the loop.
>
> > 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 != '-') {
>
> strtoul() will not parse beyond keyname_len, because it'll only accept
> hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop.
>
> I guess I'd throw in assert(endp <= keys + keyname_len), and test
> endp != keys + keyname_len. What do you think?
Makes sense, but I doubt it'll ever be hit with sane strtoul()
implementations, but an assetion can't be harmful here either :-)
> > 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;
> > }
> > }
>
> Could !strncmp(key, QKeyCode_lookup[i], key_length + 1), but that's
> probably overly clever.
That's assuming the key name ends with a \0, which is not the case
coming from a combined key combination where key points to "ctrl-alt-f1"
and should find "ctrl".
> Overall, this is more subtle than a simple g_strndup() solution. But it
> doesn't quite reach
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Wolfgang Bumiller writes:
> 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);
Preexisting: I wonder why the compiler doesn't warn here: separator -
keys is ptrdiff_t, strlen() is size_t, and the left hand side is int.
> -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) {
This strncmp() is a rather roundabout way to say keys[0] == '<'. I
guess I'd dumb it down while touching it. Your choice.
> +keys = "less";
Works because we're resetting keys to point into the argument string at
the end of the loop.
> 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 != '-') {
strtoul() will not parse beyond keyname_len, because it'll only accept
hex digits after 0x, thus the '-' or 0 at keyname_len will make it stop.
I guess I'd throw in assert(endp <= keys + keyname_len), and test
endp != keys + keyname_len. What do you think?
> 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);
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
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
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Wolfgang Bumiller writes:
>> On January 12, 2016 at 5:00 PM Markus Armbruster wrote:
>> separator = strchr(keys, '-');
>> keyname_len = separator ? separator - keys : strlen(keys);
>> pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
>> [...]
>> keyname_buf[keyname_len] = 0;
>>
>> This is stupid. If we already know how many characters we need, we
>> should copy exactly those:
>
> I mentioned that and didn't touch it because the same holds for the
> copying of the word "less" below and should IMO be in a separate
> cleanup patch together with that one.
Stupidest way I can think of:
>> separator = strchr(keys, '-');
>> keyname_len = separator ? separator - keys : strlen(keys);
>> if (keyname_len >= sizeof(keyname_buf))
>> goto err_out;
>> memcpy(keyname_buf, keyname_len, keys);
>> keyname_buf[keyname_len] = 0;
if (!strcmp(keyname_buf, "<")) {
strcpy(keyname_buf, "less");
}
>> But I'd simply dispense with the static buffer, and do something like
>>
>> separator = strchr(keys, '-');
>> key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys);
>> [...]
>> g_free(key);
>>
>> This is advice, not recommendation.
>
> Sure, also a good alternative.
>
>> (...)
>>
>> 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.
Your previous patch almost qualifies: it's simple, it fixes the bug, but
it regresses the error message a bit. I pointed out how to avoid the
latter, and I asked you to either add comments explaining why truncation
works (even though it's preexisting), or to redo the thing in a more
obvious way (your choice). I'm pretty sure your next patch will be fine
or very close.
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
> On January 12, 2016 at 5:00 PM Markus Armbruster wrote: > separator = strchr(keys, '-'); > keyname_len = separator ? separator - keys : strlen(keys); > pstrcpy(keyname_buf, sizeof(keyname_buf), keys); > [...] > keyname_buf[keyname_len] = 0; > > This is stupid. If we already know how many characters we need, we > should copy exactly those: I mentioned that and didn't touch it because the same holds for the copying of the word "less" below and should IMO be in a separate cleanup patch together with that one. > separator = strchr(keys, '-'); > keyname_len = separator ? separator - keys : strlen(keys); > if (keyname_len >= sizeof(keyname_buf)) > goto err_out; > memcpy(keyname_buf, keyname_len, keys); > keyname_buf[keyname_len] = 0; > > But I'd simply dispense with the static buffer, and do something like > > separator = strchr(keys, '-'); > key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys); > [...] > g_free(key); > > This is advice, not recommendation. Sure, also a good alternative. > (...) > > 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.
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Wolfgang Bumiller writes:
>> On January 12, 2016 at 9:45 AM Markus Armbruster wrote:
>>
>> Wolfgang Bumiller writes:
>>
>> > When processing 'sendkey' command, hmp_sendkey routine null
>> > terminates the 'keyname_buf' array. This results in an OOB
>>
>> Well, it technically doesn't terminate,
>
> It does for combined keys where it replaces the '-' sign (eg. ctrl-alt-f1).
>
>> > @@ -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;
>>
>> Style nit: missing braces.
>>
>> Works because the longest member of QKeyCode_lookup[] is 13 characters,
>> and therefore truncation implies "no match". But it's not obvious.
>> No worse than before, but "you touch it, you own it".
>>
>> Options:
>>
>> * Add a comments
>>
>> char keyname_buf[16]; /* can hold any QKeyCode */
>>
>> and
>>
>> if (keyname_len >= sizeof(keyname_buf)) {
>> /* too long to match any QKeyCode */
>> goto err_out;
>> }
>>
>> * Make index_from_key() take pointer into string and size instead of a
>> string.
>
> That actually looks like a good idea.
>
>> * Get rid of the static buffer and malloc the sucker already.
>>
>> > 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");
>>keyname_len = 4;
>>}
>>keyname_buf[keyname_len] = 0;
>>
>> Why keep this assignment?
>
> See above, it terminates keys when using combined keys.
You're right. We copy out up to 15 characters, then zap the '-':
separator = strchr(keys, '-');
keyname_len = separator ? separator - keys : strlen(keys);
pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
[...]
keyname_buf[keyname_len] = 0;
This is stupid. If we already know how many characters we need, we
should copy exactly those:
separator = strchr(keys, '-');
keyname_len = separator ? separator - keys : strlen(keys);
if (keyname_len >= sizeof(keyname_buf))
goto err_out;
memcpy(keyname_buf, keyname_len, keys);
keyname_buf[keyname_len] = 0;
But I'd simply dispense with the static buffer, and do something like
separator = strchr(keys, '-');
key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys);
[...]
g_free(key);
This is advice, not recommendation.
>> > @@ -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;
>> > }
>>
>> Before your patch, the message shows the (possibly truncated) offending
>> key name. With your patch, it shows the tail of the argument starting
>> with the offending key name. Why is that an improvement?
>
> I guess that's a matter of preference and the if() can be put after the
> pstrcpy() without changing the error output.
>
>> Could use %.*s to print exactly the offending key name.
>
> Does that work on all supported platforms? (I see windows-related files
> in the code base and last time I checked it doesn't do everything.)
> If so then this + changing index_from_key() as you suggested above seems
> to be the simplest option.
git-grep -F '%.*s' shows several instances, at least one of them in
Windows code.
If we strdup the key, we can simply print it, of course.
>> What's wrong with Prasad's simple fix?
>
> See above, breaks combined keys and eg. 'ctrl-alt-f1' then errors.
Will you prepare a revised patch?
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
> On January 12, 2016 at 9:45 AM Markus Armbruster wrote:
>
> Wolfgang Bumiller writes:
>
> > When processing 'sendkey' command, hmp_sendkey routine null
> > terminates the 'keyname_buf' array. This results in an OOB
>
> Well, it technically doesn't terminate,
It does for combined keys where it replaces the '-' sign (eg. ctrl-alt-f1).
> > @@ -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;
>
> Style nit: missing braces.
>
> Works because the longest member of QKeyCode_lookup[] is 13 characters,
> and therefore truncation implies "no match". But it's not obvious.
> No worse than before, but "you touch it, you own it".
>
> Options:
>
> * Add a comments
>
> char keyname_buf[16]; /* can hold any QKeyCode */
>
> and
>
> if (keyname_len >= sizeof(keyname_buf)) {
> /* too long to match any QKeyCode */
> goto err_out;
> }
>
> * Make index_from_key() take pointer into string and size instead of a
> string.
That actually looks like a good idea.
> * Get rid of the static buffer and malloc the sucker already.
>
> > 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");
>keyname_len = 4;
>}
>keyname_buf[keyname_len] = 0;
>
> Why keep this assignment?
See above, it terminates keys when using combined keys.
> > @@ -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;
> > }
>
> Before your patch, the message shows the (possibly truncated) offending
> key name. With your patch, it shows the tail of the argument starting
> with the offending key name. Why is that an improvement?
I guess that's a matter of preference and the if() can be put after the
pstrcpy() without changing the error output.
> Could use %.*s to print exactly the offending key name.
Does that work on all supported platforms? (I see windows-related files
in the code base and last time I checked it doesn't do everything.)
If so then this + changing index_from_key() as you suggested above seems
to be the simplest option.
> What's wrong with Prasad's simple fix?
See above, breaks combined keys and eg. 'ctrl-alt-f1' then errors.
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Wolfgang Bumiller writes:
> 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
> 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
Well, it technically doesn't terminate,
> 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
> Signed-off-by: Wolfgang Bumiller
> ---
> 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;
Style nit: missing braces.
Works because the longest member of QKeyCode_lookup[] is 13 characters,
and therefore truncation implies "no match". But it's not obvious.
No worse than before, but "you touch it, you own it".
Options:
* Add a comments
char keyname_buf[16]; /* can hold any QKeyCode */
and
if (keyname_len >= sizeof(keyname_buf)) {
/* too long to match any QKeyCode */
goto err_out;
}
* Make index_from_key() take pointer into string and size instead of a
string.
* Get rid of the static buffer and malloc the sucker already.
> 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");
keyname_len = 4;
}
keyname_buf[keyname_len] = 0;
Why keep this assignment?
> @@ -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;
> }
Before your patch, the message shows the (possibly truncated) offending
key name. With your patch, it shows the tail of the argument starting
with the offending key name. Why is that an improvement?
Could use %.*s to print exactly the offending key name.
What's wrong with Prasad's simple fix?
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Luiz Capitulino writes:
> On Thu, 17 Dec 2015 18:10:59 +0530 (IST)
> 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.
>
> Markus, are you planning a pull request soon?
I have nothing queued for the monitor, but I can take this fix or the
proposed alternative through my tree anyway.
> Reviewed-by: Luiz Capitulino
>>
>> ===
>> From b0363f4c0e91671064dd7ffece8a6923c8dcaf20 Mon Sep 17 00:00:00 2001
>> From: Prasad J Pandit
>> 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
>> Signed-off-by: Prasad J Pandit
>> ---
>> 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[keyname_len] = 0;
>>
>> keylist = g_malloc0(sizeof(*keylist));
>> keylist->value = g_malloc0(sizeof(*keylist->value));
Minimally invasive fix, works for me.
Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
+-- 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
| 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
| Signed-off-by: Wolfgang Bumiller
| ---
| 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
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
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
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
Signed-off-by: Wolfgang Bumiller
---
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
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Hello, +-- On Sun, 10 Jan 2016, 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) Yes, if the patch is not yet merged upstream, it'd be good to include this CVE in the commit message. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
So, what's the status of this issue now? (it is CVE-2015-8619 btw, maybe worth to mention this in the commit message) Thanks, /mjt
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
+-- On Sat, 9 Jan 2016, Wolfgang Bumiller wrote --+ | > could say: if (!strcmp(keyname_buf, "<")). | | keyname_len+1 (size instead of length) to include the \0, then yes I think | strcmp can be used this way. The +1 should be fine there (since >= covers | it). Yes, right. -- - P J P 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
> On January 8, 2016 at 6:32 PM P J P wrote:
>
>
> +-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+
> | On Fri, Jan 08, 2016 at 07:29:31PM +0530, P J P wrote:
> | > + if (!strncmp(keyname_buf, "<-", 2))
> | > and remove the 'keyname_len' altogether.
> |
> | This wouldn't catch '<' without '-'. (`sendkey <`)
> | Also, strncmp with a length of 1 (in the original) seems weird.
>
> Ah, true.
>
> | keyname_len is not useless and perhaps it would be best to just do an
> | early error check there as I do below.
> |
> | Alternatively the if() can simply happen after pstrcpy() as a cut-off
> | error should be good enough anyway.
> |
> | @@ -1749,6 +1749,9 @@ 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);
>
> Yes, this looks good. With that, maybe 'keyname_len' could be sent to
> pstrcpy() above, instead of sizeof(keyname_buf)? If so, then the subsequent if
>
> could say: if (!strcmp(keyname_buf, "<")).
keyname_len+1 (size instead of length) to include the \0, then yes I think
strcmp can be used this way. The +1 should be fine there (since >= covers
it).
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
+-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+
| On Fri, Jan 08, 2016 at 07:29:31PM +0530, P J P wrote:
| > + if (!strncmp(keyname_buf, "<-", 2))
| > and remove the 'keyname_len' altogether.
|
| This wouldn't catch '<' without '-'. (`sendkey <`)
| Also, strncmp with a length of 1 (in the original) seems weird.
Ah, true.
| keyname_len is not useless and perhaps it would be best to just do an
| early error check there as I do below.
|
| Alternatively the if() can simply happen after pstrcpy() as a cut-off
| error should be good enough anyway.
|
| @@ -1749,6 +1749,9 @@ 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);
Yes, this looks good. With that, maybe 'keyname_len' could be sent to
pstrcpy() above, instead of sizeof(keyname_buf)? If so, then the subsequent if
could say: if (!strcmp(keyname_buf, "<")).
--
- P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
On Fri, Jan 08, 2016 at 07:29:31PM +0530, P J P wrote:
> +-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+
> | Ah yes, how could I miss that. Maybe just add a min() around the
> | keyname_len computation?
> |
> | - keyname_len = separator ? separator - keys : strlen(keys);
> | + keyname_len = MIN(sizeof(keyname_buf), separator ? separator - keys :
> strlen(keys))
>
> Actually, only use for 'keyname_len' is in the subsequent if statement,
And the replacing of the minus in combined keys.
> which IIUC compares the keyname_buf for "<" key. Maybe it could say
>
> + if (!strncmp(keyname_buf, "<-", 2))
>
> and remove the 'keyname_len' altogether.
This wouldn't catch '<' without '-'. (`sendkey <`)
Also, strncmp with a length of 1 (in the original) seems weird.
In the end my MIN() approach would be too forgiving when a key name is
longer than keyname_buf and its 15-byte substring exists (which I
doubt, though). Ie. {15chars}{and more} would be treated as {15chars}.
Worse with {15chars}{and more}-{anotherkey}.
keyname_len is not useless and perhaps it would be best to just do an
early error check there as I do below.
This makes sure no OOB write happens and changes the error output to
include the 'keys' part instead of keyname_buf. We can do this because
the two other `goto err_out` cases happen after using keyname_buf which
had been pstrcpy()d from the last keys, so 'keys' will contain the same
or more info, and works better for the new case since a string that is
too long might be cut off and then only part of it is displayed if the
input is say a combination of one too-long (thus invalid) key and a
second one.
Alternatively the if() can simply happen after pstrcpy() as a cut-off
error should be good enough anyway.
@@ -1749,6 +1749,9 @@ 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 +1803,7 @@ out:
return;
err_out:
-monitor_printf(mon, "invalid parameter: %s\n", keyname_buf);
+monitor_printf(mon, "invalid parameter: %s\n", keys);
goto out;
}
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
+-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+ | Ah yes, how could I miss that. Maybe just add a min() around the | keyname_len computation? | | - keyname_len = separator ? separator - keys : strlen(keys); | + keyname_len = MIN(sizeof(keyname_buf), separator ? separator - keys : strlen(keys)) Actually, only use for 'keyname_len' is in the subsequent if statement, which IIUC compares the keyname_buf for "<" key. Maybe it could say + if (!strncmp(keyname_buf, "<-", 2)) and remove the 'keyname_len' altogether. -- - P J P 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
On Fri, Jan 08, 2016 at 05:49:51PM +0530, P J P wrote:
>Hello,
>
> +-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+
> | > 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);
>
> The OOB issue occurs when there is no separator, and strlen(keys) is longer
> than '16' characters. In that case, "keyname_buf[keyname_len] = 0;" writes
> beyond the 'keyname_buf' array. It's removed because 'pstrcpy()' routine also
> null terminates the buffer.
Ah yes, how could I miss that. Maybe just add a min() around the
keyname_len computation?
- keyname_len = separator ? separator - keys : strlen(keys);
+ keyname_len = MIN(sizeof(keyname_buf), separator ? separator - keys :
strlen(keys))
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Hello,
+-- On Fri, 8 Jan 2016, Wolfgang Bumiller wrote --+
| > 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);
The OOB issue occurs when there is no separator, and strlen(keys) is longer
than '16' characters. In that case, "keyname_buf[keyname_len] = 0;" writes
beyond the 'keyname_buf' array. It's removed because 'pstrcpy()' routine also
null terminates the buffer.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
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
> 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
> Signed-off-by: Prasad J Pandit
> ---
> 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
>
>
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
On Thu, 17 Dec 2015 18:10:59 +0530 (IST)
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.
Markus, are you planning a pull request soon?
Reviewed-by: Luiz Capitulino
>
> ===
> From b0363f4c0e91671064dd7ffece8a6923c8dcaf20 Mon Sep 17 00:00:00 2001
> From: Prasad J Pandit
> 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
> Signed-off-by: Prasad J Pandit
> ---
> 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[keyname_len] = 0;
>
> keylist = g_malloc0(sizeof(*keylist));
> keylist->value = g_malloc0(sizeof(*keylist->value));
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Hello Prasad,
Can you give this a cve id?
Thank you.
-Original Message-
From: P J P [mailto:ppan...@redhat.com]
Sent: Thursday, December 17, 2015 8:41 PM
To: qemu-devel@nongnu.org
Cc: 刘令
Subject: [PATCH] hmp: avoid redundant null termination of buffer
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
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
Signed-off-by: Prasad J Pandit
---
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[keyname_len] = 0;
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
Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Hello Ling, +-- On Fri, 18 Dec 2015, 刘令 wrote --+ | Can you give this a cve id? Yes, I'll request one once it is accepted upstream. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
[Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
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
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
Signed-off-by: Prasad J Pandit
---
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[keyname_len] = 0;
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
