On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov [off-list ref] wrote:

On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers [off-list ref] wrote:

quoted

Hi Dmitry,

On Wed, Nov 14, 2018 at 02:28:56PM -0800, 'Dmitry Torokhov' via syzkaller-bugs wrote:

quoted

On Wed, Nov 14, 2018 at 2:05 PM Jann Horn [off-list ref] wrote:

quoted

On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers [off-list ref] wrote:

quoted

From: Eric Biggers <redacted>

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: <redacted> # v3.6+
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Eric Biggers <redacted>

Reviewed-by: Jann Horn <jannh@google.com>

quoted

---
 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.

quoted

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.

Yep, any sane distribution would restrict the permissions of
uhid/userio/uinput to only be accessed by root. If that ever changes,
there is already an issue with the system and it was compromised
either by a terribly dizzy sysadmin.

quoted

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.

quoted

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.

AFAICT, there are 2 users of uhid:
- bluez for BLE devices (using HID over GATT)
- hid-replay for debugging.

There might be a few other users that are making some user space
driver out of opencv, but I wouldn't expect those to be really
widespread.

IIRC, bluez uses UHID_CREATE2 and hid-replay should also (or ought to
be, but this can be easily fixed as I maintain the code and I am the
almost sole user).

Regarding the sendpage fix, doesn't David's patch fixes the issue
already (https://patchwork.kernel.org/patch/10682549/).

I am fine applying whatever patch that fixes the security issues, as
long as it doesn't break bluez nor the hid-replay uses I have for
debugging and regression testing.

Cheers,
Benjamin

quoted

- Eric

quoted

quoted

quoted

+               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