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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211213160337.5uc7ifhysk4hdao7@wittgenstein>
Date:   Mon, 13 Dec 2021 17:03:37 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Stefan Berger <stefanb@...ux.ibm.com>
Cc:     linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
        serge@...lyn.com, containers@...ts.linux.dev,
        dmitry.kasatkin@...il.com, ebiederm@...ssion.com,
        krzysztof.struczynski@...wei.com, roberto.sassu@...wei.com,
        mpeters@...hat.com, lhinds@...hat.com, lsturman@...hat.com,
        puiterwi@...hat.com, jejb@...ux.ibm.com, jamjoom@...ibm.com,
        linux-kernel@...r.kernel.org, paul@...l-moore.com, rgb@...hat.com,
        linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem
 related variables into ima_namespace

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@...ntu.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.