Bug 1054127 (CVE-2016-5863)

Summary: VUL-0: CVE-2016-5863: kernel-source: usbhid: ioctl handler in the Linux kernel, several sanity checks are missing which can lead to out-of-bounds accesses.
Product: [Novell Products] SUSE Security Incidents Reporter: Marcus Meissner <meissner>
Component: IncidentsAssignee: Security Team bot <security-team>
Status: RESOLVED DUPLICATE QA Contact: Security Team bot <security-team>
Severity: Normal    
Priority: P3 - Medium CC: smash_bz
Version: unspecified   
Target Milestone: ---   
Hardware: Other   
OS: Other   
URL: https://smash.suse.de/issue/190600/
Whiteboard: CVSSv2:NVD:CVE-2016-5863:9.3:(AV:N/AC:M/Au:N/C:C/I:C/A:C) CVSSv3:NVD:CVE-2016-5863:7.8:(AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H)
Found By: Security Response Team Services Priority:
Business Priority: Blocker: ---
Marketing QA Status: --- IT Deployment: ---

Description Marcus Meissner 2017-08-16 20:49:28 UTC 
CVE-2016-5863

In an ioctl handler in all Qualcomm products with Android releases from CAF
using the Linux kernel, several sanity checks are missing which can lead to
out-of-bounds accesses.

References:
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2016-5863
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-5863
https://source.codeaurora.org/quic/la/kernel/msm-3.18/commit/?id=daf0acd54a6a80de227baef9a06285e4aa5f8c93
Comment 1 Marcus Meissner 2017-08-16 20:51:24 UTC 
This was already fixed in mainline

by commit 93a2001bdfd5376c3dc2158653034c20392d15c5
Author: Scott Bauer <sbauer@plzdonthack.me>
Date:   Thu Jun 23 08:59:47 2016 -0600

    HID: hiddev: validate num_values for HIDIOCGUSAGES, HIDIOCSUSAGES commands
    
    This patch validates the num_values parameter from userland during the
    HIDIOCGUSAGES and HIDIOCSUSAGES commands. Previously, if the report id was set
    to HID_REPORT_ID_UNKNOWN, we would fail to validate the num_values parameter
    leading to a heap overflow.
    
    Cc: stable@vger.kernel.org
    Signed-off-by: Scott Bauer <sbauer@plzdonthack.me>
    Signed-off-by: Jiri Kosina <jkosina@suse.cz>


and so is a duplicated of bug 986572 aka CVE-2016-5829

*** This bug has been marked as a duplicate of bug 986572 ***
Comment 2 Marcus Meissner 2017-08-16 20:53:58 UTC 
that said, the patch is a bit different.

It also moves out this block out of the else branch, which the mainline fix does not do.

                        if (cmd == HIDIOCGCOLLECTIONINDEX) {
                                if (uref->usage_index >= field->maxusage)
                                        goto inval;
                        } else if (uref->usage_index >= field->report_count)
                                goto inval;

So half is fixed, rest needs review.
Comment 3 Michal Marek 2017-08-21 09:07:38 UTC 
Oliver, can you please check whether we are still vulnerable or not?
Comment 4 Oliver Neukum 2017-08-21 12:49:14 UTC 
(In reply to Marcus Meissner from comment #2)
> that said, the patch is a bit different.
> 
> It also moves out this block out of the else branch, which the mainline fix
> does not do.
> 
>                         if (cmd == HIDIOCGCOLLECTIONINDEX) {
>                                 if (uref->usage_index >= field->maxusage)
>                                         goto inval;
>                         } else if (uref->usage_index >= field->report_count)
>                                 goto inval;
> 
> So half is fixed, rest needs review.

If this code branch without the sanity check is executed

if (uref->report_id == HID_REPORT_ID_UNKNOWN)

hiddev_lookup_usage() is executed. If rewrites some fields of uref


                                if (field->usage[j].hid == uref->usage_code) {
                                        uref->report_id = report->id;
                                        uref->field_index = i;
                                        uref->usage_index = j;


Thus uref->usage_index in that case comes from a kernel data structure not provided by user land, hence need not be validated.

This code and the upstream fix leaving the check conditional is secure; readable code it is not.
Comment 5 Marcus Meissner 2017-08-21 14:06:48 UTC 
ok, thanks for checking!
Comment 6 Marcus Meissner 2017-08-21 14:07:15 UTC 
dup of bug 986572

*** This bug has been marked as a duplicate of bug 986572 ***