| Message ID | 20181230132856.24095-1-jlee@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | sysfs: Add hook for checking the file capability of opener | expand |
On Sun, Dec 30, 2018 at 09:28:54PM +0800, Lee, Chun-Yi wrote: > There have some discussion in the following mail loop about checking > capability in sysfs write handler: > https://lkml.org/lkml/2018/9/13/978 A sysfs callback should not care about stuff like this. Worst case, do a simple: if (!capable(CAP_FOO)) return -EPERM you don't care or need to worry about the file handle for that at all, right? > Sometimes we check the capability in sysfs implementation by using > capable function. Which should be fine, right? > But the checking can be bypassed by opening sysfs > file within an unprivileged process then writing the file within a > privileged process. The tricking way has been exposed by Andy Lutomirski > for CVE-2013-1959. And who does this for a sysfs file? And why? > Because the sysfs_ops does not forward the file descriptor to the > show/store callback, there doesn't have chance to check the capability > of file's opener. Which is by design. If you care about open, you are using sysfs wrong. > This patch adds the hook to sysfs_ops that allows > different implementation in object and attribute levels for checking > file capable before accessing sysfs interfaces. No, please no. > The callback function of kobject sysfs_ops is the first implementation > of new hook. It casts attribute to kobj_attribute then calls the file > capability callback function of attribute level. The same logic can > be implemented in other sysfs file types, like: device, driver and > bus type. > > The capability checking logic in wake_lock/wake_unlock sysfs interface > is the first example for kobject. It will check the opener's capability. Why doesn't the file permission of that sysfs file determine who can or can not write to that file? thanks, greg k-h
Hi Greg, Thanks for your review! On Sun, Dec 30, 2018 at 03:45:06PM +0100, Greg Kroah-Hartman wrote: > On Sun, Dec 30, 2018 at 09:28:54PM +0800, Lee, Chun-Yi wrote: > > There have some discussion in the following mail loop about checking > > capability in sysfs write handler: > > https://lkml.org/lkml/2018/9/13/978 > > A sysfs callback should not care about stuff like this. > > Worst case, do a simple: > if (!capable(CAP_FOO)) > return -EPERM > > you don't care or need to worry about the file handle for that at all, > right? > The capable() can be bypassed. Unprivileged process may reads or writes those sysfs if file permission be relaxed by root for non-root user. > > Sometimes we check the capability in sysfs implementation by using > > capable function. > > Which should be fine, right? > If file permission is enough to restrict sysfs that can only be used by root. Why do some sysfs interfaces use capable()? It's not redundancy? > > But the checking can be bypassed by opening sysfs > > file within an unprivileged process then writing the file within a > > privileged process. The tricking way has been exposed by Andy Lutomirski > > for CVE-2013-1959. > > And who does this for a sysfs file? And why? > Just want to bypass the capable() checking. > > Because the sysfs_ops does not forward the file descriptor to the > > show/store callback, there doesn't have chance to check the capability > > of file's opener. > > Which is by design. If you care about open, you are using sysfs wrong. > OK~ So the sysfs doesn't care opener's capability. > > This patch adds the hook to sysfs_ops that allows > > different implementation in object and attribute levels for checking > > file capable before accessing sysfs interfaces. > > No, please no. > > > The callback function of kobject sysfs_ops is the first implementation > > of new hook. It casts attribute to kobj_attribute then calls the file > > capability callback function of attribute level. The same logic can > > be implemented in other sysfs file types, like: device, driver and > > bus type. > > > > The capability checking logic in wake_lock/wake_unlock sysfs interface > > is the first example for kobject. It will check the opener's capability. > > Why doesn't the file permission of that sysfs file determine who can or > can not write to that file? > I agree that the file permission can restrict the writer of sysfs. But, I still confused for why do some sysfs interface use capable()? Thanks a lot! Joey Lee
On Mon, Dec 31, 2018 at 05:41:05PM +0800, joeyli wrote: > Hi Greg, > > Thanks for your review! > > On Sun, Dec 30, 2018 at 03:45:06PM +0100, Greg Kroah-Hartman wrote: > > On Sun, Dec 30, 2018 at 09:28:54PM +0800, Lee, Chun-Yi wrote: > > > There have some discussion in the following mail loop about checking > > > capability in sysfs write handler: > > > https://lkml.org/lkml/2018/9/13/978 > > > > A sysfs callback should not care about stuff like this. > > > > Worst case, do a simple: > > if (!capable(CAP_FOO)) > > return -EPERM > > > > you don't care or need to worry about the file handle for that at all, > > right? > > > > The capable() can be bypassed. How? > Unprivileged process may reads or writes those sysfs if file > permission be relaxed by root for non-root user. So if root says "I want a non-root user to be able to write to this file" by changing its permissions, yes, a non-root user will be able to write to them. But the capable() check will catch this, right? And why is the kernel trying to protect userspace from itself? Root explicitly asked for anyone to read/write this file, why do we think the kernel knows better? > > > Sometimes we check the capability in sysfs implementation by using > > > capable function. > > > > Which should be fine, right? > > > > If file permission is enough to restrict sysfs that can only be used > by root. Why do some sysfs interfaces use capable()? It's not > redundancy? I don't know why some sysfs files use this, it was the choice of that author it seems. Perhaps it is to "check" that the root user really does have that capability. Some root users do not have all capabilities if they were previously "dropped". > > > But the checking can be bypassed by opening sysfs > > > file within an unprivileged process then writing the file within a > > > privileged process. The tricking way has been exposed by Andy Lutomirski > > > for CVE-2013-1959. > > > > And who does this for a sysfs file? And why? > > Just want to bypass the capable() checking. But why would you want to bypass this? I don't understand. What is the use case you are trying to solve for here. > > > Because the sysfs_ops does not forward the file descriptor to the > > > show/store callback, there doesn't have chance to check the capability > > > of file's opener. > > > > Which is by design. If you care about open, you are using sysfs wrong. > > > > OK~ So the sysfs doesn't care opener's capability. Exactly. Except for the permission/mode that the file has on it. The vfs checks this at open time and either allows it or forbids it. > > > This patch adds the hook to sysfs_ops that allows > > > different implementation in object and attribute levels for checking > > > file capable before accessing sysfs interfaces. > > > > No, please no. > > > > > The callback function of kobject sysfs_ops is the first implementation > > > of new hook. It casts attribute to kobj_attribute then calls the file > > > capability callback function of attribute level. The same logic can > > > be implemented in other sysfs file types, like: device, driver and > > > bus type. > > > > > > The capability checking logic in wake_lock/wake_unlock sysfs interface > > > is the first example for kobject. It will check the opener's capability. > > > > Why doesn't the file permission of that sysfs file determine who can or > > can not write to that file? > > > > I agree that the file permission can restrict the writer of sysfs. But, > I still confused for why do some sysfs interface use capable()? Go ask the individual authors who added those checks :) thanks, greg k-h
There have some discussion in the following mail loop about checking capability in sysfs write handler: https://lkml.org/lkml/2018/9/13/978 Sometimes we check the capability in sysfs implementation by using capable function. But the checking can be bypassed by opening sysfs file within an unprivileged process then writing the file within a privileged process. The tricking way has been exposed by Andy Lutomirski for CVE-2013-1959. Because the sysfs_ops does not forward the file descriptor to the show/store callback, there doesn't have chance to check the capability of file's opener. This patch adds the hook to sysfs_ops that allows different implementation in object and attribute levels for checking file capable before accessing sysfs interfaces. The callback function of kobject sysfs_ops is the first implementation of new hook. It casts attribute to kobj_attribute then calls the file capability callback function of attribute level. The same logic can be implemented in other sysfs file types, like: device, driver and bus type. The capability checking logic in wake_lock/wake_unlock sysfs interface is the first example for kobject. It will check the opener's capability. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> Cc: Chen Yu <yu.c.chen@intel.com> Cc: Giovanni Gherdovich <ggherdovich@suse.cz> Cc: Jann Horn <jannh@google.com> Cc: Andy Lutomirski <luto@kernel.org> Cc: Pavel Machek <pavel@ucw.cz> Cc: Len Brown <len.brown@intel.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Joe Perches <joe@perches.com> Cc: Bart Van Assche <bvanassche@acm.org> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> Lee, Chun-Yi (2): sysfs: Add hook for checking the file capable PM / Sleep: Checking the file capability when writing wak lock interface fs/sysfs/file.c | 8 ++++++++ include/linux/kobject.h | 2 ++ include/linux/sysfs.h | 2 ++ kernel/power/main.c | 14 ++++++++++++++ kernel/power/wakelock.c | 6 ------ lib/kobject.c | 26 ++++++++++++++++++++++++++ 6 files changed, 52 insertions(+), 6 deletions(-)