Re: [PATCH v3] leds: Introduce userspace leds driver

From: David Lechner
Date: Fri Sep 16 2016 - 11:10:03 EST

On 09/16/2016 02:07 AM, Jacek Anaszewski wrote:
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->[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
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
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.

Again, I am going to disagree here. docs/sysfs-rules.txt says nothing about restricting characters for device names, so I don't think we should do so here. In fact, the only thing it says about names is "applications need to handle spaces and characters like '!' in the name". My opinion is that if people want to give devices dumb names with special characters and spaces, we should let them.

If someone can point out a real security issue here, then I will gladly fix it, otherwise I am inclined to leave it as it is (with the checks for '.', '..' and '/').

If this was a regular userspace library, I would feel differently, but since the kernel has limited means to pass errors to userspace, all of these checks will pass the same -EINVAL to userspace if they fail. We could print error messages to the kernel log, but it is really annoying to have to check the kernel log to find out why your userspace application is not working. Any if you are not a kernel hacker, you would probably not even know to check the kernel logs.