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
