Messages in this thread | | | Date | Mon, 13 Dec 2021 10:33:40 -0500 | Subject | Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace | From | Stefan Berger <> |
| |
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.
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...
> + .open = dcache_dir_open, > + .release = dcache_dir_close, > + .llseek = dcache_dir_lseek, > + .read = generic_read_dir, > + .iterate_shared = dcache_readdir, > + .fsync = noop_fsync, > +}; > + > /** > * securityfs_create_dentry - create a dentry in the securityfs filesystem > * > @@ -167,8 +196,8 @@ 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_fop = &simple_dir_operations; > + inode->i_op = &securityfs_dir_inode_operations; > + inode->i_fop = &securityfs_dir_operations; > inc_nlink(inode); > inc_nlink(dir); > } else if (S_ISLNK(mode)) {
| |