linux-kernel - Re: [PATCH] HID: uhid: fix a missing-check bug

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] [day] [month] [year] [list]

Date:   Mon, 7 May 2018 08:27:27 +0200
From:   David Herrmann <dh.herrmann@...il.com>
To:     Wenwen Wang <wang6495@....edu>
Cc:     Kangjie Lu <kjlu@....edu>,
        David Herrmann <dh.herrmann@...glemail.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        "open list:UHID USERSPACE HID IO DRIVER:" 
        <linux-input@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCH] HID: uhid: fix a missing-check bug

Hey

On Sat, May 5, 2018 at 6:17 AM, Wenwen Wang <wang6495@....edu> wrote:
> In uhid_event_from_user(), if it is in_compat_syscall(), the 'type' of the
> event is first fetched from the 'buffer' in userspace and checked. If the
> 'type' is UHID_CREATE, it is a messed up request with compat pointer, which
> could be more than 256 bytes, so it is better allocated from the heap, as
> mentioned in the comment. Its fields are then prepared one by one instead
> of using a whole copy. For all other cases, the event object is copied
> directly from user space. In other words, based on the 'type', the memory
> size and structure of the event object vary.
>
> Given that the 'buffer' resides in userspace, a malicious userspace process
> can race to change the 'type' between the two copies, which will cause
> inconsistency issues, potentially security issues. Plus, various operations
> such as uhid_dev_destroy() and uhid_dev_input() are performed based on
> 'type' in function uhid_char_write(). If 'type' is modified by user, there
> could be some issues such as uninitialized uses.
>
> To fix this problem, we need to recheck the type after the second fetch to
> make sure it is not UHID_CREATE.
>
> Signed-off-by: Wenwen Wang <wang6495@....edu>
> ---
>  drivers/hid/uhid.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 3c55073..0220385 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -447,11 +447,17 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
>                         event->u.create.country = compat->country;
>
>                         kfree(compat);
> -                       return 0;
> +               } else {
> +                       if (copy_from_user(event, buffer,
> +                                       min(len, sizeof(*event))))
> +                               return -EFAULT;
> +                       if (event->type == UHID_CREATE)
> +                               return -EINVAL;

You are correct, the race against malicious/buggy user-space exists.
However, it is harmless. Lets assume user-space rewrites the input,
all that happens is that we end up with garbage in the kernel.
However, input-validation is done *after* this function, hence
everything will work just fine. If user-space rewrites input-buffers,
they better be aware that this might mean random garbage is pushed
into the kernel.

IOW, this race simply allows user-space to circumvent the "compat
conversion". But if user-space wants that... let them. No point in
protecting them from doing something stupid.

Thanks
David

>                 }
> -               /* All others can be copied directly */
> +               return 0;
>         }
>
> +       /* Others can be copied directly */
>         if (copy_from_user(event, buffer, min(len, sizeof(*event))))
>                 return -EFAULT;
>
> --
> 2.7.4
>