Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges

[Dmitry Torokhov <dtor@google.com>]

On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Hi Dmitry,
>
> On Wed, Nov 14, 2018 at 02:28:56PM -0800, 'Dmitry Torokhov' via syzkaller-bugs wrote:
> > On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > From: Eric Biggers <ebiggers@google.com>
> > > >
> > > > 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.
> > > >
> > > > Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to
> > > > Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess
> > > > helpers fault on kernel addresses"), allowing this bug to be found.
> > > >
> > > > Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
> > > > Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")
> > > > Cc: <stable@vger.kernel.org> # v3.6+
> > > > Cc: Jann Horn <jannh@google.com>
> > > > Cc: Andy Lutomirski <luto@kernel.org>
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > >
> > > Reviewed-by: Jann Horn <jannh@google.com>
> > >
> > > > ---
> > > >  drivers/hid/uhid.c | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> > > > index 3c55073136064..051639c09f728 100644
> > > > --- a/drivers/hid/uhid.c
> > > > +++ b/drivers/hid/uhid.c
> > > > @@ -12,6 +12,7 @@
> > > >
> > > >  #include <linux/atomic.h>
> > > >  #include <linux/compat.h>
> > > > +#include <linux/cred.h>
> > > >  #include <linux/device.h>
> > > >  #include <linux/fs.h>
> > > >  #include <linux/hid.h>
> > > > @@ -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?
> >
>
> Actually uhid doesn't have any capability checks, so it's up to userspace to
> assign permissions to the device node.

Yes. There are quite a few such instances where kernel does not bother
implementing superfluous checks and instead relies on userspace to
provide sane environment. IIRC nobody in the kernel enforces root
filesystem not be accessible to ordinary users, it is done with
generic permission checks.

> I think it's best not to make
> assumptions about how the interface will be used and to be consistent with how
> other ->write() methods in the kernel handle the misfeature where a __user
> pointer in the write() or read() payload is dereferenced.

I can see that you might want to check credentials, etc, if interface
can be accessed by unprivileged process, however is it a big no no for
uhid/userio/uinput devices.

> Temporarily switching
> to USER_DS would only avoid one of the two problems.

So because of the above there is only one problem. If your system
opened access to uhid to random processes you have much bigger
problems than exposing some data from a suid binary. You can spam "rm
-rf .; rm -rf /" though uhid if there is interactive session
somewhere.

>
> Do you think the proposed restrictions would actually break anything?

It would break if someone uses UHID_CREATE with sendpage. I do not
know if anyone does. If we were certain there are no users we'd simply
removed UHID_CREATE altogether.

>
> - Eric
>
> > > > +               if (file->f_cred != current_cred() || uaccess_kernel()) {
> > > > +                       pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n",
> > > > +                                   task_tgid_vnr(current), current->comm);
> > > > +                       ret = -EACCES;
> > > > +                       goto unlock;
> > > > +               }
> > > >                 ret = uhid_dev_create(uhid, &uhid->input_buf);
> > > >                 break;
> > > >         case UHID_CREATE2:
> > > > --

Thanks.

-- 
Dmitry