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

Seth Forshee
2015-08-07T14:32:11Z

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 "@".
*/