On 09/16/2016 07:50 AM, Pavel Machek wrote:

+    if (copy_from_user(&udev->user_dev, buffer,
+               sizeof(struct uleds_user_dev))) {
+        ret = -EFAULT;
+        goto out;
+    }
+    if (!udev->user_dev.name[0]) {
+        ret = -EINVAL;
+        goto out;
+    }
+    ret = led_classdev_register(NULL, &udev->led_cdev);
+    if (ret < 0)
+        goto out;

No sanity checking on the name -> probably a security hole. Do not
push this upstream before this is fixed.

If this is a serious security issue, then you should also raise an issue
with input maintainers because this is the extent of sanity checking for
uinput device names as well.

I guess that should be fixed. But lets not add new ones.

I must confess that I am no security expert, so unless you can give specific
examples of what potential threats are, I will not be able to guess what I
need to do to fix it.

After some digging around the kernel, I don't see many instances of
validating device node names. The best I have found so far comes from
create_entry() in binfmt_misc.c

        if (!e->name[0] ||
            !strcmp(e->name, ".") ||
            !strcmp(e->name, "..") ||
            strchr(e->name, '/'))
                goto einval;

Would something like this be a sufficient sanity check? I suppose we could
also check for non-printing characters, but I don't think ignoring them
would be a security issue.

That would be minimum, yes. I guess it would be better/easier to just
limit the names to [a-zA-Z:-_0-9]*?

Right, and we also could check if there are no more then two ":"
characters in the name.

Best regards,
Jacek Anaszewski

Reply via email to