On Mon, Dec 13, 2021 at 04:50:20PM +0100, 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!
If a privileged process opens an fd refering to a struct file for the

s/a privileged process/a process that is located in an ancestor userns
of the securityfs instance

root inode and leaks it to an unprivileged process by accident the

s/unprivileged process/process located in a descendant userns

unprivileged process can open any file or directory beneath via openat()
and friends.