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

Message-Id: <1525493850-6952-1-git-send-email-wang6495@umn.edu>
Date:   Fri,  4 May 2018 23:17:29 -0500
From:   Wenwen Wang <wang6495@....edu>
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>,
        linux-input@...r.kernel.org (open list:UHID USERSPACE HID IO DRIVER:),
        linux-kernel@...r.kernel.org (open list)
Subject: [PATCH] HID: uhid: fix a missing-check bug

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;
 		}
-		/* 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