Date:	Fri, 24 Jul 2015 10:11:37 -0500
From:	Seth Forshee <seth.forshee@...onical.com>
To:	Stephen Smalley <sds@...ho.nsa.gov>
Cc:	Serge Hallyn <serge.hallyn@...onical.com>, selinux@...ho.nsa.gov,
	linux-kernel@...r.kernel.org,
	Andy Lutomirski <luto@...capital.net>,
	linux-security-module@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	James Morris <james.l.morris@...cle.com>,
	linux-fsdevel@...r.kernel.org,
	Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH 6/7] selinux: Ignore security labels on user namespace
 mounts

On Thu, Jul 23, 2015 at 11:23:31AM -0500, Seth Forshee wrote:
> On Thu, Jul 23, 2015 at 11:36:03AM -0400, Stephen Smalley wrote:
> > On 07/23/2015 10:39 AM, Seth Forshee wrote:
> > > On Thu, Jul 23, 2015 at 09:57:20AM -0400, Stephen Smalley wrote:
> > >> On 07/22/2015 04:40 PM, Stephen Smalley wrote:
> > >>> On 07/22/2015 04:25 PM, Stephen Smalley wrote:
> > >>>> On 07/22/2015 12:14 PM, Seth Forshee wrote:
> > >>>>> On Wed, Jul 22, 2015 at 12:02:13PM -0400, Stephen Smalley wrote:
> > >>>>>> On 07/16/2015 09:23 AM, Stephen Smalley wrote:
> > >>>>>>> On 07/15/2015 03:46 PM, Seth Forshee wrote:
> > >>>>>>>> Unprivileged users should not be able to supply security labels
> > >>>>>>>> in filesystems, nor should they be able to supply security
> > >>>>>>>> contexts in unprivileged mounts. For any mount where s_user_ns is
> > >>>>>>>> not init_user_ns, force the use of SECURITY_FS_USE_NONE behavior
> > >>>>>>>> and return EPERM if any contexts are supplied in the mount
> > >>>>>>>> options.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Seth Forshee <seth.forshee@...onical.com>
> > >>>>>>>
> > >>>>>>> I think this is obsoleted by the subsequent discussion, but just for the
> > >>>>>>> record: this patch would cause the files in the userns mount to be left
> > >>>>>>> with the "unlabeled" label, and therefore under typical policies,
> > >>>>>>> completely inaccessible to any process in a confined domain.
> > >>>>>>
> > >>>>>> The right way to handle this for SELinux would be to automatically use
> > >>>>>> mountpoint labeling (SECURITY_FS_USE_MNTPOINT, normally set by
> > >>>>>> specifying a context= mount option), with the sbsec->mntpoint_sid set
> > >>>>>> from some related object (e.g. the block device file context, as in your
> > >>>>>> patches for Smack).  That will cause SELinux to use that value instead
> > >>>>>> of any xattr value from the filesystem and will cause attempts by
> > >>>>>> userspace to set the security.selinux xattr to fail on that filesystem.
> > >>>>>>  That is how SELinux normally deals with untrusted filesystems, except
> > >>>>>> that it is normally specified as a mount option by a trusted mounting
> > >>>>>> process, whereas in your case you need to automatically set it.
> > >>>>>
> > >>>>> Excellent, thank you for the advice. I'll start on this when I've
> > >>>>> finished with Smack.
> > >>>>
> > >>>> Not tested, but something like this should work. Note that it should
> > >>>> come after the call to security_fs_use() so we know whether SELinux
> > >>>> would even try to use xattrs supplied by the filesystem in the first place.
> > >>>>
> > >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > >>>> index 564079c..84da3a2 100644
> > >>>> --- a/security/selinux/hooks.c
> > >>>> +++ b/security/selinux/hooks.c
> > >>>> @@ -745,6 +745,30 @@ static int selinux_set_mnt_opts(struct super_block *sb,
> > >>>>                         goto out;
> > >>>>                 }
> > >>>>         }
> > >>>> +
> > >>>> +       /*
> > >>>> +        * If this is a user namespace mount, no contexts are allowed
> > >>>> +        * on the command line and security labels must be ignored.
> > >>>> +        */
> > >>>> +       if (sb->s_user_ns != &init_user_ns) {
> > >>>> +               if (context_sid || fscontext_sid || rootcontext_sid ||
> > >>>> +                   defcontext_sid) {
> > >>>> +                       rc = -EACCES;
> > >>>> +                       goto out;
> > >>>> +               }
> > >>>> +               if (sbsec->behavior == SECURITY_FS_USE_XATTR) {
> > >>>> +                       struct block_device *bdev = sb->s_bdev;
> > >>>> +                       sbsec->behavior = SECURITY_FS_USE_MNTPOINT;
> > >>>> +                       if (bdev) {
> > >>>> +                               struct inode_security_struct *isec =
> > >>>> bdev->bd_inode;
> > >>>
> > >>> That should be bdev->bd_inode->i_security.
> > >>
> > >> Sorry, this won't work.  bd_inode is not the inode of the block device
> > >> file that was passed to mount, and it isn't labeled in any way.  It will
> > >> just be unlabeled.
> > >>
> > >> So I guess the only real option here as a fallback is
> > >> sbsec->mntpoint_sid = current_sid().  Which isn't great either, as the
> > >> only case where we currently assign task labels to files is for their
> > >> /proc/pid inodes, and no current policy will therefore allow create
> > >> permission to such files.
> > > 
> > > Darn, you're right, that isn't the inode we want. There really doesn't
> > > seem to be any way to get back to the one we want from the LSM, short of
> > > adding a new hook.
> > 
> > Maybe list_first_entry(&sb->s_bdev->bd_inodes, struct inode, i_devices)?
> > Feels like a layering violation though...
> 
> Yeah, and even though that probably works out to be the inode we want in
> most cases I don't think we can be absolutely certain that it is. Maybe
> there's some way we could walk the list and be sure we've found the
> right inode, but I'm not seeing it.

I guess we could do something like this (note that most of the changes
here are just to give a version of blkdev_get_by_path which takes a
struct path * so that the filename lookup doesn't have to be done
twice). Basically add a new hook that informs the security module of the
inode for the backing device file passed to mount and call that from
mount_bdev. The security module could grab a reference to the inode and
stash it away.

Something else to note is that, as I have it here, the hook would end up
getting called for every mount of a given block device, not just the
first. So it's possible the security module could see the hook called a
second time with a different inode that has a different label. The hook
could be changed to return int if you wanted to have the opportunity to
reject such mounts.

Seth

---

diff --git a/fs/block_dev.c b/fs/block_dev.c
index f8ce371c437c..dc2173e24e30 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1372,14 +1372,39 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
 }
 EXPORT_SYMBOL(blkdev_get);
 
+static struct block_device *__lookup_bdev(struct path *path);
+
+struct block_device * __blkdev_get_by_path(struct path *path, fmode_t mode,
+					   void *holder)
+{
+	struct block_device *bdev;
+	int err;
+
+	bdev = __lookup_bdev(path);
+	if (IS_ERR(bdev))
+		return bdev;
+
+	err = blkdev_get(bdev, mode, holder);
+	if (err)
+		return ERR_PTR(err);
+
+	if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
+		blkdev_put(bdev, mode);
+		return ERR_PTR(-EACCES);
+	}
+
+	return bdev;
+}
+EXPORT_SYMBOL(__blkdev_get_by_path);
+
 /**
  * blkdev_get_by_path - open a block device by name
- * @path: path to the block device to open
+ * @pathname: path to the block device to open
  * @mode: FMODE_* mask
  * @holder: exclusive holder identifier
  *
- * Open the blockdevice described by the device file at @path.  @mode
- * and @holder are identical to blkdev_get().
+ * Open the blockdevice described by the device file at @pathname.
+ * @mode and @holder are identical to blkdev_get().
  *
  * On success, the returned block_device has reference count of one.
  *
@@ -1389,25 +1414,22 @@ EXPORT_SYMBOL(blkdev_get);
  * RETURNS:
  * Pointer to block_device on success, ERR_PTR(-errno) on failure.
  */
-struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
+struct block_device *blkdev_get_by_path(const char *pathname, fmode_t mode,
 					void *holder)
 {
 	struct block_device *bdev;
-	int err;
-
-	bdev = lookup_bdev(path);
-	if (IS_ERR(bdev))
-		return bdev;
+	struct path path;
+	int error;
 
-	err = blkdev_get(bdev, mode, holder);
-	if (err)
-		return ERR_PTR(err);
+	if (!pathname || !*pathname)
+		return ERR_PTR(-EINVAL);
 
-	if ((mode & FMODE_WRITE) && bdev_read_only(bdev)) {
-		blkdev_put(bdev, mode);
-		return ERR_PTR(-EACCES);
-	}
+	error = kern_path(pathname, LOOKUP_FOLLOW, &path);
+	if (error)
+		return ERR_PTR(error);
 
+	bdev = __blkdev_get_by_path(&path, mode, holder);
+	path_put(&path);
 	return bdev;
 }
 EXPORT_SYMBOL(blkdev_get_by_path);
@@ -1702,6 +1724,30 @@ int ioctl_by_bdev(struct block_device *bdev, unsigned cmd, unsigned long arg)
 
 EXPORT_SYMBOL(ioctl_by_bdev);
 
+static struct block_device *__lookup_bdev(struct path *path)
+{
+	struct block_device *bdev;
+	struct inode *inode;
+	int error;
+
+	inode = d_backing_inode(path->dentry);
+	error = -ENOTBLK;
+	if (!S_ISBLK(inode->i_mode))
+		goto fail;
+	error = -EACCES;
+	if (!may_open_dev(path))
+		goto fail;
+	error = -ENOMEM;
+	bdev = bd_acquire(inode);
+	if (!bdev)
+		goto fail;
+out:
+	return bdev;
+fail:
+	bdev = ERR_PTR(error);
+	goto out;
+}
+
 /**
  * lookup_bdev  - lookup a struct block_device by name
  * @pathname:	special file representing the block device
@@ -1713,7 +1759,6 @@ EXPORT_SYMBOL(ioctl_by_bdev);
 struct block_device *lookup_bdev(const char *pathname)
 {
 	struct block_device *bdev;
-	struct inode *inode;
 	struct path path;
 	int error;
 
@@ -1724,23 +1769,9 @@ struct block_device *lookup_bdev(const char *pathname)
 	if (error)
 		return ERR_PTR(error);
 
-	inode = d_backing_inode(path.dentry);
-	error = -ENOTBLK;
-	if (!S_ISBLK(inode->i_mode))
-		goto fail;
-	error = -EACCES;
-	if (!may_open_dev(&path))
-		goto fail;
-	error = -ENOMEM;
-	bdev = bd_acquire(inode);
-	if (!bdev)
-		goto fail;
-out:
+	bdev = __lookup_bdev(&path);
 	path_put(&path);
 	return bdev;
-fail:
-	bdev = ERR_PTR(error);
-	goto out;
 }
 EXPORT_SYMBOL(lookup_bdev);
 
diff --git a/fs/super.c b/fs/super.c
index 008f938e3ec0..558f7845a171 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -34,6 +34,7 @@
 #include <linux/fsnotify.h>
 #include <linux/lockdep.h>
 #include <linux/user_namespace.h>
+#include <linux/namei.h>
 #include "internal.h"
 
 
@@ -980,15 +981,26 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 {
 	struct block_device *bdev;
 	struct super_block *s;
+	struct path path;
+	struct inode *inode;
 	fmode_t mode = FMODE_READ | FMODE_EXCL;
 	int error = 0;
 
 	if (!(flags & MS_RDONLY))
 		mode |= FMODE_WRITE;
 
-	bdev = blkdev_get_by_path(dev_name, mode, fs_type);
-	if (IS_ERR(bdev))
-		return ERR_CAST(bdev);
+	if (!dev_name || !*dev_name)
+		return ERR_PTR(-EINVAL);
+
+	error = kern_path(dev_name, LOOKUP_FOLLOW, &path);
+	if (error)
+		return ERR_PTR(error);
+
+	bdev = __blkdev_get_by_path(&path, mode, fs_type);
+	if (IS_ERR(bdev)) {
+		error = PTR_ERR(bdev);
+		goto error;
+	}
 
 	/*
 	 * once the super is inserted into the list by sget, s_umount
@@ -1040,6 +1052,10 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 		bdev->bd_super = s;
 	}
 
+	inode = d_backing_inode(path.dentry);
+	security_sb_backing_dev(s, inode);
+	path_put(&path);
+
 	return dget(s->s_root);
 
 error_s:
@@ -1047,6 +1063,7 @@ error_s:
 error_bdev:
 	blkdev_put(bdev, mode);
 error:
+	path_put(&path);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL(mount_bdev);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4597420ab933..3748945bf0d5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2315,6 +2315,8 @@ extern int ioctl_by_bdev(struct block_device *, unsigned, unsigned long);
 extern int blkdev_ioctl(struct block_device *, fmode_t, unsigned, unsigned long);
 extern long compat_blkdev_ioctl(struct file *, unsigned, unsigned long);
 extern int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder);
+extern struct block_device *__blkdev_get_by_path(struct path *path, fmode_t mode,
+						 void *holder);
 extern struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
 					       void *holder);
 extern struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9429f054c323..52ce1a094e04 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1351,6 +1351,7 @@ union security_list_options {
 	int (*sb_clone_mnt_opts)(const struct super_block *oldsb,
 					struct super_block *newsb);
 	int (*sb_parse_opts_str)(char *options, struct security_mnt_opts *opts);
+	void (*sb_backing_dev)(struct super_block *sb, struct inode *inode);
 	int (*dentry_init_security)(struct dentry *dentry, int mode,
 					struct qstr *name, void **ctx,
 					u32 *ctxlen);
@@ -1648,6 +1649,7 @@ struct security_hook_heads {
 	struct list_head sb_set_mnt_opts;
 	struct list_head sb_clone_mnt_opts;
 	struct list_head sb_parse_opts_str;
+	struct list_head sb_backing_dev;
 	struct list_head dentry_init_security;
 #ifdef CONFIG_SECURITY_PATH
 	struct list_head path_unlink;
diff --git a/include/linux/security.h b/include/linux/security.h
index 79d85ddf8093..7a4d8382af20 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -231,6 +231,7 @@ int security_sb_set_mnt_opts(struct super_block *sb,
 int security_sb_clone_mnt_opts(const struct super_block *oldsb,
 				struct super_block *newsb);
 int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);
+void security_sb_backing_dev(struct super_block *sb, struct inode *inode);
 int security_dentry_init_security(struct dentry *dentry, int mode,
 					struct qstr *name, void **ctx,
 					u32 *ctxlen);
@@ -562,6 +563,10 @@ static inline int security_sb_parse_opts_str(char *options, struct security_mnt_
 	return 0;
 }
 
+static inline void security_sb_backing_dev(struct super_block *sb,
+					   struct inode *inode)
+{ }
+
 static inline int security_inode_alloc(struct inode *inode)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 062f3c997fdc..f6f89e0f06d8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -347,6 +347,11 @@ int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
 }
 EXPORT_SYMBOL(security_sb_parse_opts_str);
 
+void security_sb_backing_dev(struct super_block *sb, struct inode *inode)
+{
+	call_void_hook(sb_backing_dev, sb, inode);
+}
+
 int security_inode_alloc(struct inode *inode)
 {
 	inode->i_security = NULL;
@@ -1595,6 +1600,8 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.sb_clone_mnt_opts),
 	.sb_parse_opts_str =
 		LIST_HEAD_INIT(security_hook_heads.sb_parse_opts_str),
+	.sb_backing_dev =
+		LIST_HEAD_INIT(security_hook_heads.sb_backing_dev),
 	.dentry_init_security =
 		LIST_HEAD_INIT(security_hook_heads.dentry_init_security),
 #ifdef CONFIG_SECURITY_PATH
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists