linux-kernel - Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D1DFB962-DAC7-4CD6-A627-4A69062EDDE5@amacapital.net>
Date:   Wed, 14 Nov 2018 16:39:41 -0800
From:   Andy Lutomirski <luto@...capital.net>
To:     Dmitry Torokhov <dtor@...gle.com>
Cc:     jannh@...gle.com, ebiggers@...nel.org, dh.herrmann@...glemail.com,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        syzkaller-bugs@...glegroups.com,
        Dmitry Vyukov <dvyukov@...gle.com>,
        syzbot+72473edc9bf4eb1c6556@...kaller.appspotmail.com,
        stable@...r.kernel.org, luto@...nel.org
Subject: Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges



> On Nov 14, 2018, at 2:46 PM, Dmitry Torokhov <dtor@...gle.com> wrote:
> 
>> On Wed, Nov 14, 2018 at 2:38 PM Jann Horn <jannh@...gle.com> wrote:
>> 
>>> On Wed, Nov 14, 2018 at 11:29 PM Dmitry Torokhov <dtor@...gle.com> wrote:
>>>> On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@...gle.com> wrote:
>>>>> On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@...nel.org> wrote:
>>>>> When a UHID_CREATE command is written to the uhid char device, a
>>>>> copy_from_user() is done from a user pointer embedded in the command.
>>>>> When the address limit is KERNEL_DS, e.g. as is the case during
>>>>> sys_sendfile(), this can read from kernel memory.  Alternatively,
>>>>> information can be leaked from a setuid binary that is tricked to write
>>>>> to the file descriptor.  Therefore, forbid UHID_CREATE in these cases.
>>>>> 
>>>>> No other commands in uhid_char_write() are affected by this bug and
>>>>> UHID_CREATE is marked as "obsolete", so apply the restriction to
>>>>> UHID_CREATE only rather than to uhid_char_write() entirely.
>> [...]
>>>>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> [...]
>>>>> @@ -722,6 +723,17 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
>>>>> 
>>>>>        switch (uhid->input_buf.type) {
>>>>>        case UHID_CREATE:
>>>>> +               /*
>>>>> +                * 'struct uhid_create_req' contains a __user pointer which is
>>>>> +                * copied from, so it's unsafe to allow this with elevated
>>>>> +                * privileges (e.g. from a setuid binary) or via kernel_write().
>>>>> +                */
>>> 
>>> uhid is a privileged interface so we would go from root to less
>>> privileged (if at all). If non-privileged process can open uhid it can
>>> construct virtual keyboard and inject whatever keystrokes it wants.
>>> 
>>> Also, instead of disallowing access, can we ensure that we switch back
>>> to USER_DS before trying to load data from the user pointer?
>> 
>> Does that even make sense? You are using some deprecated legacy
>> interface; you interact with it by splicing a request from something
>> like a file or a pipe into the uhid device; but the request you're
>> splicing through contains a pointer into userspace memory? Do you know
>> of anyone who is actually doing that? If not, anyone who does want to
>> do this for some reason in the future can just go use UHID_CREATE2
>> instead.
> 
> I do not know if anyone is still using UHID_CREATE with sendpage and
> neither do you really. It is all about not breaking userspace without
> good reason and here ensuring that we switch to USER_DS and then back
> to whatever it was does not seem too hard.

It’s about not breaking userspace *except as needed for security fixes*. User pointers in a write() payload is a big no-no.

Also, that f_cred hack is only barely enough. This isn’t just about attacking suid things — this bug allows poking at the address space of an unsuspecting process. So, if a privileged program opens a uhid fd and is then tricked into writing untrusted data to the same fd (which is supposed to be safe), then we have a problem. Fortunately, identically privileged programs usually still don’t share a cred pointer unless they came from the right place.

The real right fix is to remove UHID_CREATE outright. This is terminally broken.