Messages in this thread | | | Subject | Re: [PATCH v3] leds: Introduce userspace leds driver | From | Jacek Anaszewski <> | Date | Fri, 16 Sep 2016 09:07:45 +0200 |
| |
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.
-- Best regards, Jacek Anaszewski
| |