Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace

[Christian Brauner <christian.brauner@ubuntu.com>]

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:
> > 
> > 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!
> 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.