Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Amir Goldstein]

On Thu, Jul 16, 2015 at 5:47 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
>
>> Initially this will be used to eliminate the implicit MNT_NODEV
>> flag for mounts from user namespaces. In the future it will also
>> be used for translating ids and checking capabilities for
>> filesystems mounted from user namespaces.
>>
>> s_user_ns is initialized in alloc_super() and is generally set to
>> current_user_ns(). To avoid security and corruption issues, two
>> additional mount checks are also added:
>>
>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>>    in current_user_ns().
>>
>>  - sget() will fail with EBUSY when the filesystem it's looking
>>    for is already mounted from another user namespace.
>>
>> proc needs some special handling here. The user namespace of
>> current isn't appropriate when forking as a result of clone (2)
>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
>> from within the new user namespace. Instead, the user namespace
>> which owns the new pid namespace should be used. sget_userns() is
>> added to allow passing of a user namespace other than that of
>> current, and this is used by proc_mount(). sget() becomes a
>> wrapper around sget_userns() which passes current_user_ns().
>
> From bits of the previous conversation.
>
> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> xattrs can travel from one mount of sysfs to another via the sysfs
> backing store.
>
> For tmpfs and any other filesystems we support mounting without
> privilige that support xattrs.  We need to identify them and
> see if userspace is taking advantage of the ability to set
> xattrs and file caps (unlikely).  If they are we need to call
> sget_userns(..., &init_user_ns) on those filesystems as well.
>
> Possibly/Probably we should just do that for all of the interesting
> filesystems to start with and then change back to an ordinary old sget
> after we have done the testing and confirmed we will not be introducing
> userspace regressions.

Eric,

Perhaps it is too soon to discuss here, but how do you envision
handling of file system private mount options in user ns.

For example, suppose that we get to a point where we can trust
an ext4 loopback mount to be non vulnerable to exploits.
That loopback mounted fs could very well have errors and so
error=panic option would be very much undesired from unprivileged user mount.

Do you think this would require extra flags/callbacks from VFS to
file system code or would s_user_ns be sufficient?

>
> Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Eric W. Biederman]

Amir Goldstein <amir@cellrox.com> writes:

> On Thu, Jul 16, 2015 at 5:47 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Seth Forshee <seth.forshee@canonical.com> writes:
>>
>>> Initially this will be used to eliminate the implicit MNT_NODEV
>>> flag for mounts from user namespaces. In the future it will also
>>> be used for translating ids and checking capabilities for
>>> filesystems mounted from user namespaces.
>>>
>>> s_user_ns is initialized in alloc_super() and is generally set to
>>> current_user_ns(). To avoid security and corruption issues, two
>>> additional mount checks are also added:
>>>
>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>>>    in current_user_ns().
>>>
>>>  - sget() will fail with EBUSY when the filesystem it's looking
>>>    for is already mounted from another user namespace.
>>>
>>> proc needs some special handling here. The user namespace of
>>> current isn't appropriate when forking as a result of clone (2)
>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
>>> from within the new user namespace. Instead, the user namespace
>>> which owns the new pid namespace should be used. sget_userns() is
>>> added to allow passing of a user namespace other than that of
>>> current, and this is used by proc_mount(). sget() becomes a
>>> wrapper around sget_userns() which passes current_user_ns().
>>
>> From bits of the previous conversation.
>>
>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
>> xattrs can travel from one mount of sysfs to another via the sysfs
>> backing store.
>>
>> For tmpfs and any other filesystems we support mounting without
>> privilige that support xattrs.  We need to identify them and
>> see if userspace is taking advantage of the ability to set
>> xattrs and file caps (unlikely).  If they are we need to call
>> sget_userns(..., &init_user_ns) on those filesystems as well.
>>
>> Possibly/Probably we should just do that for all of the interesting
>> filesystems to start with and then change back to an ordinary old sget
>> after we have done the testing and confirmed we will not be introducing
>> userspace regressions.
>
> Eric,
>
> Perhaps it is too soon to discuss here, but how do you envision
> handling of file system private mount options in user ns.
>
> For example, suppose that we get to a point where we can trust
> an ext4 loopback mount to be non vulnerable to exploits.
> That loopback mounted fs could very well have errors and so
> error=panic option would be very much undesired from unprivileged user mount.
>
> Do you think this would require extra flags/callbacks from VFS to
> file system code or would s_user_ns be sufficient?

This case is easy.  In mount or remount we just need to check
capable(CAP_SYS_ADMIN) if someone sets error=panic, and if the capable
call fails don't allow the mount or the remount.

But this corner case is another good reminder that we have to be very
deliberate and very careful before we enable mounting a filesystem this
way.

Eric

[PATCH 0/7] Initial support for user namespace owned mounts

[Seth Forshee]

These are the first in a larger set of patches that I've been working on
(with help from Eric Biederman) to support mounting ext4 and fuse
filesystems from within user namespaces. I've pushed the full series to:

  git://kernel.ubuntu.com/sforshee/linux.git userns-mounts

Taking the series as a whole, the strategy is to handle as much of the
heavy lifting as possible in the vfs so the filesystems don't have to
handle weird edge cases. If you look at the full series you'll find that
the changes in ext4 to support user namespace mounts turn out to be
fairly minimal (fuse is a bit more complicated though as it must deal
with translating ids for a userspace process which is running in pid and
user namespaces).

The patches I'm sending today lay some of the groundwork in the vfs and
related code. They fall into two broad groups:

 1. Patches 1-2 add s_user_ns and simplify MNT_NODEV handling. These are
    pretty straightforward, and Eric has expressed interest in merging
    these patches soon. Note that patch 2 won't apply cleanly without
    Eric's noexec patches for proc and sys [1].

 2. Patches 2-7 tighten down security for mounts with s_user_ns !=
    &init_user_ns. This includes updates to how file caps and suid are
    handled and LSM updates to ignore security labels on superblocks
    from non-init namespaces.

    The LSM changes in particular may not be optimal, as I don't have a
    lot of familiarity with this code, so I'd be especially appreciative
    of review of these changes and suggestions on how to improve them.

Subsequent patches will update the vfs for id translation, handling
various corner cases, giving privileges to the user namsepace which owns
a superblock, and finally supporting user namespace mounts for ext4 and
fuse.

Thanks,
Seth

[1] http://lkml.kernel.org/r/87mvz4yomp.fsf_-_@x220.int.ebiederm.org


Andy Lutomirski (1):
  fs: Treat foreign mounts as nosuid

Eric W. Biederman (1):
  userns: Simpilify MNT_NODEV handling.

Seth Forshee (5):
  fs: Add user namesapace member to struct super_block
  fs: Ignore file caps in mounts from other user namespaces
  security: Restrict security attribute updates for userns mounts
  selinux: Ignore security labels on user namespace mounts
  smack: Don't use security labels for user namespace mounts

 fs/block_dev.c                 |  2 +-
 fs/exec.c                      |  2 +-
 fs/namei.c                     |  9 ++++++++-
 fs/namespace.c                 | 34 ++++++++++++++++++++--------------
 fs/proc/root.c                 |  3 ++-
 fs/super.c                     | 38 +++++++++++++++++++++++++++++++++-----
 include/linux/fs.h             |  9 +++++++++
 include/linux/mount.h          |  1 +
 include/linux/user_namespace.h |  8 ++++++++
 kernel/user_namespace.c        | 14 ++++++++++++++
 security/commoncap.c           |  4 +++-
 security/security.c            | 10 +++++++++-
 security/selinux/hooks.c       | 16 +++++++++++++++-
 security/smack/smack_lsm.c     | 12 ++++++++++--
 14 files changed, 134 insertions(+), 28 deletions(-)

[PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

Initially this will be used to eliminate the implicit MNT_NODEV
flag for mounts from user namespaces. In the future it will also
be used for translating ids and checking capabilities for
filesystems mounted from user namespaces.

s_user_ns is initialized in alloc_super() and is generally set to
current_user_ns(). To avoid security and corruption issues, two
additional mount checks are also added:

 - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
   in current_user_ns().

 - sget() will fail with EBUSY when the filesystem it's looking
   for is already mounted from another user namespace.

proc needs some special handling here. The user namespace of
current isn't appropriate when forking as a result of clone (2)
with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
from within the new user namespace. Instead, the user namespace
which owns the new pid namespace should be used. sget_userns() is
added to allow passing of a user namespace other than that of
current, and this is used by proc_mount(). sget() becomes a
wrapper around sget_userns() which passes current_user_ns().

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 fs/namespace.c     |  3 +++
 fs/proc/root.c     |  3 ++-
 fs/super.c         | 38 +++++++++++++++++++++++++++++++++-----
 include/linux/fs.h |  8 ++++++++
 4 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index ce428cadd41f..f1f67d663d49 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2357,6 +2357,9 @@ static int do_new_mount(struct path *path, const char *fstype, int flags,
 	struct vfsmount *mnt;
 	int err;
 
+	if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN))
+		return -EPERM;
+
 	if (!fstype)
 		return -EINVAL;
 
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 361ab4ee42fc..4b302cbf13f9 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -117,7 +117,8 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
 			return ERR_PTR(-EPERM);
 	}
 
-	sb = sget(fs_type, proc_test_super, proc_set_super, flags, ns);
+	sb = sget_userns(fs_type, proc_test_super, proc_set_super, flags,
+			 ns->user_ns, ns);
 	if (IS_ERR(sb))
 		return ERR_CAST(sb);
 
diff --git a/fs/super.c b/fs/super.c
index b61372354f2b..b5f171aadbf7 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -33,6 +33,7 @@
 #include <linux/cleancache.h>
 #include <linux/fsnotify.h>
 #include <linux/lockdep.h>
+#include <linux/user_namespace.h>
 #include "internal.h"
 
 
@@ -148,6 +149,7 @@ static void destroy_super(struct super_block *s)
 	list_lru_destroy(&s->s_inode_lru);
 	for (i = 0; i < SB_FREEZE_LEVELS; i++)
 		percpu_counter_destroy(&s->s_writers.counter[i]);
+	put_user_ns(s->s_user_ns);
 	security_sb_free(s);
 	WARN_ON(!list_empty(&s->s_mounts));
 	kfree(s->s_subtype);
@@ -163,7 +165,8 @@ static void destroy_super(struct super_block *s)
  *	Allocates and initializes a new &struct super_block.  alloc_super()
  *	returns a pointer new superblock or %NULL if allocation had failed.
  */
-static struct super_block *alloc_super(struct file_system_type *type, int flags)
+static struct super_block *alloc_super(struct file_system_type *type, int flags,
+				       struct user_namespace *user_ns)
 {
 	struct super_block *s = kzalloc(sizeof(struct super_block),  GFP_USER);
 	static const struct super_operations default_op;
@@ -231,6 +234,8 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags)
 	s->s_shrink.count_objects = super_cache_count;
 	s->s_shrink.batch = 1024;
 	s->s_shrink.flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE;
+
+	s->s_user_ns = get_user_ns(user_ns);
 	return s;
 
 fail:
@@ -427,17 +432,17 @@ void generic_shutdown_super(struct super_block *sb)
 EXPORT_SYMBOL(generic_shutdown_super);
 
 /**
- *	sget	-	find or create a superblock
+ *	sget_userns -	find or create a superblock
  *	@type:	filesystem type superblock should belong to
  *	@test:	comparison callback
  *	@set:	setup callback
  *	@flags:	mount flags
  *	@data:	argument to each of them
  */
-struct super_block *sget(struct file_system_type *type,
+struct super_block *sget_userns(struct file_system_type *type,
 			int (*test)(struct super_block *,void *),
 			int (*set)(struct super_block *,void *),
-			int flags,
+			int flags, struct user_namespace *user_ns,
 			void *data)
 {
 	struct super_block *s = NULL;
@@ -450,6 +455,10 @@ retry:
 		hlist_for_each_entry(old, &type->fs_supers, s_instances) {
 			if (!test(old, data))
 				continue;
+			if (user_ns != old->s_user_ns) {
+				spin_unlock(&sb_lock);
+				return ERR_PTR(-EBUSY);
+			}
 			if (!grab_super(old))
 				goto retry;
 			if (s) {
@@ -462,7 +471,7 @@ retry:
 	}
 	if (!s) {
 		spin_unlock(&sb_lock);
-		s = alloc_super(type, flags);
+		s = alloc_super(type, flags, user_ns);
 		if (!s)
 			return ERR_PTR(-ENOMEM);
 		goto retry;
@@ -485,6 +494,25 @@ retry:
 	return s;
 }
 
+EXPORT_SYMBOL(sget_userns);
+
+/**
+ *	sget	-	find or create a superblock
+ *	@type:	  filesystem type superblock should belong to
+ *	@test:	  comparison callback
+ *	@set:	  setup callback
+ *	@flags:	  mount flags
+ *	@data:	  argument to each of them
+ */
+struct super_block *sget(struct file_system_type *type,
+			int (*test)(struct super_block *,void *),
+			int (*set)(struct super_block *,void *),
+			int flags,
+			void *data)
+{
+	return sget_userns(type, test, set, flags, current_user_ns(), data);
+}
+
 EXPORT_SYMBOL(sget);
 
 void drop_super(struct super_block *sb)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 42912f8d286e..1876477ac9f8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -30,6 +30,7 @@
 #include <linux/lockdep.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/blk_types.h>
+#include <linux/user_namespace.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -1353,6 +1354,8 @@ struct super_block {
 	struct workqueue_struct *s_dio_done_wq;
 	struct hlist_head s_pins;
 
+	struct user_namespace *s_user_ns;
+
 	/*
 	 * Keep the lru lists last in the structure so they always sit on their
 	 * own individual cachelines.
@@ -1959,6 +1962,11 @@ void deactivate_locked_super(struct super_block *sb);
 int set_anon_super(struct super_block *s, void *data);
 int get_anon_bdev(dev_t *);
 void free_anon_bdev(dev_t);
+struct super_block *sget_userns(struct file_system_type *type,
+			int (*test)(struct super_block *,void *),
+			int (*set)(struct super_block *,void *),
+			int flags, struct user_namespace *user_ns,
+			void *data);
 struct super_block *sget(struct file_system_type *type,
 			int (*test)(struct super_block *,void *),
 			int (*set)(struct super_block *,void *),
-- 
1.9.1

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> Initially this will be used to eliminate the implicit MNT_NODEV
> flag for mounts from user namespaces. In the future it will also
> be used for translating ids and checking capabilities for
> filesystems mounted from user namespaces.
>
> s_user_ns is initialized in alloc_super() and is generally set to
> current_user_ns(). To avoid security and corruption issues, two
> additional mount checks are also added:
>
>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>    in current_user_ns().
>
>  - sget() will fail with EBUSY when the filesystem it's looking
>    for is already mounted from another user namespace.
>
> proc needs some special handling here. The user namespace of
> current isn't appropriate when forking as a result of clone (2)
> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> from within the new user namespace. Instead, the user namespace
> which owns the new pid namespace should be used. sget_userns() is
> added to allow passing of a user namespace other than that of
> current, and this is used by proc_mount(). sget() becomes a
> wrapper around sget_userns() which passes current_user_ns().

>From bits of the previous conversation.

We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
xattrs can travel from one mount of sysfs to another via the sysfs
backing store.

For tmpfs and any other filesystems we support mounting without
privilige that support xattrs.  We need to identify them and
see if userspace is taking advantage of the ability to set
xattrs and file caps (unlikely).  If they are we need to call
sget_userns(..., &init_user_ns) on those filesystems as well.

Possibly/Probably we should just do that for all of the interesting
filesystems to start with and then change back to an ordinary old sget
after we have done the testing and confirmed we will not be introducing
userspace regressions.

Eric

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > Initially this will be used to eliminate the implicit MNT_NODEV
> > flag for mounts from user namespaces. In the future it will also
> > be used for translating ids and checking capabilities for
> > filesystems mounted from user namespaces.
> >
> > s_user_ns is initialized in alloc_super() and is generally set to
> > current_user_ns(). To avoid security and corruption issues, two
> > additional mount checks are also added:
> >
> >  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
> >    in current_user_ns().
> >
> >  - sget() will fail with EBUSY when the filesystem it's looking
> >    for is already mounted from another user namespace.
> >
> > proc needs some special handling here. The user namespace of
> > current isn't appropriate when forking as a result of clone (2)
> > with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> > from within the new user namespace. Instead, the user namespace
> > which owns the new pid namespace should be used. sget_userns() is
> > added to allow passing of a user namespace other than that of
> > current, and this is used by proc_mount(). sget() becomes a
> > wrapper around sget_userns() which passes current_user_ns().
> 
> From bits of the previous conversation.
> 
> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> xattrs can travel from one mount of sysfs to another via the sysfs
> backing store.
> 
> For tmpfs and any other filesystems we support mounting without
> privilige that support xattrs.  We need to identify them and
> see if userspace is taking advantage of the ability to set
> xattrs and file caps (unlikely).  If they are we need to call
> sget_userns(..., &init_user_ns) on those filesystems as well.
> 
> Possibly/Probably we should just do that for all of the interesting
> filesystems to start with and then change back to an ordinary old sget
> after we have done the testing and confirmed we will not be introducing
> userspace regressions.

I was reviewing everything in preparation for sending v2 patches, and I
realized that doing this has an undesirable side effect. In patch 2 the
implicit nodev is removed for unprivileged mounts, and instead s_user_ns
is used to block opening devices in these mounts. When we set s_user_ns
to &init_user_ns, it becomes possible to open device nodes from
unprivileged mounts of these filesystems.

This doesn't pose a real problem today. The only filesystems it will
affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
&init_user_ns for user namespace mounts), and all of these aren't
problems. sysfs is okay because kernfs doesn't (currently?) allow device
nodes, and a user would require CAP_MKNOD to create any device nodes in
a tmpfs or ramfs mount.

But for sysfs in particular it does mean that we will need to make sure
that there's no way that device nodes could start appearing in an
unprivileged mount.

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Eric W. Biederman]

Seth Forshee <seth.forshee@canonical.com> writes:

> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
>> Seth Forshee <seth.forshee@canonical.com> writes:
>> 
>> > Initially this will be used to eliminate the implicit MNT_NODEV
>> > flag for mounts from user namespaces. In the future it will also
>> > be used for translating ids and checking capabilities for
>> > filesystems mounted from user namespaces.
>> >
>> > s_user_ns is initialized in alloc_super() and is generally set to
>> > current_user_ns(). To avoid security and corruption issues, two
>> > additional mount checks are also added:
>> >
>> >  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>> >    in current_user_ns().
>> >
>> >  - sget() will fail with EBUSY when the filesystem it's looking
>> >    for is already mounted from another user namespace.
>> >
>> > proc needs some special handling here. The user namespace of
>> > current isn't appropriate when forking as a result of clone (2)
>> > with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
>> > from within the new user namespace. Instead, the user namespace
>> > which owns the new pid namespace should be used. sget_userns() is
>> > added to allow passing of a user namespace other than that of
>> > current, and this is used by proc_mount(). sget() becomes a
>> > wrapper around sget_userns() which passes current_user_ns().
>> 
>> From bits of the previous conversation.
>> 
>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
>> xattrs can travel from one mount of sysfs to another via the sysfs
>> backing store.
>> 
>> For tmpfs and any other filesystems we support mounting without
>> privilige that support xattrs.  We need to identify them and
>> see if userspace is taking advantage of the ability to set
>> xattrs and file caps (unlikely).  If they are we need to call
>> sget_userns(..., &init_user_ns) on those filesystems as well.
>> 
>> Possibly/Probably we should just do that for all of the interesting
>> filesystems to start with and then change back to an ordinary old sget
>> after we have done the testing and confirmed we will not be introducing
>> userspace regressions.
>
> I was reviewing everything in preparation for sending v2 patches, and I
> realized that doing this has an undesirable side effect. In patch 2 the
> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
> is used to block opening devices in these mounts. When we set s_user_ns
> to &init_user_ns, it becomes possible to open device nodes from
> unprivileged mounts of these filesystems.
>
> This doesn't pose a real problem today. The only filesystems it will
> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
> &init_user_ns for user namespace mounts), and all of these aren't
> problems. sysfs is okay because kernfs doesn't (currently?) allow device
> nodes, and a user would require CAP_MKNOD to create any device nodes in
> a tmpfs or ramfs mount.
>
> But for sysfs in particular it does mean that we will need to make sure
> that there's no way that device nodes could start appearing in an
> unprivileged mount.

Good point about nodev.  

For tmpfs and ramfs and security labels the smack policy of allowing but
filtering security labels mean smack once it has those bits will not
care which user namespace ramfs and tmpfs live in.  The labels should
pretty much stay the same in any case.

If the same class of handling will also apply to selinux and those are
the only two security modules that apply labels than we can leave tmpfs
and ramfs with the security labels of whomever mounted them.

For sysfs things get a little more interesting.  Assuming tmpfs and
ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
with possibly invalid securitly labels set on a different mount of
selinux.  (I am wondering now how all of these labels work in the
context of nfs).

The worst case for sysfs is that we come up with a cousin of
SB_I_NO_EXEC say SB_I_NO_DEV.

But at the moment I am hoping that limited label storage in a user
namespace as you and Casey have been talking about winds up being the
norm and then we can follow the standard rules for setting s_user_ns and
still preserve the current label setting behavior.

Eric

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
> >> Seth Forshee <seth.forshee@canonical.com> writes:
> >> 
> >> > Initially this will be used to eliminate the implicit MNT_NODEV
> >> > flag for mounts from user namespaces. In the future it will also
> >> > be used for translating ids and checking capabilities for
> >> > filesystems mounted from user namespaces.
> >> >
> >> > s_user_ns is initialized in alloc_super() and is generally set to
> >> > current_user_ns(). To avoid security and corruption issues, two
> >> > additional mount checks are also added:
> >> >
> >> >  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
> >> >    in current_user_ns().
> >> >
> >> >  - sget() will fail with EBUSY when the filesystem it's looking
> >> >    for is already mounted from another user namespace.
> >> >
> >> > proc needs some special handling here. The user namespace of
> >> > current isn't appropriate when forking as a result of clone (2)
> >> > with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> >> > from within the new user namespace. Instead, the user namespace
> >> > which owns the new pid namespace should be used. sget_userns() is
> >> > added to allow passing of a user namespace other than that of
> >> > current, and this is used by proc_mount(). sget() becomes a
> >> > wrapper around sget_userns() which passes current_user_ns().
> >> 
> >> From bits of the previous conversation.
> >> 
> >> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> >> xattrs can travel from one mount of sysfs to another via the sysfs
> >> backing store.
> >> 
> >> For tmpfs and any other filesystems we support mounting without
> >> privilige that support xattrs.  We need to identify them and
> >> see if userspace is taking advantage of the ability to set
> >> xattrs and file caps (unlikely).  If they are we need to call
> >> sget_userns(..., &init_user_ns) on those filesystems as well.
> >> 
> >> Possibly/Probably we should just do that for all of the interesting
> >> filesystems to start with and then change back to an ordinary old sget
> >> after we have done the testing and confirmed we will not be introducing
> >> userspace regressions.
> >
> > I was reviewing everything in preparation for sending v2 patches, and I
> > realized that doing this has an undesirable side effect. In patch 2 the
> > implicit nodev is removed for unprivileged mounts, and instead s_user_ns
> > is used to block opening devices in these mounts. When we set s_user_ns
> > to &init_user_ns, it becomes possible to open device nodes from
> > unprivileged mounts of these filesystems.
> >
> > This doesn't pose a real problem today. The only filesystems it will
> > affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
> > &init_user_ns for user namespace mounts), and all of these aren't
> > problems. sysfs is okay because kernfs doesn't (currently?) allow device
> > nodes, and a user would require CAP_MKNOD to create any device nodes in
> > a tmpfs or ramfs mount.
> >
> > But for sysfs in particular it does mean that we will need to make sure
> > that there's no way that device nodes could start appearing in an
> > unprivileged mount.
> 
> Good point about nodev.  
> 
> For tmpfs and ramfs and security labels the smack policy of allowing but
> filtering security labels mean smack once it has those bits will not
> care which user namespace ramfs and tmpfs live in.  The labels should
> pretty much stay the same in any case.

Smack does care which namespace ramfs and tmpfs are in. With the patch
I've got right now, if s_user_ns != &init_user_ns and the label of an
inode does not match that of the root inode then
security_inode_permission() will return EACCES.

So if something with CAP_MAC_ADMIN is changing security labels in such a
mount, suddenly those inodes might become inaccessible. And while it may
be unlikely that anyone is doing this it's impossible for me to prove
that's the case.

> If the same class of handling will also apply to selinux and those are
> the only two security modules that apply labels than we can leave tmpfs
> and ramfs with the security labels of whomever mounted them.

For SELinux I now have a patch which applies mountpoint labeling to
mounts for which s_user_ns != &init_user_ns. I'm less sure then with
Smack how this behavior will differ from what happens today, but my
understanding is that this means that the label of the mountpoint is
used for all objects from that superblock. Afaik it does not have the
Smack behavior of denying access to filesystem objects which have a
different label in the backing store.

> For sysfs things get a little more interesting.  Assuming tmpfs and
> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
> with possibly invalid securitly labels set on a different mount of
> selinux.  (I am wondering now how all of these labels work in the
> context of nfs).

If someone was using Smack to label sysfs then a mount with s_user_ns !=
&init_user_ns is going to leave inaccessible anything without the same
label as the process which performed the mount.

Again with SELinux I'm less certain, but I think you could end up with a
sysfs superblock that has mountpoint labeling, and thus any labels set
in the mount in the init namespace would be ignored.

> The worst case for sysfs is that we come up with a cousin of
> SB_I_NO_EXEC say SB_I_NO_DEV.

That idea occurred to me. Or else something that indicated to the
security module that the filesystem has no user-controlled backing store
which could be used to inject security labels, thus allowing us to set
s_user_ns to a non-init namespace while still allowing standard MAC
labeling behavior.

> But at the moment I am hoping that limited label storage in a user
> namespace as you and Casey have been talking about winds up being the
> norm and then we can follow the standard rules for setting s_user_ns and
> still preserve the current label setting behavior.

Unfortunately I'm afraid that's not going to work out.

Seth

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Stephen Smalley]

On 08/06/2015 10:20 AM, Seth Forshee wrote:
> On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
>> Seth Forshee <seth.forshee@canonical.com> writes:
>>
>>> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
>>>> Seth Forshee <seth.forshee@canonical.com> writes:
>>>>
>>>>> Initially this will be used to eliminate the implicit MNT_NODEV
>>>>> flag for mounts from user namespaces. In the future it will also
>>>>> be used for translating ids and checking capabilities for
>>>>> filesystems mounted from user namespaces.
>>>>>
>>>>> s_user_ns is initialized in alloc_super() and is generally set to
>>>>> current_user_ns(). To avoid security and corruption issues, two
>>>>> additional mount checks are also added:
>>>>>
>>>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>>>>>    in current_user_ns().
>>>>>
>>>>>  - sget() will fail with EBUSY when the filesystem it's looking
>>>>>    for is already mounted from another user namespace.
>>>>>
>>>>> proc needs some special handling here. The user namespace of
>>>>> current isn't appropriate when forking as a result of clone (2)
>>>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
>>>>> from within the new user namespace. Instead, the user namespace
>>>>> which owns the new pid namespace should be used. sget_userns() is
>>>>> added to allow passing of a user namespace other than that of
>>>>> current, and this is used by proc_mount(). sget() becomes a
>>>>> wrapper around sget_userns() which passes current_user_ns().
>>>>
>>>> From bits of the previous conversation.
>>>>
>>>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
>>>> xattrs can travel from one mount of sysfs to another via the sysfs
>>>> backing store.
>>>>
>>>> For tmpfs and any other filesystems we support mounting without
>>>> privilige that support xattrs.  We need to identify them and
>>>> see if userspace is taking advantage of the ability to set
>>>> xattrs and file caps (unlikely).  If they are we need to call
>>>> sget_userns(..., &init_user_ns) on those filesystems as well.
>>>>
>>>> Possibly/Probably we should just do that for all of the interesting
>>>> filesystems to start with and then change back to an ordinary old sget
>>>> after we have done the testing and confirmed we will not be introducing
>>>> userspace regressions.
>>>
>>> I was reviewing everything in preparation for sending v2 patches, and I
>>> realized that doing this has an undesirable side effect. In patch 2 the
>>> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
>>> is used to block opening devices in these mounts. When we set s_user_ns
>>> to &init_user_ns, it becomes possible to open device nodes from
>>> unprivileged mounts of these filesystems.
>>>
>>> This doesn't pose a real problem today. The only filesystems it will
>>> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
>>> &init_user_ns for user namespace mounts), and all of these aren't
>>> problems. sysfs is okay because kernfs doesn't (currently?) allow device
>>> nodes, and a user would require CAP_MKNOD to create any device nodes in
>>> a tmpfs or ramfs mount.
>>>
>>> But for sysfs in particular it does mean that we will need to make sure
>>> that there's no way that device nodes could start appearing in an
>>> unprivileged mount.
>>
>> Good point about nodev.  
>>
>> For tmpfs and ramfs and security labels the smack policy of allowing but
>> filtering security labels mean smack once it has those bits will not
>> care which user namespace ramfs and tmpfs live in.  The labels should
>> pretty much stay the same in any case.
> 
> Smack does care which namespace ramfs and tmpfs are in. With the patch
> I've got right now, if s_user_ns != &init_user_ns and the label of an
> inode does not match that of the root inode then
> security_inode_permission() will return EACCES.
> 
> So if something with CAP_MAC_ADMIN is changing security labels in such a
> mount, suddenly those inodes might become inaccessible. And while it may
> be unlikely that anyone is doing this it's impossible for me to prove
> that's the case.
> 
>> If the same class of handling will also apply to selinux and those are
>> the only two security modules that apply labels than we can leave tmpfs
>> and ramfs with the security labels of whomever mounted them.
> 
> For SELinux I now have a patch which applies mountpoint labeling to
> mounts for which s_user_ns != &init_user_ns. I'm less sure then with
> Smack how this behavior will differ from what happens today, but my
> understanding is that this means that the label of the mountpoint is
> used for all objects from that superblock. Afaik it does not have the
> Smack behavior of denying access to filesystem objects which have a
> different label in the backing store.
> 
>> For sysfs things get a little more interesting.  Assuming tmpfs and
>> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
>> with possibly invalid securitly labels set on a different mount of
>> selinux.  (I am wondering now how all of these labels work in the
>> context of nfs).
> 
> If someone was using Smack to label sysfs then a mount with s_user_ns !=
> &init_user_ns is going to leave inaccessible anything without the same
> label as the process which performed the mount.
> 
> Again with SELinux I'm less certain, but I think you could end up with a
> sysfs superblock that has mountpoint labeling, and thus any labels set
> in the mount in the init namespace would be ignored.

If you're using the logic I suggested for SELinux, then SELinux will
only use mountpoint labeling if SELinux would otherwise fetch the
extended attribute value from the filesystem via ->getxattr (this is the
SECURITY_FS_USE_XATTR test in the code).  As this is not the case for
purely in-memory filesystems like tmpfs, ramfs, or sysfs, SELinux will
still label those filesystems in the usual manner, i.e. it initially
computes a default label for new inodes, and if userspace later performs
a setxattr(), then it updates its internal state at that time from the
relevant hooks (inode_post_setxattr or inode_setsecurity).
So nothing should change for SELinux wrt labeling of tmpfs, ramfs, or
sysfs in userns mounts aside from not allowing the use of the additional
mount options (e.g. context=).

Also, a superblock can only have a single labeling behavior, so you
can't have different mounts of sysfs, one using mountpoint labeling and
one not.  An inode can only have one label, no matter how you reach it.

>> The worst case for sysfs is that we come up with a cousin of
>> SB_I_NO_EXEC say SB_I_NO_DEV.
> 
> That idea occurred to me. Or else something that indicated to the
> security module that the filesystem has no user-controlled backing store
> which could be used to inject security labels, thus allowing us to set
> s_user_ns to a non-init namespace while still allowing standard MAC
> labeling behavior.
> 
>> But at the moment I am hoping that limited label storage in a user
>> namespace as you and Casey have been talking about winds up being the
>> norm and then we can follow the standard rules for setting s_user_ns and
>> still preserve the current label setting behavior.
> 
> Unfortunately I'm afraid that's not going to work out.
> 
> Seth
> 

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

On Thu, Aug 06, 2015 at 10:51:16AM -0400, Stephen Smalley wrote:
> On 08/06/2015 10:20 AM, Seth Forshee wrote:
> > On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
> >> Seth Forshee <seth.forshee@canonical.com> writes:
> >>
> >>> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
> >>>> Seth Forshee <seth.forshee@canonical.com> writes:
> >>>>
> >>>>> Initially this will be used to eliminate the implicit MNT_NODEV
> >>>>> flag for mounts from user namespaces. In the future it will also
> >>>>> be used for translating ids and checking capabilities for
> >>>>> filesystems mounted from user namespaces.
> >>>>>
> >>>>> s_user_ns is initialized in alloc_super() and is generally set to
> >>>>> current_user_ns(). To avoid security and corruption issues, two
> >>>>> additional mount checks are also added:
> >>>>>
> >>>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
> >>>>>    in current_user_ns().
> >>>>>
> >>>>>  - sget() will fail with EBUSY when the filesystem it's looking
> >>>>>    for is already mounted from another user namespace.
> >>>>>
> >>>>> proc needs some special handling here. The user namespace of
> >>>>> current isn't appropriate when forking as a result of clone (2)
> >>>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> >>>>> from within the new user namespace. Instead, the user namespace
> >>>>> which owns the new pid namespace should be used. sget_userns() is
> >>>>> added to allow passing of a user namespace other than that of
> >>>>> current, and this is used by proc_mount(). sget() becomes a
> >>>>> wrapper around sget_userns() which passes current_user_ns().
> >>>>
> >>>> From bits of the previous conversation.
> >>>>
> >>>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> >>>> xattrs can travel from one mount of sysfs to another via the sysfs
> >>>> backing store.
> >>>>
> >>>> For tmpfs and any other filesystems we support mounting without
> >>>> privilige that support xattrs.  We need to identify them and
> >>>> see if userspace is taking advantage of the ability to set
> >>>> xattrs and file caps (unlikely).  If they are we need to call
> >>>> sget_userns(..., &init_user_ns) on those filesystems as well.
> >>>>
> >>>> Possibly/Probably we should just do that for all of the interesting
> >>>> filesystems to start with and then change back to an ordinary old sget
> >>>> after we have done the testing and confirmed we will not be introducing
> >>>> userspace regressions.
> >>>
> >>> I was reviewing everything in preparation for sending v2 patches, and I
> >>> realized that doing this has an undesirable side effect. In patch 2 the
> >>> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
> >>> is used to block opening devices in these mounts. When we set s_user_ns
> >>> to &init_user_ns, it becomes possible to open device nodes from
> >>> unprivileged mounts of these filesystems.
> >>>
> >>> This doesn't pose a real problem today. The only filesystems it will
> >>> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
> >>> &init_user_ns for user namespace mounts), and all of these aren't
> >>> problems. sysfs is okay because kernfs doesn't (currently?) allow device
> >>> nodes, and a user would require CAP_MKNOD to create any device nodes in
> >>> a tmpfs or ramfs mount.
> >>>
> >>> But for sysfs in particular it does mean that we will need to make sure
> >>> that there's no way that device nodes could start appearing in an
> >>> unprivileged mount.
> >>
> >> Good point about nodev.  
> >>
> >> For tmpfs and ramfs and security labels the smack policy of allowing but
> >> filtering security labels mean smack once it has those bits will not
> >> care which user namespace ramfs and tmpfs live in.  The labels should
> >> pretty much stay the same in any case.
> > 
> > Smack does care which namespace ramfs and tmpfs are in. With the patch
> > I've got right now, if s_user_ns != &init_user_ns and the label of an
> > inode does not match that of the root inode then
> > security_inode_permission() will return EACCES.
> > 
> > So if something with CAP_MAC_ADMIN is changing security labels in such a
> > mount, suddenly those inodes might become inaccessible. And while it may
> > be unlikely that anyone is doing this it's impossible for me to prove
> > that's the case.
> > 
> >> If the same class of handling will also apply to selinux and those are
> >> the only two security modules that apply labels than we can leave tmpfs
> >> and ramfs with the security labels of whomever mounted them.
> > 
> > For SELinux I now have a patch which applies mountpoint labeling to
> > mounts for which s_user_ns != &init_user_ns. I'm less sure then with
> > Smack how this behavior will differ from what happens today, but my
> > understanding is that this means that the label of the mountpoint is
> > used for all objects from that superblock. Afaik it does not have the
> > Smack behavior of denying access to filesystem objects which have a
> > different label in the backing store.
> > 
> >> For sysfs things get a little more interesting.  Assuming tmpfs and
> >> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
> >> with possibly invalid securitly labels set on a different mount of
> >> selinux.  (I am wondering now how all of these labels work in the
> >> context of nfs).
> > 
> > If someone was using Smack to label sysfs then a mount with s_user_ns !=
> > &init_user_ns is going to leave inaccessible anything without the same
> > label as the process which performed the mount.
> > 
> > Again with SELinux I'm less certain, but I think you could end up with a
> > sysfs superblock that has mountpoint labeling, and thus any labels set
> > in the mount in the init namespace would be ignored.
> 
> If you're using the logic I suggested for SELinux, then SELinux will
> only use mountpoint labeling if SELinux would otherwise fetch the
> extended attribute value from the filesystem via ->getxattr (this is the
> SECURITY_FS_USE_XATTR test in the code).  As this is not the case for
> purely in-memory filesystems like tmpfs, ramfs, or sysfs, SELinux will
> still label those filesystems in the usual manner, i.e. it initially
> computes a default label for new inodes, and if userspace later performs
> a setxattr(), then it updates its internal state at that time from the
> relevant hooks (inode_post_setxattr or inode_setsecurity).
> So nothing should change for SELinux wrt labeling of tmpfs, ramfs, or
> sysfs in userns mounts aside from not allowing the use of the additional
> mount options (e.g. context=).

This is the patch I have currently:

http://kernel.ubuntu.com/git/sforshee/linux.git/commit/?h=userns-mounts&id=080e5f5ee58143a56cfc57b4e51dff58b7a3cb1a

I haven't been able to figure out which labeling behavior sysfs would
end up with normally from just inspecting the code. kernfs does support
xattrs, but now that I look at the implementation it handles security
xattrs differently and calls security_inode_setsecurity whenever one is
written. I'm not sure how all of that is going to work out in practice
with SELinux.

> Also, a superblock can only have a single labeling behavior, so you
> can't have different mounts of sysfs, one using mountpoint labeling and
> one not.  An inode can only have one label, no matter how you reach it.

There are multiple sysfs superblocks though, see sysfs_mount(). It calls
kernfs_mount_ns(), passing a kobject for the current net ns.
kernfs_test_super() only matches if the net ns matches an existing
superblock, so you end up with a different superblock per net ns.

For kobjects which aren't namespaced, the same path within two different
sysfs superblocks will be backed by the same kernfs node. kernfs stashes
the security context inside the kernfs node, so inodes in different
superblocks backed by the same kernfs node will have the same security
context.

So, with sysfs you can have different superblocks with (partially) the
same backing store, and it would be possible for those superblocks to
end up with different labeling behavior. I think we want to avoid having
security labels applied to sysfs files in the init namespace and have
those get lost in a mount from another namespace.

Seth

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Stephen Smalley]

On 08/06/2015 11:44 AM, Seth Forshee wrote:
> On Thu, Aug 06, 2015 at 10:51:16AM -0400, Stephen Smalley wrote:
>> On 08/06/2015 10:20 AM, Seth Forshee wrote:
>>> On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
>>>> Seth Forshee <seth.forshee@canonical.com> writes:
>>>>
>>>>> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
>>>>>> Seth Forshee <seth.forshee@canonical.com> writes:
>>>>>>
>>>>>>> Initially this will be used to eliminate the implicit MNT_NODEV
>>>>>>> flag for mounts from user namespaces. In the future it will also
>>>>>>> be used for translating ids and checking capabilities for
>>>>>>> filesystems mounted from user namespaces.
>>>>>>>
>>>>>>> s_user_ns is initialized in alloc_super() and is generally set to
>>>>>>> current_user_ns(). To avoid security and corruption issues, two
>>>>>>> additional mount checks are also added:
>>>>>>>
>>>>>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>>>>>>>    in current_user_ns().
>>>>>>>
>>>>>>>  - sget() will fail with EBUSY when the filesystem it's looking
>>>>>>>    for is already mounted from another user namespace.
>>>>>>>
>>>>>>> proc needs some special handling here. The user namespace of
>>>>>>> current isn't appropriate when forking as a result of clone (2)
>>>>>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
>>>>>>> from within the new user namespace. Instead, the user namespace
>>>>>>> which owns the new pid namespace should be used. sget_userns() is
>>>>>>> added to allow passing of a user namespace other than that of
>>>>>>> current, and this is used by proc_mount(). sget() becomes a
>>>>>>> wrapper around sget_userns() which passes current_user_ns().
>>>>>>
>>>>>> From bits of the previous conversation.
>>>>>>
>>>>>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
>>>>>> xattrs can travel from one mount of sysfs to another via the sysfs
>>>>>> backing store.
>>>>>>
>>>>>> For tmpfs and any other filesystems we support mounting without
>>>>>> privilige that support xattrs.  We need to identify them and
>>>>>> see if userspace is taking advantage of the ability to set
>>>>>> xattrs and file caps (unlikely).  If they are we need to call
>>>>>> sget_userns(..., &init_user_ns) on those filesystems as well.
>>>>>>
>>>>>> Possibly/Probably we should just do that for all of the interesting
>>>>>> filesystems to start with and then change back to an ordinary old sget
>>>>>> after we have done the testing and confirmed we will not be introducing
>>>>>> userspace regressions.
>>>>>
>>>>> I was reviewing everything in preparation for sending v2 patches, and I
>>>>> realized that doing this has an undesirable side effect. In patch 2 the
>>>>> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
>>>>> is used to block opening devices in these mounts. When we set s_user_ns
>>>>> to &init_user_ns, it becomes possible to open device nodes from
>>>>> unprivileged mounts of these filesystems.
>>>>>
>>>>> This doesn't pose a real problem today. The only filesystems it will
>>>>> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
>>>>> &init_user_ns for user namespace mounts), and all of these aren't
>>>>> problems. sysfs is okay because kernfs doesn't (currently?) allow device
>>>>> nodes, and a user would require CAP_MKNOD to create any device nodes in
>>>>> a tmpfs or ramfs mount.
>>>>>
>>>>> But for sysfs in particular it does mean that we will need to make sure
>>>>> that there's no way that device nodes could start appearing in an
>>>>> unprivileged mount.
>>>>
>>>> Good point about nodev.  
>>>>
>>>> For tmpfs and ramfs and security labels the smack policy of allowing but
>>>> filtering security labels mean smack once it has those bits will not
>>>> care which user namespace ramfs and tmpfs live in.  The labels should
>>>> pretty much stay the same in any case.
>>>
>>> Smack does care which namespace ramfs and tmpfs are in. With the patch
>>> I've got right now, if s_user_ns != &init_user_ns and the label of an
>>> inode does not match that of the root inode then
>>> security_inode_permission() will return EACCES.
>>>
>>> So if something with CAP_MAC_ADMIN is changing security labels in such a
>>> mount, suddenly those inodes might become inaccessible. And while it may
>>> be unlikely that anyone is doing this it's impossible for me to prove
>>> that's the case.
>>>
>>>> If the same class of handling will also apply to selinux and those are
>>>> the only two security modules that apply labels than we can leave tmpfs
>>>> and ramfs with the security labels of whomever mounted them.
>>>
>>> For SELinux I now have a patch which applies mountpoint labeling to
>>> mounts for which s_user_ns != &init_user_ns. I'm less sure then with
>>> Smack how this behavior will differ from what happens today, but my
>>> understanding is that this means that the label of the mountpoint is
>>> used for all objects from that superblock. Afaik it does not have the
>>> Smack behavior of denying access to filesystem objects which have a
>>> different label in the backing store.
>>>
>>>> For sysfs things get a little more interesting.  Assuming tmpfs and
>>>> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
>>>> with possibly invalid securitly labels set on a different mount of
>>>> selinux.  (I am wondering now how all of these labels work in the
>>>> context of nfs).
>>>
>>> If someone was using Smack to label sysfs then a mount with s_user_ns !=
>>> &init_user_ns is going to leave inaccessible anything without the same
>>> label as the process which performed the mount.
>>>
>>> Again with SELinux I'm less certain, but I think you could end up with a
>>> sysfs superblock that has mountpoint labeling, and thus any labels set
>>> in the mount in the init namespace would be ignored.
>>
>> If you're using the logic I suggested for SELinux, then SELinux will
>> only use mountpoint labeling if SELinux would otherwise fetch the
>> extended attribute value from the filesystem via ->getxattr (this is the
>> SECURITY_FS_USE_XATTR test in the code).  As this is not the case for
>> purely in-memory filesystems like tmpfs, ramfs, or sysfs, SELinux will
>> still label those filesystems in the usual manner, i.e. it initially
>> computes a default label for new inodes, and if userspace later performs
>> a setxattr(), then it updates its internal state at that time from the
>> relevant hooks (inode_post_setxattr or inode_setsecurity).
>> So nothing should change for SELinux wrt labeling of tmpfs, ramfs, or
>> sysfs in userns mounts aside from not allowing the use of the additional
>> mount options (e.g. context=).
> 
> This is the patch I have currently:
> 
> http://kernel.ubuntu.com/git/sforshee/linux.git/commit/?h=userns-mounts&id=080e5f5ee58143a56cfc57b4e51dff58b7a3cb1a
> 
> I haven't been able to figure out which labeling behavior sysfs would
> end up with normally from just inspecting the code. kernfs does support
> xattrs, but now that I look at the implementation it handles security
> xattrs differently and calls security_inode_setsecurity whenever one is
> written. I'm not sure how all of that is going to work out in practice
> with SELinux.

sysfs would have a labeling behavior of SECURITY_FS_USE_GENFS
(policy-driven).  It wouldn't make sense to configure sysfs with
SECURITY_FS_USE_XATTR, because that would cause SELinux to ask the
filesystem via ->getxattr for the initial value for the label when the
inode is first instantiated, and sysfs would have no answer there.  So,
in practice, sysfs will still get labeled exactly as before, and there
would be no change in behavior.  Similarly for tmpfs
(SECURITY_FS_USE_TRANS) or ramfs.  The only filesystem types that get
SECURITY_FS_USE_XATTR are the ones that actually support storing SELinux
attributes persistently and therefore could provide an initial value
from backing store.

>> Also, a superblock can only have a single labeling behavior, so you
>> can't have different mounts of sysfs, one using mountpoint labeling and
>> one not.  An inode can only have one label, no matter how you reach it.
> 
> There are multiple sysfs superblocks though, see sysfs_mount(). It calls
> kernfs_mount_ns(), passing a kobject for the current net ns.
> kernfs_test_super() only matches if the net ns matches an existing
> superblock, so you end up with a different superblock per net ns.
> 
> For kobjects which aren't namespaced, the same path within two different
> sysfs superblocks will be backed by the same kernfs node. kernfs stashes
> the security context inside the kernfs node, so inodes in different
> superblocks backed by the same kernfs node will have the same security
> context.
> 
> So, with sysfs you can have different superblocks with (partially) the
> same backing store, and it would be possible for those superblocks to
> end up with different labeling behavior. I think we want to avoid having
> security labels applied to sysfs files in the init namespace and have
> those get lost in a mount from another namespace.

As long as we prohibit context= mounts on the userns mounts (which your
patch does), then this shouldn't be possible.  Maybe we should just do
that for sysfs always.

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

On Thu, Aug 06, 2015 at 12:11:53PM -0400, Stephen Smalley wrote:
> On 08/06/2015 11:44 AM, Seth Forshee wrote:
> > On Thu, Aug 06, 2015 at 10:51:16AM -0400, Stephen Smalley wrote:
> >> On 08/06/2015 10:20 AM, Seth Forshee wrote:
> >>> On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
> >>>> Seth Forshee <seth.forshee@canonical.com> writes:
> >>>>
> >>>>> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
> >>>>>> Seth Forshee <seth.forshee@canonical.com> writes:
> >>>>>>
> >>>>>>> Initially this will be used to eliminate the implicit MNT_NODEV
> >>>>>>> flag for mounts from user namespaces. In the future it will also
> >>>>>>> be used for translating ids and checking capabilities for
> >>>>>>> filesystems mounted from user namespaces.
> >>>>>>>
> >>>>>>> s_user_ns is initialized in alloc_super() and is generally set to
> >>>>>>> current_user_ns(). To avoid security and corruption issues, two
> >>>>>>> additional mount checks are also added:
> >>>>>>>
> >>>>>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
> >>>>>>>    in current_user_ns().
> >>>>>>>
> >>>>>>>  - sget() will fail with EBUSY when the filesystem it's looking
> >>>>>>>    for is already mounted from another user namespace.
> >>>>>>>
> >>>>>>> proc needs some special handling here. The user namespace of
> >>>>>>> current isn't appropriate when forking as a result of clone (2)
> >>>>>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> >>>>>>> from within the new user namespace. Instead, the user namespace
> >>>>>>> which owns the new pid namespace should be used. sget_userns() is
> >>>>>>> added to allow passing of a user namespace other than that of
> >>>>>>> current, and this is used by proc_mount(). sget() becomes a
> >>>>>>> wrapper around sget_userns() which passes current_user_ns().
> >>>>>>
> >>>>>> From bits of the previous conversation.
> >>>>>>
> >>>>>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> >>>>>> xattrs can travel from one mount of sysfs to another via the sysfs
> >>>>>> backing store.
> >>>>>>
> >>>>>> For tmpfs and any other filesystems we support mounting without
> >>>>>> privilige that support xattrs.  We need to identify them and
> >>>>>> see if userspace is taking advantage of the ability to set
> >>>>>> xattrs and file caps (unlikely).  If they are we need to call
> >>>>>> sget_userns(..., &init_user_ns) on those filesystems as well.
> >>>>>>
> >>>>>> Possibly/Probably we should just do that for all of the interesting
> >>>>>> filesystems to start with and then change back to an ordinary old sget
> >>>>>> after we have done the testing and confirmed we will not be introducing
> >>>>>> userspace regressions.
> >>>>>
> >>>>> I was reviewing everything in preparation for sending v2 patches, and I
> >>>>> realized that doing this has an undesirable side effect. In patch 2 the
> >>>>> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
> >>>>> is used to block opening devices in these mounts. When we set s_user_ns
> >>>>> to &init_user_ns, it becomes possible to open device nodes from
> >>>>> unprivileged mounts of these filesystems.
> >>>>>
> >>>>> This doesn't pose a real problem today. The only filesystems it will
> >>>>> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
> >>>>> &init_user_ns for user namespace mounts), and all of these aren't
> >>>>> problems. sysfs is okay because kernfs doesn't (currently?) allow device
> >>>>> nodes, and a user would require CAP_MKNOD to create any device nodes in
> >>>>> a tmpfs or ramfs mount.
> >>>>>
> >>>>> But for sysfs in particular it does mean that we will need to make sure
> >>>>> that there's no way that device nodes could start appearing in an
> >>>>> unprivileged mount.
> >>>>
> >>>> Good point about nodev.  
> >>>>
> >>>> For tmpfs and ramfs and security labels the smack policy of allowing but
> >>>> filtering security labels mean smack once it has those bits will not
> >>>> care which user namespace ramfs and tmpfs live in.  The labels should
> >>>> pretty much stay the same in any case.
> >>>
> >>> Smack does care which namespace ramfs and tmpfs are in. With the patch
> >>> I've got right now, if s_user_ns != &init_user_ns and the label of an
> >>> inode does not match that of the root inode then
> >>> security_inode_permission() will return EACCES.
> >>>
> >>> So if something with CAP_MAC_ADMIN is changing security labels in such a
> >>> mount, suddenly those inodes might become inaccessible. And while it may
> >>> be unlikely that anyone is doing this it's impossible for me to prove
> >>> that's the case.
> >>>
> >>>> If the same class of handling will also apply to selinux and those are
> >>>> the only two security modules that apply labels than we can leave tmpfs
> >>>> and ramfs with the security labels of whomever mounted them.
> >>>
> >>> For SELinux I now have a patch which applies mountpoint labeling to
> >>> mounts for which s_user_ns != &init_user_ns. I'm less sure then with
> >>> Smack how this behavior will differ from what happens today, but my
> >>> understanding is that this means that the label of the mountpoint is
> >>> used for all objects from that superblock. Afaik it does not have the
> >>> Smack behavior of denying access to filesystem objects which have a
> >>> different label in the backing store.
> >>>
> >>>> For sysfs things get a little more interesting.  Assuming tmpfs and
> >>>> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
> >>>> with possibly invalid securitly labels set on a different mount of
> >>>> selinux.  (I am wondering now how all of these labels work in the
> >>>> context of nfs).
> >>>
> >>> If someone was using Smack to label sysfs then a mount with s_user_ns !=
> >>> &init_user_ns is going to leave inaccessible anything without the same
> >>> label as the process which performed the mount.
> >>>
> >>> Again with SELinux I'm less certain, but I think you could end up with a
> >>> sysfs superblock that has mountpoint labeling, and thus any labels set
> >>> in the mount in the init namespace would be ignored.
> >>
> >> If you're using the logic I suggested for SELinux, then SELinux will
> >> only use mountpoint labeling if SELinux would otherwise fetch the
> >> extended attribute value from the filesystem via ->getxattr (this is the
> >> SECURITY_FS_USE_XATTR test in the code).  As this is not the case for
> >> purely in-memory filesystems like tmpfs, ramfs, or sysfs, SELinux will
> >> still label those filesystems in the usual manner, i.e. it initially
> >> computes a default label for new inodes, and if userspace later performs
> >> a setxattr(), then it updates its internal state at that time from the
> >> relevant hooks (inode_post_setxattr or inode_setsecurity).
> >> So nothing should change for SELinux wrt labeling of tmpfs, ramfs, or
> >> sysfs in userns mounts aside from not allowing the use of the additional
> >> mount options (e.g. context=).
> > 
> > This is the patch I have currently:
> > 
> > http://kernel.ubuntu.com/git/sforshee/linux.git/commit/?h=userns-mounts&id=080e5f5ee58143a56cfc57b4e51dff58b7a3cb1a
> > 
> > I haven't been able to figure out which labeling behavior sysfs would
> > end up with normally from just inspecting the code. kernfs does support
> > xattrs, but now that I look at the implementation it handles security
> > xattrs differently and calls security_inode_setsecurity whenever one is
> > written. I'm not sure how all of that is going to work out in practice
> > with SELinux.
> 
> sysfs would have a labeling behavior of SECURITY_FS_USE_GENFS
> (policy-driven).  It wouldn't make sense to configure sysfs with
> SECURITY_FS_USE_XATTR, because that would cause SELinux to ask the
> filesystem via ->getxattr for the initial value for the label when the
> inode is first instantiated, and sysfs would have no answer there.  So,
> in practice, sysfs will still get labeled exactly as before, and there
> would be no change in behavior.  Similarly for tmpfs
> (SECURITY_FS_USE_TRANS) or ramfs.  The only filesystem types that get
> SECURITY_FS_USE_XATTR are the ones that actually support storing SELinux
> attributes persistently and therefore could provide an initial value
> from backing store.
> 
> >> Also, a superblock can only have a single labeling behavior, so you
> >> can't have different mounts of sysfs, one using mountpoint labeling and
> >> one not.  An inode can only have one label, no matter how you reach it.
> > 
> > There are multiple sysfs superblocks though, see sysfs_mount(). It calls
> > kernfs_mount_ns(), passing a kobject for the current net ns.
> > kernfs_test_super() only matches if the net ns matches an existing
> > superblock, so you end up with a different superblock per net ns.
> > 
> > For kobjects which aren't namespaced, the same path within two different
> > sysfs superblocks will be backed by the same kernfs node. kernfs stashes
> > the security context inside the kernfs node, so inodes in different
> > superblocks backed by the same kernfs node will have the same security
> > context.
> > 
> > So, with sysfs you can have different superblocks with (partially) the
> > same backing store, and it would be possible for those superblocks to
> > end up with different labeling behavior. I think we want to avoid having
> > security labels applied to sysfs files in the init namespace and have
> > those get lost in a mount from another namespace.
> 
> As long as we prohibit context= mounts on the userns mounts (which your
> patch does), then this shouldn't be possible.  Maybe we should just do
> that for sysfs always.

Great. Thanks for your help.

Seth

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

On Thu, Aug 06, 2015 at 09:20:29AM -0500, Seth Forshee wrote:
> On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
> > Seth Forshee <seth.forshee@canonical.com> writes:
> > 
> > > On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
> > >> Seth Forshee <seth.forshee@canonical.com> writes:
> > >> 
> > >> > Initially this will be used to eliminate the implicit MNT_NODEV
> > >> > flag for mounts from user namespaces. In the future it will also
> > >> > be used for translating ids and checking capabilities for
> > >> > filesystems mounted from user namespaces.
> > >> >
> > >> > s_user_ns is initialized in alloc_super() and is generally set to
> > >> > current_user_ns(). To avoid security and corruption issues, two
> > >> > additional mount checks are also added:
> > >> >
> > >> >  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
> > >> >    in current_user_ns().
> > >> >
> > >> >  - sget() will fail with EBUSY when the filesystem it's looking
> > >> >    for is already mounted from another user namespace.
> > >> >
> > >> > proc needs some special handling here. The user namespace of
> > >> > current isn't appropriate when forking as a result of clone (2)
> > >> > with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> > >> > from within the new user namespace. Instead, the user namespace
> > >> > which owns the new pid namespace should be used. sget_userns() is
> > >> > added to allow passing of a user namespace other than that of
> > >> > current, and this is used by proc_mount(). sget() becomes a
> > >> > wrapper around sget_userns() which passes current_user_ns().
> > >> 
> > >> From bits of the previous conversation.
> > >> 
> > >> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> > >> xattrs can travel from one mount of sysfs to another via the sysfs
> > >> backing store.
> > >> 
> > >> For tmpfs and any other filesystems we support mounting without
> > >> privilige that support xattrs.  We need to identify them and
> > >> see if userspace is taking advantage of the ability to set
> > >> xattrs and file caps (unlikely).  If they are we need to call
> > >> sget_userns(..., &init_user_ns) on those filesystems as well.
> > >> 
> > >> Possibly/Probably we should just do that for all of the interesting
> > >> filesystems to start with and then change back to an ordinary old sget
> > >> after we have done the testing and confirmed we will not be introducing
> > >> userspace regressions.
> > >
> > > I was reviewing everything in preparation for sending v2 patches, and I
> > > realized that doing this has an undesirable side effect. In patch 2 the
> > > implicit nodev is removed for unprivileged mounts, and instead s_user_ns
> > > is used to block opening devices in these mounts. When we set s_user_ns
> > > to &init_user_ns, it becomes possible to open device nodes from
> > > unprivileged mounts of these filesystems.
> > >
> > > This doesn't pose a real problem today. The only filesystems it will
> > > affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
> > > &init_user_ns for user namespace mounts), and all of these aren't
> > > problems. sysfs is okay because kernfs doesn't (currently?) allow device
> > > nodes, and a user would require CAP_MKNOD to create any device nodes in
> > > a tmpfs or ramfs mount.
> > >
> > > But for sysfs in particular it does mean that we will need to make sure
> > > that there's no way that device nodes could start appearing in an
> > > unprivileged mount.
> > 
> > Good point about nodev.  
> > 
> > For tmpfs and ramfs and security labels the smack policy of allowing but
> > filtering security labels mean smack once it has those bits will not
> > care which user namespace ramfs and tmpfs live in.  The labels should
> > pretty much stay the same in any case.
> 
> Smack does care which namespace ramfs and tmpfs are in. With the patch
> I've got right now, if s_user_ns != &init_user_ns and the label of an
> inode does not match that of the root inode then
> security_inode_permission() will return EACCES.
> 
> So if something with CAP_MAC_ADMIN is changing security labels in such a
> mount, suddenly those inodes might become inaccessible. And while it may
> be unlikely that anyone is doing this it's impossible for me to prove
> that's the case.
> 
> > If the same class of handling will also apply to selinux and those are
> > the only two security modules that apply labels than we can leave tmpfs
> > and ramfs with the security labels of whomever mounted them.
> 
> For SELinux I now have a patch which applies mountpoint labeling to
> mounts for which s_user_ns != &init_user_ns. I'm less sure then with
> Smack how this behavior will differ from what happens today, but my
> understanding is that this means that the label of the mountpoint is
> used for all objects from that superblock. Afaik it does not have the
> Smack behavior of denying access to filesystem objects which have a
> different label in the backing store.
> 
> > For sysfs things get a little more interesting.  Assuming tmpfs and
> > ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
> > with possibly invalid securitly labels set on a different mount of
> > selinux.  (I am wondering now how all of these labels work in the
> > context of nfs).
> 
> If someone was using Smack to label sysfs then a mount with s_user_ns !=
> &init_user_ns is going to leave inaccessible anything without the same
> label as the process which performed the mount.
> 
> Again with SELinux I'm less certain, but I think you could end up with a
> sysfs superblock that has mountpoint labeling, and thus any labels set
> in the mount in the init namespace would be ignored.
> 
> > The worst case for sysfs is that we come up with a cousin of
> > SB_I_NO_EXEC say SB_I_NO_DEV.
> 
> That idea occurred to me. Or else something that indicated to the
> security module that the filesystem has no user-controlled backing store
> which could be used to inject security labels, thus allowing us to set
> s_user_ns to a non-init namespace while still allowing standard MAC
> labeling behavior.
> 
> > But at the moment I am hoping that limited label storage in a user
> > namespace as you and Casey have been talking about winds up being the
> > norm and then we can follow the standard rules for setting s_user_ns and
> > still preserve the current label setting behavior.
> 
> Unfortunately I'm afraid that's not going to work out.

What I really meant here was that it wasn't going to work out for these
few filesystems. There's no reason why that couldn't be the norm moving
forward.

Casey: Would you have a problem with special-casing Smack for these
filesystems? It's not ideal, but it avoids regressions for those
filesystems that can already be mounted in a user namespace with trusted
labels. Something like this (on top of the changes we've already
discussed).

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 244e035e5a99..473cfc355a8d 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -76,8 +76,14 @@ struct superblock_smack {
 	struct smack_known	*smk_hat;
 	struct smack_known	*smk_default;
 	int			smk_initialized;
+	int			smk_flags;
 };
 
+/*
+ * Superblock flags
+ */
+#define SMK_SB_UNTRUSTED	0x01
+
 struct socket_smack {
 	struct smack_known	*smk_out;	/* outbound label */
 	struct smack_known	*smk_in;	/* inbound label */
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8e631a66b03c..44e27f5f2a43 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -662,8 +662,16 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
 		skp = smk_of_current();
 		sp->smk_root = skp;
 		sp->smk_default = skp;
-		if (sb_in_userns(sb))
+		/*
+		 * For a handful of fs types with no user-controlled
+		 * backing store it's okay to trust security labels
+		 * in the filesystem. The rest are untrusted.
+		 */
+		if (sb->s_magic != SYSFS_MAGIC && sb->s_magic != TMPFS_MAGIC &&
+		    sb->s_magic != RAMFS_MAGIC) {
 			transmute = 1;
+			sp->smk_flags |= SMK_SB_UNTRUSTED;
+		}
 	}
 	/*
 	 * Initialize the root inode.
@@ -1014,6 +1022,7 @@ static int smack_inode_rename(struct inode *old_inode,
  */
 static int smack_inode_permission(struct inode *inode, int mask)
 {
+	struct superblock_smack *sbsp = inode->i_sb->s_security;
 	struct smk_audit_info ad;
 	int no_block = mask & MAY_NOT_BLOCK;
 	int rc;
@@ -1025,8 +1034,7 @@ static int smack_inode_permission(struct inode *inode, int mask)
 	if (mask == 0)
 		return 0;
 
-	if (sb_in_userns(inode->i_sb)) {
-		struct superblock_smack *sbsp = inode->i_sb->s_security;
+	if (sbsp->smk_flags & SMK_SB_UNTRUSTED) {
 		if (smk_of_inode(inode) != sbsp->smk_root)
 			return -EACCES;
 	}
@@ -3228,7 +3236,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
 			if (rc >= 0)
 				transflag = SMK_INODE_TRANSMUTE;
 		}
-		if (!sb_in_userns(inode->i_sb)) {
+		if (!(sbsp->smk_flags & SMK_SB_UNTRUSTED)) {
 			/*
 			 * Don't let the exec or mmap label be "*" or "@".
 			 */

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Casey Schaufler]

On 8/7/2015 7:32 AM, Seth Forshee wrote:
> On Thu, Aug 06, 2015 at 09:20:29AM -0500, Seth Forshee wrote:
>> On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
>>> Seth Forshee <seth.forshee@canonical.com> writes:
>>>
>>>> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
>>>>> Seth Forshee <seth.forshee@canonical.com> writes:
>>>>>
>>>>>> Initially this will be used to eliminate the implicit MNT_NODEV
>>>>>> flag for mounts from user namespaces. In the future it will also
>>>>>> be used for translating ids and checking capabilities for
>>>>>> filesystems mounted from user namespaces.
>>>>>>
>>>>>> s_user_ns is initialized in alloc_super() and is generally set to
>>>>>> current_user_ns(). To avoid security and corruption issues, two
>>>>>> additional mount checks are also added:
>>>>>>
>>>>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
>>>>>>    in current_user_ns().
>>>>>>
>>>>>>  - sget() will fail with EBUSY when the filesystem it's looking
>>>>>>    for is already mounted from another user namespace.
>>>>>>
>>>>>> proc needs some special handling here. The user namespace of
>>>>>> current isn't appropriate when forking as a result of clone (2)
>>>>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
>>>>>> from within the new user namespace. Instead, the user namespace
>>>>>> which owns the new pid namespace should be used. sget_userns() is
>>>>>> added to allow passing of a user namespace other than that of
>>>>>> current, and this is used by proc_mount(). sget() becomes a
>>>>>> wrapper around sget_userns() which passes current_user_ns().
>>>>> From bits of the previous conversation.
>>>>>
>>>>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
>>>>> xattrs can travel from one mount of sysfs to another via the sysfs
>>>>> backing store.
>>>>>
>>>>> For tmpfs and any other filesystems we support mounting without
>>>>> privilige that support xattrs.  We need to identify them and
>>>>> see if userspace is taking advantage of the ability to set
>>>>> xattrs and file caps (unlikely).  If they are we need to call
>>>>> sget_userns(..., &init_user_ns) on those filesystems as well.
>>>>>
>>>>> Possibly/Probably we should just do that for all of the interesting
>>>>> filesystems to start with and then change back to an ordinary old sget
>>>>> after we have done the testing and confirmed we will not be introducing
>>>>> userspace regressions.
>>>> I was reviewing everything in preparation for sending v2 patches, and I
>>>> realized that doing this has an undesirable side effect. In patch 2 the
>>>> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
>>>> is used to block opening devices in these mounts. When we set s_user_ns
>>>> to &init_user_ns, it becomes possible to open device nodes from
>>>> unprivileged mounts of these filesystems.
>>>>
>>>> This doesn't pose a real problem today. The only filesystems it will
>>>> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
>>>> &init_user_ns for user namespace mounts), and all of these aren't
>>>> problems. sysfs is okay because kernfs doesn't (currently?) allow device
>>>> nodes, and a user would require CAP_MKNOD to create any device nodes in
>>>> a tmpfs or ramfs mount.
>>>>
>>>> But for sysfs in particular it does mean that we will need to make sure
>>>> that there's no way that device nodes could start appearing in an
>>>> unprivileged mount.
>>> Good point about nodev.  
>>>
>>> For tmpfs and ramfs and security labels the smack policy of allowing but
>>> filtering security labels mean smack once it has those bits will not
>>> care which user namespace ramfs and tmpfs live in.  The labels should
>>> pretty much stay the same in any case.
>> Smack does care which namespace ramfs and tmpfs are in. With the patch
>> I've got right now, if s_user_ns != &init_user_ns and the label of an
>> inode does not match that of the root inode then
>> security_inode_permission() will return EACCES.
>>
>> So if something with CAP_MAC_ADMIN is changing security labels in such a
>> mount, suddenly those inodes might become inaccessible. And while it may
>> be unlikely that anyone is doing this it's impossible for me to prove
>> that's the case.
>>
>>> If the same class of handling will also apply to selinux and those are
>>> the only two security modules that apply labels than we can leave tmpfs
>>> and ramfs with the security labels of whomever mounted them.
>> For SELinux I now have a patch which applies mountpoint labeling to
>> mounts for which s_user_ns != &init_user_ns. I'm less sure then with
>> Smack how this behavior will differ from what happens today, but my
>> understanding is that this means that the label of the mountpoint is
>> used for all objects from that superblock. Afaik it does not have the
>> Smack behavior of denying access to filesystem objects which have a
>> different label in the backing store.
>>
>>> For sysfs things get a little more interesting.  Assuming tmpfs and
>>> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
>>> with possibly invalid securitly labels set on a different mount of
>>> selinux.  (I am wondering now how all of these labels work in the
>>> context of nfs).
>> If someone was using Smack to label sysfs then a mount with s_user_ns !=
>> &init_user_ns is going to leave inaccessible anything without the same
>> label as the process which performed the mount.
>>
>> Again with SELinux I'm less certain, but I think you could end up with a
>> sysfs superblock that has mountpoint labeling, and thus any labels set
>> in the mount in the init namespace would be ignored.
>>
>>> The worst case for sysfs is that we come up with a cousin of
>>> SB_I_NO_EXEC say SB_I_NO_DEV.
>> That idea occurred to me. Or else something that indicated to the
>> security module that the filesystem has no user-controlled backing store
>> which could be used to inject security labels, thus allowing us to set
>> s_user_ns to a non-init namespace while still allowing standard MAC
>> labeling behavior.
>>
>>> But at the moment I am hoping that limited label storage in a user
>>> namespace as you and Casey have been talking about winds up being the
>>> norm and then we can follow the standard rules for setting s_user_ns and
>>> still preserve the current label setting behavior.
>> Unfortunately I'm afraid that's not going to work out.
> What I really meant here was that it wasn't going to work out for these
> few filesystems. There's no reason why that couldn't be the norm moving
> forward.
>
> Casey: Would you have a problem with special-casing Smack for these
> filesystems? It's not ideal, but it avoids regressions for those
> filesystems that can already be mounted in a user namespace with trusted
> labels. Something like this (on top of the changes we've already
> discussed).

As badly as I want to run away screaming, I can't see a reason
that this approach doesn't make sense. With no backing store there's
no way the untrusted mounter can get untoward access to data, and
the data isn't persistent. If there weren't already filesystem
special casing in Smack I could object to that, but I've already
started down that slope.

So I'm not real happy, but I don't have a better solution.

>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 244e035e5a99..473cfc355a8d 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -76,8 +76,14 @@ struct superblock_smack {
>  	struct smack_known	*smk_hat;
>  	struct smack_known	*smk_default;
>  	int			smk_initialized;
> +	int			smk_flags;
>  };
>  
> +/*
> + * Superblock flags
> + */
> +#define SMK_SB_UNTRUSTED	0x01
> +
>  struct socket_smack {
>  	struct smack_known	*smk_out;	/* outbound label */
>  	struct smack_known	*smk_in;	/* inbound label */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8e631a66b03c..44e27f5f2a43 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -662,8 +662,16 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
>  		skp = smk_of_current();
>  		sp->smk_root = skp;
>  		sp->smk_default = skp;
> -		if (sb_in_userns(sb))
> +		/*
> +		 * For a handful of fs types with no user-controlled
> +		 * backing store it's okay to trust security labels
> +		 * in the filesystem. The rest are untrusted.
> +		 */
> +		if (sb->s_magic != SYSFS_MAGIC && sb->s_magic != TMPFS_MAGIC &&
> +		    sb->s_magic != RAMFS_MAGIC) {
>  			transmute = 1;
> +			sp->smk_flags |= SMK_SB_UNTRUSTED;
> +		}
>  	}
>  	/*
>  	 * Initialize the root inode.
> @@ -1014,6 +1022,7 @@ static int smack_inode_rename(struct inode *old_inode,
>   */
>  static int smack_inode_permission(struct inode *inode, int mask)
>  {
> +	struct superblock_smack *sbsp = inode->i_sb->s_security;
>  	struct smk_audit_info ad;
>  	int no_block = mask & MAY_NOT_BLOCK;
>  	int rc;
> @@ -1025,8 +1034,7 @@ static int smack_inode_permission(struct inode *inode, int mask)
>  	if (mask == 0)
>  		return 0;
>  
> -	if (sb_in_userns(inode->i_sb)) {
> -		struct superblock_smack *sbsp = inode->i_sb->s_security;
> +	if (sbsp->smk_flags & SMK_SB_UNTRUSTED) {
>  		if (smk_of_inode(inode) != sbsp->smk_root)
>  			return -EACCES;
>  	}
> @@ -3228,7 +3236,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  			if (rc >= 0)
>  				transflag = SMK_INODE_TRANSMUTE;
>  		}
> -		if (!sb_in_userns(inode->i_sb)) {
> +		if (!(sbsp->smk_flags & SMK_SB_UNTRUSTED)) {
>  			/*
>  			 * Don't let the exec or mmap label be "*" or "@".
>  			 */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/7] fs: Add user namesapace member to struct super_block

[Seth Forshee]

On Fri, Aug 07, 2015 at 11:35:31AM -0700, Casey Schaufler wrote:
> On 8/7/2015 7:32 AM, Seth Forshee wrote:
> > On Thu, Aug 06, 2015 at 09:20:29AM -0500, Seth Forshee wrote:
> >> On Wed, Aug 05, 2015 at 04:19:03PM -0500, Eric W. Biederman wrote:
> >>> Seth Forshee <seth.forshee@canonical.com> writes:
> >>>
> >>>> On Wed, Jul 15, 2015 at 09:47:11PM -0500, Eric W. Biederman wrote:
> >>>>> Seth Forshee <seth.forshee@canonical.com> writes:
> >>>>>
> >>>>>> Initially this will be used to eliminate the implicit MNT_NODEV
> >>>>>> flag for mounts from user namespaces. In the future it will also
> >>>>>> be used for translating ids and checking capabilities for
> >>>>>> filesystems mounted from user namespaces.
> >>>>>>
> >>>>>> s_user_ns is initialized in alloc_super() and is generally set to
> >>>>>> current_user_ns(). To avoid security and corruption issues, two
> >>>>>> additional mount checks are also added:
> >>>>>>
> >>>>>>  - do_new_mount() gains a check that the user has CAP_SYS_ADMIN
> >>>>>>    in current_user_ns().
> >>>>>>
> >>>>>>  - sget() will fail with EBUSY when the filesystem it's looking
> >>>>>>    for is already mounted from another user namespace.
> >>>>>>
> >>>>>> proc needs some special handling here. The user namespace of
> >>>>>> current isn't appropriate when forking as a result of clone (2)
> >>>>>> with CLONE_NEWPID|CLONE_NEWUSER, as it will make proc unmountable
> >>>>>> from within the new user namespace. Instead, the user namespace
> >>>>>> which owns the new pid namespace should be used. sget_userns() is
> >>>>>> added to allow passing of a user namespace other than that of
> >>>>>> current, and this is used by proc_mount(). sget() becomes a
> >>>>>> wrapper around sget_userns() which passes current_user_ns().
> >>>>> From bits of the previous conversation.
> >>>>>
> >>>>> We need sget_userns(..., &init_user_ns) for sysfs.  The sysfs
> >>>>> xattrs can travel from one mount of sysfs to another via the sysfs
> >>>>> backing store.
> >>>>>
> >>>>> For tmpfs and any other filesystems we support mounting without
> >>>>> privilige that support xattrs.  We need to identify them and
> >>>>> see if userspace is taking advantage of the ability to set
> >>>>> xattrs and file caps (unlikely).  If they are we need to call
> >>>>> sget_userns(..., &init_user_ns) on those filesystems as well.
> >>>>>
> >>>>> Possibly/Probably we should just do that for all of the interesting
> >>>>> filesystems to start with and then change back to an ordinary old sget
> >>>>> after we have done the testing and confirmed we will not be introducing
> >>>>> userspace regressions.
> >>>> I was reviewing everything in preparation for sending v2 patches, and I
> >>>> realized that doing this has an undesirable side effect. In patch 2 the
> >>>> implicit nodev is removed for unprivileged mounts, and instead s_user_ns
> >>>> is used to block opening devices in these mounts. When we set s_user_ns
> >>>> to &init_user_ns, it becomes possible to open device nodes from
> >>>> unprivileged mounts of these filesystems.
> >>>>
> >>>> This doesn't pose a real problem today. The only filesystems it will
> >>>> affect is sysfs, tmpfs, and ramfs (no others need s_user_ns =
> >>>> &init_user_ns for user namespace mounts), and all of these aren't
> >>>> problems. sysfs is okay because kernfs doesn't (currently?) allow device
> >>>> nodes, and a user would require CAP_MKNOD to create any device nodes in
> >>>> a tmpfs or ramfs mount.
> >>>>
> >>>> But for sysfs in particular it does mean that we will need to make sure
> >>>> that there's no way that device nodes could start appearing in an
> >>>> unprivileged mount.
> >>> Good point about nodev.  
> >>>
> >>> For tmpfs and ramfs and security labels the smack policy of allowing but
> >>> filtering security labels mean smack once it has those bits will not
> >>> care which user namespace ramfs and tmpfs live in.  The labels should
> >>> pretty much stay the same in any case.
> >> Smack does care which namespace ramfs and tmpfs are in. With the patch
> >> I've got right now, if s_user_ns != &init_user_ns and the label of an
> >> inode does not match that of the root inode then
> >> security_inode_permission() will return EACCES.
> >>
> >> So if something with CAP_MAC_ADMIN is changing security labels in such a
> >> mount, suddenly those inodes might become inaccessible. And while it may
> >> be unlikely that anyone is doing this it's impossible for me to prove
> >> that's the case.
> >>
> >>> If the same class of handling will also apply to selinux and those are
> >>> the only two security modules that apply labels than we can leave tmpfs
> >>> and ramfs with the security labels of whomever mounted them.
> >> For SELinux I now have a patch which applies mountpoint labeling to
> >> mounts for which s_user_ns != &init_user_ns. I'm less sure then with
> >> Smack how this behavior will differ from what happens today, but my
> >> understanding is that this means that the label of the mountpoint is
> >> used for all objects from that superblock. Afaik it does not have the
> >> Smack behavior of denying access to filesystem objects which have a
> >> different label in the backing store.
> >>
> >>> For sysfs things get a little more interesting.  Assuming tmpfs and
> >>> ramfs don't need s_user_ns == &init_user_ns, sysfs may be fine operating
> >>> with possibly invalid securitly labels set on a different mount of
> >>> selinux.  (I am wondering now how all of these labels work in the
> >>> context of nfs).
> >> If someone was using Smack to label sysfs then a mount with s_user_ns !=
> >> &init_user_ns is going to leave inaccessible anything without the same
> >> label as the process which performed the mount.
> >>
> >> Again with SELinux I'm less certain, but I think you could end up with a
> >> sysfs superblock that has mountpoint labeling, and thus any labels set
> >> in the mount in the init namespace would be ignored.
> >>
> >>> The worst case for sysfs is that we come up with a cousin of
> >>> SB_I_NO_EXEC say SB_I_NO_DEV.
> >> That idea occurred to me. Or else something that indicated to the
> >> security module that the filesystem has no user-controlled backing store
> >> which could be used to inject security labels, thus allowing us to set
> >> s_user_ns to a non-init namespace while still allowing standard MAC
> >> labeling behavior.
> >>
> >>> But at the moment I am hoping that limited label storage in a user
> >>> namespace as you and Casey have been talking about winds up being the
> >>> norm and then we can follow the standard rules for setting s_user_ns and
> >>> still preserve the current label setting behavior.
> >> Unfortunately I'm afraid that's not going to work out.
> > What I really meant here was that it wasn't going to work out for these
> > few filesystems. There's no reason why that couldn't be the norm moving
> > forward.
> >
> > Casey: Would you have a problem with special-casing Smack for these
> > filesystems? It's not ideal, but it avoids regressions for those
> > filesystems that can already be mounted in a user namespace with trusted
> > labels. Something like this (on top of the changes we've already
> > discussed).
> 
> As badly as I want to run away screaming, I can't see a reason
> that this approach doesn't make sense. With no backing store there's
> no way the untrusted mounter can get untoward access to data, and
> the data isn't persistent. If there weren't already filesystem
> special casing in Smack I could object to that, but I've already
> started down that slope.
> 
> So I'm not real happy, but I don't have a better solution.

Yeah, I understand. I had hoped there would be something we could look
at to distinguish these types of filesystems generically, but I couldn't
find anything. So short of adding some flag to the fs type or the
superblock, this was the best I could come up with.

Thanks,
Seth