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

[David Lechner]

On 09/16/2016 02:07 AM, Jacek Anaszewski wrote:
> On 09/16/2016 07:50 AM, Pavel Machek wrote:
> > Hi!
> >
> > > > > > > +    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.

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.