On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> 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.
> More specific comments inlined:
>
> On 28/08/13 22:31, Jiri Kosina wrote:
>> From: Kees Cook <keescook@xxxxxxxxxxxx>
>>
>> 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 <keescook@xxxxxxxxxxxx>
>> Cc: stable@xxxxxxxxxx
>> ---
>> 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.
>> + 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.
> 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.
>> 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[].
Thanks for the review!
-Kees
>
> Cheers,
> Benjamin
>
>> }
>>
>> for (r = 0; r < report->maxfield; r++) {
>>
>
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
