[PATCH 0/2] fs: set root dir perms

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] fs: set root dir perms

Kees Cook-8
With the continuing deluge of bugs in the "debug" filesystem, I would
like to make that filesystem's root directory mode 0700 by default since
it's filled with crazy stuff that regular users do not need to see.

Better to try to just close the door completely on all the stuff in there.
It is, after all, supposed to only be used for debugging, right?


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] fs: pass root inode mode to simple_fill_super

Kees Cook-8
There was no way to specify the mode of the root directory of filesystems
created with simple_fill_super.

Signed-off-by: Kees Cook <[hidden email]>
---
 drivers/infiniband/hw/ipath/ipath_fs.c |    3 ++-
 drivers/infiniband/hw/qib/qib_fs.c     |    3 ++-
 drivers/xen/xenfs/super.c              |    3 ++-
 fs/binfmt_misc.c                       |    3 ++-
 fs/debugfs/inode.c                     |    3 ++-
 fs/fuse/control.c                      |    3 ++-
 fs/libfs.c                             |    4 ++--
 fs/nfsd/nfsctl.c                       |    3 ++-
 include/linux/fs.h                     |    3 ++-
 security/inode.c                       |    3 ++-
 security/selinux/selinuxfs.c           |    3 ++-
 security/smack/smackfs.c               |    3 ++-
 12 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/ipath/ipath_fs.c b/drivers/infiniband/hw/ipath/ipath_fs.c
index 31ae1b1..991aa4f 100644
--- a/drivers/infiniband/hw/ipath/ipath_fs.c
+++ b/drivers/infiniband/hw/ipath/ipath_fs.c
@@ -336,7 +336,8 @@ static int ipathfs_fill_super(struct super_block *sb, void *data,
  {""},
  };
 
- ret = simple_fill_super(sb, IPATHFS_MAGIC, files);
+ ret = simple_fill_super(sb, IPATHFS_MAGIC, files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
  if (ret) {
  printk(KERN_ERR "simple_fill_super failed: %d\n", ret);
  goto bail;
diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c
index df7fa25..de01b23 100644
--- a/drivers/infiniband/hw/qib/qib_fs.c
+++ b/drivers/infiniband/hw/qib/qib_fs.c
@@ -530,7 +530,8 @@ static int qibfs_fill_super(struct super_block *sb, void *data, int silent)
  {""},
  };
 
- ret = simple_fill_super(sb, QIBFS_MAGIC, files);
+ ret = simple_fill_super(sb, QIBFS_MAGIC, files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
  if (ret) {
  printk(KERN_ERR "simple_fill_super failed: %d\n", ret);
  goto bail;
diff --git a/drivers/xen/xenfs/super.c b/drivers/xen/xenfs/super.c
index 1aa3897..d5d65cf 100644
--- a/drivers/xen/xenfs/super.c
+++ b/drivers/xen/xenfs/super.c
@@ -89,7 +89,8 @@ static int xenfs_fill_super(struct super_block *sb, void *data, int silent)
  };
  int rc;
 
- rc = simple_fill_super(sb, XENFS_SUPER_MAGIC, xenfs_files);
+ rc = simple_fill_super(sb, XENFS_SUPER_MAGIC, xenfs_files,
+       S_IWUSR | S_IRUGO | S_IXUGO);
  if (rc < 0)
  return rc;
 
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 1befe2e..6ad4874 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -700,7 +700,8 @@ static int bm_fill_super(struct super_block * sb, void * data, int silent)
  [3] = {"register", &bm_register_operations, S_IWUSR},
  /* last one */ {""}
  };
- int err = simple_fill_super(sb, 0x42494e4d, bm_files);
+ int err = simple_fill_super(sb, 0x42494e4d, bm_files,
+    S_IWUSR | S_IRUGO | S_IXUGO);
  if (!err)
  sb->s_op = &s_ops;
  return err;
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 37a8ca7..3cb33c3 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -132,7 +132,8 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
 {
  static struct tree_descr debug_files[] = {{""}};
 
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
 }
 
 static struct dentry *debug_mount(struct file_system_type *fs_type,
diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 85542a7..80bbb66 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -302,7 +302,8 @@ static int fuse_ctl_fill_super(struct super_block *sb, void *data, int silent)
  struct fuse_conn *fc;
  int err;
 
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr,
+ S_IWUSR | S_IRUGO | S_IXUGO);
  if (err)
  return err;
 
diff --git a/fs/libfs.c b/fs/libfs.c
index c88eab5..ea4d695 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -463,7 +463,7 @@ int simple_write_end(struct file *file, struct address_space *mapping,
  * to pass it an appropriate max_reserved value to avoid collisions.
  */
 int simple_fill_super(struct super_block *s, unsigned long magic,
-      struct tree_descr *files)
+      struct tree_descr *files, umode_t mode)
 {
  struct inode *inode;
  struct dentry *root;
@@ -484,7 +484,7 @@ int simple_fill_super(struct super_block *s, unsigned long magic,
  * entry at index 1
  */
  inode->i_ino = 1;
- inode->i_mode = S_IFDIR | 0755;
+ inode->i_mode = (mode & (S_IRWXUGO | S_ISVTX)) | S_IFDIR;
  inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
  inode->i_op = &simple_dir_inode_operations;
  inode->i_fop = &simple_dir_operations;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 33b3e2b..709ca56 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1404,7 +1404,8 @@ static int nfsd_fill_super(struct super_block * sb, void * data, int silent)
 #endif
  /* last one */ {""}
  };
- return simple_fill_super(sb, 0x6e667364, nfsd_files);
+ return simple_fill_super(sb, 0x6e667364, nfsd_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
 }
 
 static struct dentry *nfsd_mount(struct file_system_type *fs_type,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bd32159..d4dd31e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2435,7 +2435,8 @@ extern const struct file_operations simple_dir_operations;
 extern const struct inode_operations simple_dir_inode_operations;
 struct tree_descr { char *name; const struct file_operations *ops; int mode; };
 struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, unsigned long, struct tree_descr *);
+extern int simple_fill_super(struct super_block *, unsigned long,
+     struct tree_descr *, umode_t mode);
 extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
 extern void simple_release_fs(struct vfsmount **mount, int *count);
 
diff --git a/security/inode.c b/security/inode.c
index c4df2fb..d85e416 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -128,7 +128,8 @@ static int fill_super(struct super_block *sb, void *data, int silent)
 {
  static struct tree_descr files[] = {{""}};
 
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return simple_fill_super(sb, SECURITYFS_MAGIC, files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
 }
 
 static struct dentry *get_sb(struct file_system_type *fs_type,
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index ea39cb7..26f9c025 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1792,7 +1792,8 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
  [SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUSR},
  /* last one */ {""}
  };
- ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
+ ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files,
+ S_IWUSR | S_IRUGO | S_IXUGO);
  if (ret)
  goto err;
 
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 362d5ed..788fac4 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -1323,7 +1323,8 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent)
  /* last one */ {""}
  };
 
- rc = simple_fill_super(sb, SMACK_MAGIC, smack_files);
+ rc = simple_fill_super(sb, SMACK_MAGIC, smack_files,
+       S_IWUSR | S_IRUGO | S_IXUGO);
  if (rc != 0) {
  printk(KERN_ERR "%s failed %d while creating inodes\n",
  __func__, rc);
--
1.7.2.3


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] debugfs: only allow root access to debugging interfaces

Kees Cook-8
In reply to this post by Kees Cook-8
Block access to the potentially dangerous debugging interfaces in
the debugfs filesystem.

Signed-off-by: Kees Cook <[hidden email]>
---
 fs/debugfs/inode.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 3cb33c3..83c61a3 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -133,7 +133,7 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
  static struct tree_descr debug_files[] = {{""}};
 
  return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files,
- S_IWUSR | S_IRUGO | S_IXUGO);
+ S_IRWXU);
 }
 
 static struct dentry *debug_mount(struct file_system_type *fs_type,
--
1.7.2.3


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Tim Gardner-2
In reply to this post by Kees Cook-8
On 02/22/2011 11:28 AM, Kees Cook wrote:
> With the continuing deluge of bugs in the "debug" filesystem, I would
> like to make that filesystem's root directory mode 0700 by default since
> it's filled with crazy stuff that regular users do not need to see.
>
> Better to try to just close the door completely on all the stuff in there.
> It is, after all, supposed to only be used for debugging, right?
>
>

On the surface this doesn't look too bad. However, I'd kind of like to
let it cook upstream for awhile. Your email on LKML has a fairly wide
distribution, so the responses ought to be interesting.

rtg
--
Tim Gardner [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Kees Cook-8
Hi Tim,

On Tue, Feb 22, 2011 at 12:02:16PM -0700, Tim Gardner wrote:

> On 02/22/2011 11:28 AM, Kees Cook wrote:
> >With the continuing deluge of bugs in the "debug" filesystem, I would
> >like to make that filesystem's root directory mode 0700 by default since
> >it's filled with crazy stuff that regular users do not need to see.
> >
> >Better to try to just close the door completely on all the stuff in there.
> >It is, after all, supposed to only be used for debugging, right?
> >
> >
>
> On the surface this doesn't look too bad. However, I'd kind of like
> to let it cook upstream for awhile. Your email on LKML has a fairly
> wide distribution, so the responses ought to be interesting.

Oh, er, I thought it was best to get it into Natty ASAP so that we could
shake out any obvious glitches it causes. That was the impression apw gave
me, anyway.

-Kees

--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Tim Gardner-2
On 02/22/2011 12:17 PM, Kees Cook wrote:

> Hi Tim,
>
> On Tue, Feb 22, 2011 at 12:02:16PM -0700, Tim Gardner wrote:
>> On 02/22/2011 11:28 AM, Kees Cook wrote:
>>> With the continuing deluge of bugs in the "debug" filesystem, I would
>>> like to make that filesystem's root directory mode 0700 by default since
>>> it's filled with crazy stuff that regular users do not need to see.
>>>
>>> Better to try to just close the door completely on all the stuff in there.
>>> It is, after all, supposed to only be used for debugging, right?
>>>
>>>
>>
>> On the surface this doesn't look too bad. However, I'd kind of like
>> to let it cook upstream for awhile. Your email on LKML has a fairly
>> wide distribution, so the responses ought to be interesting.
>
> Oh, er, I thought it was best to get it into Natty ASAP so that we could
> shake out any obvious glitches it causes. That was the impression apw gave
> me, anyway.
>
> -Kees
>

Perhaps, while some of this is shaking out upstream, we ought to take a
closer look at not leaving debugfs mounted, e.g., umount it after
ureadahead is done. Anyone using ftrace is likely savvy enough to know
how to mount debugfs when they need it.

rtg
--
Tim Gardner [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Kees Cook-8
On Tue, Feb 22, 2011 at 12:50:43PM -0700, Tim Gardner wrote:

> On 02/22/2011 12:17 PM, Kees Cook wrote:
> >Hi Tim,
> >
> >On Tue, Feb 22, 2011 at 12:02:16PM -0700, Tim Gardner wrote:
> >>On 02/22/2011 11:28 AM, Kees Cook wrote:
> >>>With the continuing deluge of bugs in the "debug" filesystem, I would
> >>>like to make that filesystem's root directory mode 0700 by default since
> >>>it's filled with crazy stuff that regular users do not need to see.
> >>>
> >>>Better to try to just close the door completely on all the stuff in there.
> >>>It is, after all, supposed to only be used for debugging, right?
> >>>
> >>>
> >>
> >>On the surface this doesn't look too bad. However, I'd kind of like
> >>to let it cook upstream for awhile. Your email on LKML has a fairly
> >>wide distribution, so the responses ought to be interesting.
> >
> >Oh, er, I thought it was best to get it into Natty ASAP so that we could
> >shake out any obvious glitches it causes. That was the impression apw gave
> >me, anyway.
> >
> >-Kees
> >
>
> Perhaps, while some of this is shaking out upstream, we ought to
> take a closer look at not leaving debugfs mounted, e.g., umount it
> after ureadahead is done. Anyone using ftrace is likely savvy enough
> to know how to mount debugfs when they need it.

I think ureadahead already uses a private copy of debugfs in
/var/lib/ureadahead/debugfs. I think we should just not mount debugfs at
all (though we still need to keep acpi/custom_method commented out at least
until this[1] is taken).

-Kees

[1] https://lkml.org/lkml/2011/2/22/369

--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Tim Gardner-2
On 02/22/2011 12:58 PM, Kees Cook wrote:

> On Tue, Feb 22, 2011 at 12:50:43PM -0700, Tim Gardner wrote:
>> On 02/22/2011 12:17 PM, Kees Cook wrote:
>>> Hi Tim,
>>>
>>> On Tue, Feb 22, 2011 at 12:02:16PM -0700, Tim Gardner wrote:
>>>> On 02/22/2011 11:28 AM, Kees Cook wrote:
>>>>> With the continuing deluge of bugs in the "debug" filesystem, I would
>>>>> like to make that filesystem's root directory mode 0700 by default since
>>>>> it's filled with crazy stuff that regular users do not need to see.
>>>>>
>>>>> Better to try to just close the door completely on all the stuff in there.
>>>>> It is, after all, supposed to only be used for debugging, right?
>>>>>
>>>>>
>>>>
>>>> On the surface this doesn't look too bad. However, I'd kind of like
>>>> to let it cook upstream for awhile. Your email on LKML has a fairly
>>>> wide distribution, so the responses ought to be interesting.
>>>
>>> Oh, er, I thought it was best to get it into Natty ASAP so that we could
>>> shake out any obvious glitches it causes. That was the impression apw gave
>>> me, anyway.
>>>
>>> -Kees
>>>
>>
>> Perhaps, while some of this is shaking out upstream, we ought to
>> take a closer look at not leaving debugfs mounted, e.g., umount it
>> after ureadahead is done. Anyone using ftrace is likely savvy enough
>> to know how to mount debugfs when they need it.
>
> I think ureadahead already uses a private copy of debugfs in
> /var/lib/ureadahead/debugfs. I think we should just not mount debugfs at
> all (though we still need to keep acpi/custom_method commented out at least
> until this[1] is taken).
>
> -Kees
>
> [1] https://lkml.org/lkml/2011/2/22/369
>

It appears that ureadahead only uses /var/lib/ureadahead/debugfs if
/sys/kernel/debug is not already mounted, so we need to test that code path.

What package mounts debugfs ?

rtg
--
Tim Gardner [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Kees Cook-8
On Tue, Feb 22, 2011 at 01:23:57PM -0700, Tim Gardner wrote:
> It appears that ureadahead only uses /var/lib/ureadahead/debugfs if
> /sys/kernel/debug is not already mounted, so we need to test that
> code path.

I've confirmed this path -- ureadahead uses it on my system every time.

> What package mounts debugfs ?

mountall. I'm happy to patch it to not mount /sys/kernel/debug by default.

-Kees

--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Tim Gardner-2
On 02/22/2011 01:29 PM, Kees Cook wrote:

> On Tue, Feb 22, 2011 at 01:23:57PM -0700, Tim Gardner wrote:
>> It appears that ureadahead only uses /var/lib/ureadahead/debugfs if
>> /sys/kernel/debug is not already mounted, so we need to test that
>> code path.
>
> I've confirmed this path -- ureadahead uses it on my system every time.
>
>> What package mounts debugfs ?
>
> mountall. I'm happy to patch it to not mount /sys/kernel/debug by default.
>
> -Kees
>
This is what I've tested on a desktop and server. Everything appears to
work. The only window of vulnerability is while ureadahead is doing its
thing, and that should only happen after the package database changes,
right?

If you concur, then turn off debugfs and see what carnage ensues. You
should probably start a tracking bug to collect any regressions.

rtg
--
Tim Gardner [hidden email]

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

mountall.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Kees Cook-8
On Tue, Feb 22, 2011 at 02:01:09PM -0700, Tim Gardner wrote:

> On 02/22/2011 01:29 PM, Kees Cook wrote:
> >On Tue, Feb 22, 2011 at 01:23:57PM -0700, Tim Gardner wrote:
> >>It appears that ureadahead only uses /var/lib/ureadahead/debugfs if
> >>/sys/kernel/debug is not already mounted, so we need to test that
> >>code path.
> >
> >I've confirmed this path -- ureadahead uses it on my system every time.
> >
> >>What package mounts debugfs ?
> >
> >mountall. I'm happy to patch it to not mount /sys/kernel/debug by default.
> >
> >-Kees
> >
>
> This is what I've tested on a desktop and server. Everything appears
> to work. The only window of vulnerability is while ureadahead is
> doing its thing, and that should only happen after the package
> database changes, right?
>
> If you concur, then turn off debugfs and see what carnage ensues.
> You should probably start a tracking bug to collect any regressions.

Yeah, I already had the upload ready, so I'll use my version (it refers to
the lkml email where Alan Cox says it should not be used on production
systems). But yeah, I'll upload and send email to ubuntu-devel with the
list of everything in main that references /sys/kernel/debug.

-Kees

--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] fs: set root dir perms

Kees Cook-8
In reply to this post by Tim Gardner-2
On Tue, Feb 22, 2011 at 02:01:09PM -0700, Tim Gardner wrote:
> This is what I've tested on a desktop and server. Everything appears
> to work. The only window of vulnerability is while ureadahead is
> doing its thing, and that should only happen after the package
> database changes, right?
>
> If you concur, then turn off debugfs and see what carnage ensues.
> You should probably start a tracking bug to collect any regressions.

Actually, I'm going to change this a bit... I'm going to just chmod it
after mounting. Then I don't have to break apport and ftrace, and I don't
have to carry a kernel patch.

--
Kees Cook
Ubuntu Security Team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team