Currently there is no good way to effectively globally restrict an
access to sysfs files. It's possible only to chmod the sysfs'
root/directories to fully deny access to sysfs (sub-)tree to some users
or chmod files after they are created. The latter approach is racy,
however.
The patch introduces sysfs mount options parsing and adds 4 new options:
uid, gid, mode and umask. uid, gid, and umask are classical options,
mode is a global restricting mode mask that defined the most relaxed
possible file mode. E.g. if mode=0750 then "chmod 0664" changes file's
permissions to 0640.
If one uses "umask" then he also has to chmod already existing sysfs
files. "mode" tries to simplify things by restricting _all_ files'
permissions - both already existing and will be created in the future.
uid, gid, and umask values should be reachable when kobject is
created, so it must not be located in sysfs' superblock - there is no
one to one mapping of kobjects and sysfs_super_info. (If there are any
thoughts about how kobject <-> superblock connection may be introduced,
please tell me.) Only networking kobjects are connected to
sysfs_super_info with the same net namespace, but if/when other
namespaces are added to sysfs_super_info.ns it would be changed.
Currently uid, gid, umask are stored in global struct. Only root
process of the main namespace should change uid, gid, and umask options
because most kobjects are system wide. I made a fake check of
capable(CAP_SYS_ADMIN), but it needs some other check. In OpenVZ kernel
container's root and real root have different capabilities, but this is
not implemented in upstream kernel AFAIK.
TODO:
- introduce real check for uid/gid/umask
- document mount options
- divide the patch into option parsing and adding actual options
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index ea9120a..ce12dfe 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -325,8 +325,10 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
atomic_set(&sd->s_active, 0);
sd->s_name = name;
- sd->s_mode = mode;
+ sd->s_mode = mode & ~sysfs_perms.umask;
sd->s_flags = type;
+ sd->s_uid = sysfs_perms.uid;
+ sd->s_gid = sysfs_perms.gid;
return sd;
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 0a12eb8..2764d07 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -17,6 +17,7 @@
#include <linux/backing-dev.h>
#include <linux/capability.h>
#include <linux/errno.h>
+#include <linux/mount.h>
#include <linux/sched.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
@@ -217,8 +218,9 @@ static int sysfs_count_nlink(struct sysfs_dirent *sd)
static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode)
{
struct sysfs_inode_attrs *iattrs = sd->s_iattr;
+ struct sysfs_super_info *sb_info = sysfs_info(inode->i_sb);
- inode->i_mode = sd->s_mode;
+ inode->i_mode = sd->s_mode & (sb_info->mode | ~0777);
if (iattrs) {
/* sysfs_dirent has non-default attributes
* get them from persistent copy in sysfs_dirent
@@ -256,6 +258,8 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
inode->i_op = &sysfs_inode_operations;
set_default_inode_attr(inode, sd->s_mode);
+ inode->i_uid = sd->s_uid;
+ inode->i_gid = sd->s_gid;
sysfs_refresh_inode(sd, inode);
/* initialize inode according to type */
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 2668957..c6c337d 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -18,10 +18,14 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/magic.h>
+#include <linux/parser.h>
#include <linux/slab.h>
#include "sysfs.h"
+struct sysfs_perms sysfs_perms;
+
+static int sysfs_remount_fs(struct super_block * sb, int *flags, char *data);
static struct vfsmount *sysfs_mnt;
struct kmem_cache *sysfs_dir_cachep;
@@ -30,6 +34,7 @@ static const struct super_operations sysfs_ops = {
.statfs = simple_statfs,
.drop_inode = generic_delete_inode,
.evict_inode = sysfs_evict_inode,
+ .remount_fs = sysfs_remount_fs,
};
struct sysfs_dirent sysfs_root = {
@@ -40,10 +45,77 @@ struct sysfs_dirent sysfs_root = {
.s_ino = 1,
};
+enum {
+ Opt_uid, Opt_gid, Opt_umask, Opt_mode, Opt_err
+};
+
+static const match_table_t sysfs_tokens = {
+ {Opt_uid, "uid=%u"},
+ {Opt_gid, "gid=%u"},
+ {Opt_umask, "umask=%o"},
+ {Opt_mode, "mode=%o"},
+ {Opt_err, NULL},
+};
+
+static int parse_options(char *options, struct sysfs_super_info *sb_info)
+{
+ char *p;
+ int option;
+ substring_t args[MAX_OPT_ARGS];
+
+ while ((p = strsep(&options, ",")) != NULL) {
+ int token;
+ if (!*p)
+ continue;
+
+ token = match_token(p, sysfs_tokens, args);
+
+ switch (token) {
+ case Opt_uid:
+ case Opt_gid:
+ case Opt_umask:
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ default:
+ break;
+ }
+
+ switch (token) {
+ case Opt_uid:
+ if (match_int(&args[0], &option))
+ return -EINVAL;
+ sysfs_perms.uid = option;
+ break;
+ case Opt_gid:
+ if (match_int(&args[0], &option))
+ return -EINVAL;
+ sysfs_perms.gid = option;
+ break;
+ case Opt_mode:
+ if (match_octal(&args[0], &option))
+ return -EINVAL;
+ sb_info->mode = option;
+ break;
+ case Opt_umask:
+ if (match_octal(&args[0], &option))
+ return -EINVAL;
+ sysfs_perms.umask = option;
+ break;
+ default:
+ pr_err("SYSFS: Unrecognized mount option \"%s\" "
+ "or missing value\n", p);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
{
struct inode *inode;
struct dentry *root;
+ int err;
sb->s_blocksize = PAGE_CACHE_SIZE;
sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
@@ -51,6 +123,10 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_op = &sysfs_ops;
sb->s_time_gran = 1;
+ err = parse_options(data, sysfs_info(sb));
+ if (err)
+ return err;
+
/* get root inode, initialize and unlock it */
mutex_lock(&sysfs_mutex);
inode = sysfs_get_inode(sb, &sysfs_root);
@@ -109,6 +185,7 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
for (type = KOBJ_NS_TYPE_NONE; type < KOBJ_NS_TYPES; type++)
info->ns[type] = kobj_ns_current(type);
+ info->mode = 0777;
sb = sget(fs_type, sysfs_test_super, sysfs_set_super, info);
if (IS_ERR(sb) || sb->s_fs_info != info)
@@ -128,6 +205,11 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
return dget(sb->s_root);
}
+static int sysfs_remount_fs(struct super_block * sb, int *flags, char *data)
+{
+ return parse_options(data, sysfs_info(sb));
+}
+
static void sysfs_kill_sb(struct super_block *sb)
{
struct sysfs_super_info *info = sysfs_info(sb);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 3d28af3..d474f1e 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -69,6 +69,8 @@ struct sysfs_dirent {
unsigned int s_flags;
unsigned short s_mode;
+ uid_t s_uid;
+ gid_t s_gid;
ino_t s_ino;
struct sysfs_inode_attrs *s_iattr;
};
@@ -130,6 +132,13 @@ struct sysfs_addrm_cxt {
* mount.c
*/
+struct sysfs_perms {
+ unsigned int umask;
+ gid_t gid;
+ uid_t uid;
+};
+extern struct sysfs_perms sysfs_perms;
+
/*
* Each sb is associated with a set of namespace tags (i.e.
* the network namespace of the task which mounted this sysfs
@@ -137,6 +146,7 @@ struct sysfs_addrm_cxt {
*/
struct sysfs_super_info {
const void *ns[KOBJ_NS_TYPES];
+ unsigned int mode;
};
#define sysfs_info(SB) ((struct sysfs_super_info *)(SB->s_fs_info))
extern struct sysfs_dirent sysfs_root;
On Wed, May 18, 2011 at 08:31:44PM +0400, Vasiliy Kulikov wrote:
> Currently there is no good way to effectively globally restrict an
> access to sysfs files. It's possible only to chmod the sysfs'
> root/directories to fully deny access to sysfs (sub-)tree to some users
> or chmod files after they are created. The latter approach is racy,
> however.
Why do you want to do this? What is in sysfs files that is not
gloabally ok to access? That should be fixed first, if at all, instead
of wanting to modify the whole sysfs tree, right?
> The patch introduces sysfs mount options parsing and adds 4 new options:
> uid, gid, mode and umask. uid, gid, and umask are classical options,
> mode is a global restricting mode mask that defined the most relaxed
> possible file mode. E.g. if mode=0750 then "chmod 0664" changes file's
> permissions to 0640.
What is going to break if you do this? Have you tested it? I'd be very
worried about this.
Again, what's the root problem you are trying to solve here?
thanks,
greg k-h
On Wed, May 18, 2011 at 09:39 -0700, Greg KH wrote:
> On Wed, May 18, 2011 at 08:31:44PM +0400, Vasiliy Kulikov wrote:
> > Currently there is no good way to effectively globally restrict an
> > access to sysfs files. It's possible only to chmod the sysfs'
> > root/directories to fully deny access to sysfs (sub-)tree to some users
> > or chmod files after they are created. The latter approach is racy,
> > however.
>
> Why do you want to do this? What is in sysfs files that is not
> gloabally ok to access? That should be fixed first, if at all, instead
> of wanting to modify the whole sysfs tree, right?
I don't hide the goal (I didn't find any other weird permissions, if
you mean this). It is primary about additional global controlable
layer of defining permission:
1) *IF* another sensitive file with weird permissions is found, mount
option is IMO the best temporary workaround.
2) Somebody might be worried about information leaks via world readable
files - not strict bugs, but leaks in sense of local policy. See numerous
discussions about hiding kernel addresses - there is no unified opinion
about it. Some admins would be happy with denying access to almost all
system information except some white list.
> > The patch introduces sysfs mount options parsing and adds 4 new options:
> > uid, gid, mode and umask. uid, gid, and umask are classical options,
> > mode is a global restricting mode mask that defined the most relaxed
> > possible file mode. E.g. if mode=0750 then "chmod 0664" changes file's
> > permissions to 0640.
>
> What is going to break if you do this? Have you tested it? I'd be very
> worried about this.
I've tested it on my laptop (I'm writing booted with this kernel). By
default nothing has changed (umask=0000, mode=0777). Mounting sysfs
with mode=0770 leads to the predictable state - no sysfs information is
available to nonroot, e.g. no ACPI battery state. umask is tested with
pluging a USB flash.
mode has one advantage over umask. It is temporary - "mount -o
remount,mode=0777" restores the initial state.
I'd want to implement similar options for debugfs.
Thanks,
--
Vasiliy
On Wed, May 18, 2011 at 09:05:45PM +0400, Vasiliy Kulikov wrote:
> On Wed, May 18, 2011 at 09:39 -0700, Greg KH wrote:
> > On Wed, May 18, 2011 at 08:31:44PM +0400, Vasiliy Kulikov wrote:
> > > Currently there is no good way to effectively globally restrict an
> > > access to sysfs files. It's possible only to chmod the sysfs'
> > > root/directories to fully deny access to sysfs (sub-)tree to some users
> > > or chmod files after they are created. The latter approach is racy,
> > > however.
> >
> > Why do you want to do this? What is in sysfs files that is not
> > gloabally ok to access? That should be fixed first, if at all, instead
> > of wanting to modify the whole sysfs tree, right?
>
> I don't hide the goal (I didn't find any other weird permissions, if
> you mean this). It is primary about additional global controlable
> layer of defining permission:
>
> 1) *IF* another sensitive file with weird permissions is found, mount
> option is IMO the best temporary workaround.
Maybe, but fixing the file would be the obvious solution.
> 2) Somebody might be worried about information leaks via world readable
> files - not strict bugs, but leaks in sense of local policy. See numerous
> discussions about hiding kernel addresses - there is no unified opinion
> about it. Some admins would be happy with denying access to almost all
> system information except some white list.
What file in sysfs "leaks" information like this? Please let us know
and we will be glad to fix that.
> > > The patch introduces sysfs mount options parsing and adds 4 new options:
> > > uid, gid, mode and umask. uid, gid, and umask are classical options,
> > > mode is a global restricting mode mask that defined the most relaxed
> > > possible file mode. E.g. if mode=0750 then "chmod 0664" changes file's
> > > permissions to 0640.
> >
> > What is going to break if you do this? Have you tested it? I'd be very
> > worried about this.
>
> I've tested it on my laptop (I'm writing booted with this kernel). By
> default nothing has changed (umask=0000, mode=0777). Mounting sysfs
> with mode=0770 leads to the predictable state - no sysfs information is
> available to nonroot, e.g. no ACPI battery state. umask is tested with
> pluging a USB flash.
A usb storage device is hardly a huge consumer of sysfs files, sorry.
This would need a whole lot more testing before I would ever feel
confortable with it, even if I agreed that something like this should be
added (hint, I still don't.)
> mode has one advantage over umask. It is temporary - "mount -o
> remount,mode=0777" restores the initial state.
>
> I'd want to implement similar options for debugfs.
I wouldn't.
Again, lets fix the root problems here, if any, instead of adding
complexity and probably breaking systems by changing permissions without
anyone knowing about it.
thanks,
greg k-h
On Wed, May 18, 2011 at 12:17 -0700, Greg KH wrote:
> > 1) *IF* another sensitive file with weird permissions is found, mount
> > option is IMO the best temporary workaround.
>
> Maybe, but fixing the file would be the obvious solution.
I mean for a sysadmin, not for a developer.
What do you mean by "breaking system"? Root is able to chmod
and chown sysfs files already, he may do "chmod -R" or similar.
I suggest sane, race free way to globally restrict permissions *IF* root
wants it.
Here https://lkml.org/lkml/2011/2/25/300 you, not aware of usefull
applications of world-writable debugfs file, agreeded to statically
restrict permissions of all files. I suggest more flexible and
configurable in runtime solution. It doesn't break anything - default
behaviour doesn't differ from current one. What has changed in your
mind since 2/25?
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Thu, May 19, 2011 at 10:26:23AM +0400, Vasiliy Kulikov wrote:
> On Wed, May 18, 2011 at 12:17 -0700, Greg KH wrote:
> > > 1) *IF* another sensitive file with weird permissions is found, mount
> > > option is IMO the best temporary workaround.
> >
> > Maybe, but fixing the file would be the obvious solution.
>
> I mean for a sysadmin, not for a developer.
And I mean for the developer.
We have checks in place now to prevent this type of thing from happening
again in the future. If it does, and it might, we will fix it, it's
that simple.
> What do you mean by "breaking system"? Root is able to chmod
> and chown sysfs files already, he may do "chmod -R" or similar.
> I suggest sane, race free way to globally restrict permissions *IF* root
> wants it.
If root wants it, they can do this today with a simple 1 line bash
command, so I don't see the issue.
> Here https://lkml.org/lkml/2011/2/25/300 you, not aware of usefull
> applications of world-writable debugfs file, agreeded to statically
> restrict permissions of all files. I suggest more flexible and
> configurable in runtime solution. It doesn't break anything - default
> behaviour doesn't differ from current one. What has changed in your
> mind since 2/25?
That's debugfs, not sysfs, which we are talking about here, right?
thanks,
greg k-h
On Thu, May 19, 2011 at 10:12 -0700, Greg KH wrote:
> On Thu, May 19, 2011 at 10:26:23AM +0400, Vasiliy Kulikov wrote:
> > On Wed, May 18, 2011 at 12:17 -0700, Greg KH wrote:
> > > Maybe, but fixing the file would be the obvious solution.
> >
> > I mean for a sysadmin, not for a developer.
>
> And I mean for the developer.
>
> We have checks in place now to prevent this type of thing from happening
> again in the future. If it does, and it might, we will fix it, it's
> that simple.
Simple indeed. But not as fast as simple:
https://lkml.org/lkml/2011/2/4/74
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=49d50fb1c28738ef6bad0c2b87d5355a1653fed5
More than 40 days from the report to the actual commit. Sometimes it
needs some workaround.
> > What do you mean by "breaking system"? Root is able to chmod
> > and chown sysfs files already, he may do "chmod -R" or similar.
> > I suggest sane, race free way to globally restrict permissions *IF* root
> > wants it.
>
> If root wants it, they can do this today with a simple 1 line bash
> command, so I don't see the issue.
The issue is a race condition between the file creation and chmod'ing.
> > Here https://lkml.org/lkml/2011/2/25/300 you, not aware of usefull
> > applications of world-writable debugfs file, agreeded to statically
> > restrict permissions of all files. I suggest more flexible and
> > configurable in runtime solution. It doesn't break anything - default
> > behaviour doesn't differ from current one. What has changed in your
> > mind since 2/25?
>
> That's debugfs, not sysfs, which we are talking about here, right?
Correct. So, if I understood you, you are OK with adding mount options
for debugfs, but not sysfs, right? What is the difference between them
in sense of permissions?
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, May 20, 2011 at 01:59:20PM +0400, Vasiliy Kulikov wrote:
> On Thu, May 19, 2011 at 10:12 -0700, Greg KH wrote:
> > On Thu, May 19, 2011 at 10:26:23AM +0400, Vasiliy Kulikov wrote:
> > > On Wed, May 18, 2011 at 12:17 -0700, Greg KH wrote:
> > > > Maybe, but fixing the file would be the obvious solution.
> > >
> > > I mean for a sysadmin, not for a developer.
> >
> > And I mean for the developer.
> >
> > We have checks in place now to prevent this type of thing from happening
> > again in the future. If it does, and it might, we will fix it, it's
> > that simple.
>
> Simple indeed. But not as fast as simple:
>
> https://lkml.org/lkml/2011/2/4/74
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=49d50fb1c28738ef6bad0c2b87d5355a1653fed5
>
> More than 40 days from the report to the actual commit. Sometimes it
> needs some workaround.
That's sad, but it is for a very rare device, right?
You can always pester us to get those changes in sooner as well.
> > > What do you mean by "breaking system"? Root is able to chmod
> > > and chown sysfs files already, he may do "chmod -R" or similar.
> > > I suggest sane, race free way to globally restrict permissions *IF* root
> > > wants it.
> >
> > If root wants it, they can do this today with a simple 1 line bash
> > command, so I don't see the issue.
>
> The issue is a race condition between the file creation and chmod'ing.
>
> > > Here https://lkml.org/lkml/2011/2/25/300 you, not aware of usefull
> > > applications of world-writable debugfs file, agreeded to statically
> > > restrict permissions of all files. I suggest more flexible and
> > > configurable in runtime solution. It doesn't break anything - default
> > > behaviour doesn't differ from current one. What has changed in your
> > > mind since 2/25?
> >
> > That's debugfs, not sysfs, which we are talking about here, right?
>
> Correct. So, if I understood you, you are OK with adding mount options
> for debugfs, but not sysfs, right? What is the difference between them
> in sense of permissions?
debugfs is "there are no rules", so changing the permissions on it
shouldn't break anything as no userspace tools "should" rely on it. Now
that really isn't true (see the perf stuff), but overall it is, so I
don't worry about changing things there as much as sysfs, which has
hundreds of tools relying on it.
thanks,
greg k-h
On Fri, May 20, 2011 at 06:30 -0700, Greg KH wrote:
> > > > Here https://lkml.org/lkml/2011/2/25/300 you, not aware of usefull
> > > > applications of world-writable debugfs file, agreeded to statically
> > > > restrict permissions of all files. I suggest more flexible and
> > > > configurable in runtime solution. It doesn't break anything - default
> > > > behaviour doesn't differ from current one. What has changed in your
> > > > mind since 2/25?
> > >
> > > That's debugfs, not sysfs, which we are talking about here, right?
> >
> > Correct. So, if I understood you, you are OK with adding mount options
> > for debugfs, but not sysfs, right? What is the difference between them
> > in sense of permissions?
>
> debugfs is "there are no rules", so changing the permissions on it
> shouldn't break anything as no userspace tools "should" rely on it. Now
> that really isn't true (see the perf stuff), but overall it is, so I
> don't worry about changing things there as much as sysfs, which has
> hundreds of tools relying on it.
What would break if the default behaviour is not changed?
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, May 20, 2011 at 17:34 +0400, Vasiliy Kulikov wrote:
> On Fri, May 20, 2011 at 06:30 -0700, Greg KH wrote:
> > > Correct. So, if I understood you, you are OK with adding mount options
> > > for debugfs, but not sysfs, right? What is the difference between them
> > > in sense of permissions?
> >
> > debugfs is "there are no rules", so changing the permissions on it
> > shouldn't break anything as no userspace tools "should" rely on it. Now
> > that really isn't true (see the perf stuff), but overall it is, so I
> > don't worry about changing things there as much as sysfs, which has
> > hundreds of tools relying on it.
>
> What would break if the default behaviour is not changed?
Err... sorry, s/would break/would it break/
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Fri, May 20, 2011 at 05:36:15PM +0400, Vasiliy Kulikov wrote:
> On Fri, May 20, 2011 at 17:34 +0400, Vasiliy Kulikov wrote:
> > On Fri, May 20, 2011 at 06:30 -0700, Greg KH wrote:
> > > > Correct. So, if I understood you, you are OK with adding mount options
> > > > for debugfs, but not sysfs, right? What is the difference between them
> > > > in sense of permissions?
> > >
> > > debugfs is "there are no rules", so changing the permissions on it
> > > shouldn't break anything as no userspace tools "should" rely on it. Now
> > > that really isn't true (see the perf stuff), but overall it is, so I
> > > don't worry about changing things there as much as sysfs, which has
> > > hundreds of tools relying on it.
> >
> > What would break if the default behaviour is not changed?
>
> Err... sorry, s/would break/would it break/
That's the question for you, we would be changing the kernel/user api
here, and odds are, something will break.
thansk,
greg k-h
Hi Greg,
On Fri, May 20, 2011 at 06:54:24AM -0700, Greg KH wrote:
> That's the question for you, we would be changing the kernel/user api
> here, and odds are, something will break.
I feel like this is going in circles. :)
Adding this feature (with no changes to the defaults) will:
- not break anything in the standard case
- allow a system owner to locally choose the DAC perms on the mounts
The question is about providing a temporal buffer between when a flaw is
found in sysfs or debugfs and when the fix for it gets to the system owner.
This, of course, presupposes that flaws are even publicly discovered in the
first place. It gives paranoid system owners the option to lock down sysfs
and debugfs from non-root users while still having them mounted without
needing to make rather non-standard changes to boot time scripts beyond
changing the fstab.
Given that things like tmpfs support these options, it seems only sane to
suppor them here too.
-Kees
--
Kees Cook
Ubuntu Security Team