Messages in this thread Patch in this message | | | Date | Mon, 13 Dec 2021 11:25:28 -0500 | Subject | Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace | From | Stefan Berger <> |
| |
On 12/13/21 10:50, Christian Brauner wrote: > On Mon, Dec 13, 2021 at 10:33:40AM -0500, Stefan Berger wrote: >> On 12/11/21 04:50, Christian Brauner wrote: >>> On Fri, Dec 10, 2021 at 08:57:11AM -0500, Stefan Berger wrote: >>>> >>>> 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 <christian.brauner@ubuntu.com> >>> 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. > >> 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);
| |