From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: [PATCH 11/14] HID: multitouch: validate feature report details Date: Wed, 4 Sep 2013 09:31:00 -0700 Message-ID: References: <521F0D5F.7040108@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: Received: from mail-ie0-f176.google.com ([209.85.223.176]:64207 "EHLO mail-ie0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934810Ab3IDQbA (ORCPT ); Wed, 4 Sep 2013 12:31:00 -0400 Received: by mail-ie0-f176.google.com with SMTP id s9so1086927iec.7 for ; Wed, 04 Sep 2013 09:31:00 -0700 (PDT) In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Benjamin Tissoires Cc: Benjamin Tissoires , Jiri Kosina , linux-input , Henrik Rydberg On Fri, Aug 30, 2013 at 11:27 AM, Kees Cook wrote: > On Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires > wrote: >> On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook wrote: >>> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires >>> wrote: >>>> Hi Kees, >>>> >>>> I would be curious to have the HID report descriptors (maybe off list) >>>> to understand how things can be that bad. >>> >>> Certainly! I'll send them your way. I did have to get pretty creative >>> to tickle these conditions. >>> >>>> On overall, I'd prefer all those checks to be in hid-core so that we >>>> have the guarantee that we don't have to open a new CVE each time a >>>> specific hid driver do not check for these ranges. >>> >>> I pondered doing this, but it seemed like something that needed wider >>> discussion, so I thought I'd start with just the dump of fixes. It >>> seems like the entire HID report interface should use access functions >>> to enforce range checking -- perhaps further enforced by making the >>> structure opaque to the drivers. >> >> The problem with access functions with range checking is when they are >> used at each input report. This will overload the kernel processing >> for static checks. >> >>> >>>> More specific comments inlined: >>>> >>>> On 28/08/13 22:31, Jiri Kosina wrote: >>>>> From: Kees Cook >>>>> >>>>> When working on report indexes, always validate that they are in bounds. >>>>> Without this, a HID device could report a malicious feature report that >>>>> could trick the driver into a heap overflow: >>>>> >>>>> [ 634.885003] usb 1-1: New USB device found, idVendor=0596, idProduct=0500 >>>>> ... >>>>> [ 676.469629] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten >>>>> >>>>> CVE-2013-2897 >>>>> >>>>> Signed-off-by: Kees Cook >>>>> Cc: stable@kernel.org >>>>> --- >>>>> drivers/hid/hid-multitouch.c | 25 ++++++++++++++++++++----- >>>>> 1 file changed, 20 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c >>>>> index cb0e361..2aa275e 100644 >>>>> --- a/drivers/hid/hid-multitouch.c >>>>> +++ b/drivers/hid/hid-multitouch.c >>>>> @@ -330,9 +330,18 @@ static void mt_feature_mapping(struct hid_device *hdev, >>>>> break; >>>>> } >>>>> } >>>>> + /* Ignore if value index is out of bounds. */ >>>>> + if (td->inputmode_index < 0 || >>>> >>>> td->inputmode_index can not be less than 0 >>> >>> Well, it certainly _shouldn't_ be less than zero. :) However, it is >>> defined as s8, and gets set from an int: >>> >>> for (i=0; i < field->maxusage; i++) { >>> if (field->usage[i].hid == usage->hid) { >>> td->inputmode_index = i; >>> break; >>> } >>> } >>> >>> Both "i" and "maxusage" are int, and I can generate a large maxusage >>> and usage array where the first matching hid equality happens when i >>> is >127, causing inputmode_index to wrap. >> >> ouch. Sorry. This piece of code (the current code) is junk. We should >> switch to usage->usage_index and change from __s8 to __s16 the >> declaration ASAP. >> >>> >>>>> + td->inputmode_index >= field->report_count) { >>>> >>>> if this is really required, we could just change the for loop above to >>>> go from 0 to field->report_count instead. >>> >>> That's certainly true, but since usage count and report count are not >>> directly associated, I don't know if there are devices that will freak >>> out with this restriction. >> >> Actually, I just again another long time understanding all the mess of >> having usage count and report count different in hid-core. >> >> I think it is a bug at some point (but it looks like I tend to be >> wrong on a lot of things recently :-P ), because when you look at the >> actual code of hid_input_field() in hid-core.c, we never go further >> than field->report_count when dealing with input report. >> So my guess is that we are declaring extra usages that are never used >> to parse incoming data. >> >>> >>>> However, I think we could just rely on usage->usage_index to get the >>>> actual index. >>>> I'll do more tests today and tell you later. >>>> >>>>> + dev_err(&hdev->dev, "HID_DG_INPUTMODE out of range\n"); >>>>> + td->inputmode = -1; >>>>> + } >>>>> >>>>> break; >>>>> case HID_DG_CONTACTMAX: >>>>> + /* Ignore if value count is out of bounds. */ >>>>> + if (field->report_count < 1) >>>>> + break; >>>> >>>> If you can trigger this, I would say that the fix should be in hid-core, >>>> not in every driver. A null report_count should not be allowed by the >>>> HID protocol. >>> >>> Again, I have no problem with that idea, but there seem to be lots of >>> general cases where this may not be possible (i.e. a HID with 0 OUTPUT >>> or FEATURE reports). I didn't see a sensible way to approach this in >>> core without declaring a "I require $foo many OUTPUT reports, $bar >>> many INPUT, and $baz many FEATURE" for each driver. I instead opted >>> for the report validation function instead. >>> >> >> I think we can just add this check in both hidinput_configure_usage() >> and report_features() (both in hid-input.c) so that we don't call the >> *_mapping() callbacks with non sense fields. Can you suggest a patch for this? (And please include reference to CVE-2013-2897 in the commit.) I'll send a v2 of the rest of the series. Thanks! -Kees >> This way, the specific hid drivers will know only the correct fields, >> and don't need to check this. So patches 9, 11 and 13 can be amended. >> >> It looks to me that you mismatched field->report_count and the actual >> report_count in your answer: field->report_count is the number of >> consecutive fields of field->report_size that are present for the >> parsing of the report. It has nothing to do with the total report >> count. >> >>>>> td->maxcontact_report_id = field->report->id; >>>>> td->maxcontacts = field->value[0]; >>>>> if (!td->maxcontacts && >>>>> @@ -743,15 +752,21 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report) >>>>> unsigned count; >>>>> int r, n; >>>>> >>>>> + if (report->maxfield == 0) >>>>> + return; >>>>> + >>>>> /* >>>>> * Includes multi-packet support where subsequent >>>>> * packets are sent with zero contactcount. >>>>> */ >>>>> - if (td->cc_index >= 0) { >>>>> - struct hid_field *field = report->field[td->cc_index]; >>>>> - int value = field->value[td->cc_value_index]; >>>>> - if (value) >>>>> - td->num_expected = value; >>>>> + if (td->cc_index >= 0 && td->cc_index < report->maxfield) { >>>>> + field = report->field[td->cc_index]; >>>> >>>> looks like we previously overwrote the definition of field :( >>>> >>>>> + if (td->cc_value_index >= 0 && >>>>> + td->cc_value_index < field->report_count) { >>>>> + int value = field->value[td->cc_value_index]; >>>>> + if (value) >>>>> + td->num_expected = value; >>>>> + } >>>> >>>> I can not see why td->cc_index and td->cc_value_index could have bad >>>> values. They are initially created by hid-core, and are not provided by >>>> the device. >>>> Anyway, if you are able to produce bad values with them, I'd rather have >>>> those checks during the assignment of td->cc_index (in >>>> mt_touch_input_mapping()), instead of checking them for each report. >>> >>> Well, the problem comes again from there not being a hard link between >>> the usage array and the value array: >>> >>> td->cc_value_index = usage->usage_index; >>> ... >>> int value = field->value[td->cc_value_index]; >>> >>> And all of this would be irrelevant if there was an access function >>> for field->value[]. >> >> True, with the current implementation, td->cc_value_index can be >> greater than field->report_count. Still, moving this check in >> mt_touch_input_mapping() will allow us to make it only once. >> >>> >>> Thanks for the review! >> >> you are welcome :) > > Okay, so, where does the whole patch series stand? It seems like the > checks are all worth adding; and my fixes are, I think, "minimal" so > having them go into the tree (so that they land in stable) seems like > a good idea. The work beyond that could be done on top of the existing > patches? There seems to be a lot of redesign ideas, but those probably > aren't so great for stable, and I'm probably not the best person to > write them, since everything I know about HID code I learned two weeks > ago. :) > > Given the 12 flaws, what do you see as the best way forward? -- Kees Cook Chrome OS Security