| Message ID | 20160111075914.GA28466@olga |
|---|---|
| State | New |
| Headers | show |
+-- 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 <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;
| }
It looks good.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Wolfgang Bumiller <w.bumiller@proxmox.com> 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 <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 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 <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; 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?
> On January 12, 2016 at 9:45 AM Markus Armbruster <armbru@redhat.com> wrote: > > Wolfgang Bumiller <w.bumiller@proxmox.com> 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.
Wolfgang Bumiller <w.bumiller@proxmox.com> writes: >> On January 12, 2016 at 9:45 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> Wolfgang Bumiller <w.bumiller@proxmox.com> 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?
> On January 12, 2016 at 5:00 PM Markus Armbruster <armbru@redhat.com> 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.
Wolfgang Bumiller <w.bumiller@proxmox.com> writes: >> On January 12, 2016 at 5:00 PM Markus Armbruster <armbru@redhat.com> 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.
=== 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; }