On 12/13/21 10:50, Christian Brauner wrote:

On Mon, Dec 13, 2021 at 10:33:40AM -0500, Stefan Berger wrote:

quoted

On 12/11/21 04:50, Christian Brauner wrote:

quoted

On Fri, Dec 10, 2021 at 08:57:11AM -0500, Stefan Berger wrote:

quoted

there anything that would prevent us from setns()'ing to that target user
namespace so that we would now see that of a user namespace that we are not
allowed to see?

If you're really worried about someone being able to access a securityfs
instance whose userns doesn't match the userns the securityfs instance
was mounted in there are multiple ways to fix it. The one that I tend to
prefer is:

  From e0ff6a8dcc573763568e685dd70d1547efd68df9 Mon Sep 17 00:00:00 2001
From: Christian Brauner <redacted>
Date: Fri, 10 Dec 2021 11:47:37 +0100
Subject: !!!! HERE BE DRAGONS - COMPLETELY UNTESTED !!!!

securityfs: only allow access to securityfs from within same namespace

Limit opening of securityfs files to callers located in the same namespace.

---
   security/inode.c | 33 +++++++++++++++++++++++++++++++--
   1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index eaccba7017d9..9eaf757c08cb 100644
--- a/security/inode.c
+++ b/security/inode.c

@@ -80,6 +80,35 @@ static struct file_system_type fs_type = {
   	.fs_flags =	FS_USERNS_MOUNT,
   };
+static int securityfs_permission(struct user_namespace *mnt_userns,
+				 struct inode *inode, int mask)
+{
+	int err;
+
+	err = generic_permission(&init_user_ns, inode, mask);
+	if (!err) {
+		if (inode->i_sb->s_user_ns != current_user_ns())
+			err = -EACCES;
+	}
+
+	return err;
+}
+
+const struct inode_operations securityfs_dir_inode_operations = {
+	.permission	= securityfs_permission,
+	.lookup		= simple_lookup,
+};
+
+const struct file_operations securityfs_dir_operations = {
+	.permission	= securityfs_permission,

This interface function on file operations doesn't exist.

It's almost as if the subject line of this patch warned about its draft
character. That was supposed for regular files.

quoted

I'll use the inode_operations and also hook it to the root dentry of the
super_block. Then there's no need to have this check on symlinks and
files...

Don't special case the inode_operations for the root inode!

I modified the inode_operations *also* for the root node, since that one 
is initialized with &simple_dir_inode_operationsby simple_fill_super, so 
I didn't want to miss it...

If a privileged process opens an fd refering to a struct file for the
root inode and leaks it to an unprivileged process by accident the
unprivileged process can open any file or directory beneath via openat()
and friends.

Symlinks don't need a .permission handler anyway because they just
contain the name of another file and that is checked for .permission
unless you have a separate .getlink handler where you want to restrict
link contents further.

But regular files need to have a .permission method see openat().

So here's what I have now for the hardening.

diff --git a/security/inode.c b/security/inode.c
index fee01ff4d831..a0d9f086e3d5 100644
--- a/security/inode.c
+++ b/security/inode.c

@@ -26,6 +26,29 @@
  static struct vfsmount *init_securityfs_mount;
  static int init_securityfs_mount_count;

+static int securityfs_permission(struct user_namespace *mnt_userns,
+                                struct inode *inode, int mask)
+{
+       int err;
+
+       err = generic_permission(&init_user_ns, inode, mask);
+       if (!err) {
+               if (inode->i_sb->s_user_ns != current_user_ns())
+                       err = -EACCES;
+       }
+
+       return err;
+}
+
+const struct inode_operations securityfs_dir_inode_operations = {
+       .permission     = securityfs_permission,
+       .lookup         = simple_lookup,
+};
+
+const struct inode_operations securityfs_file_inode_operations = {
+       .permission     = securityfs_permission,
+};
+
  static void securityfs_free_inode(struct inode *inode)
  {
         if (S_ISLNK(inode->i_mode))

@@ -41,20 +64,25 @@ static const struct super_operations 

securityfs_super_operations = {
  static int securityfs_fill_super(struct super_block *sb, struct 
fs_context *fc)
  {
         static const struct tree_descr files[] = {{""}};
+       struct user_namespace *ns = fc->user_ns;
         int error;

+       if (WARN_ON(ns != current_user_ns()))
+               return -EINVAL;
+
         error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
         if (error)
                 return error;

         sb->s_op = &securityfs_super_operations;
+       sb->s_root->d_inode->i_op = &securityfs_dir_inode_operations;

         return 0;
  }
[...]

@@ -157,7 +186,7 @@ static struct dentry *securityfs_create_dentry(const 

char *name, umode_t mode,
         inode->i_atime = inode->i_mtime = inode->i_ctime = 
current_time(inode);
         inode->i_private = data;
         if (S_ISDIR(mode)) {
-               inode->i_op = &simple_dir_inode_operations;
+               inode->i_op = &securityfs_dir_inode_operations;
                 inode->i_fop = &simple_dir_operations;
                 inc_nlink(inode);
                 inc_nlink(dir);

@@ -165,10 +194,10 @@ static struct dentry 

*securityfs_create_dentry(const char *name, umode_t mode,
                 inode->i_op = iops ? iops : 
&simple_symlink_inode_operations;
                 inode->i_link = data;
         } else {
+               inode->i_op = &securityfs_file_inode_operations;
                 inode->i_fop = fops;
         }
         d_instantiate(dentry, inode);