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

[Christian Brauner]

On Mon, Dec 13, 2021 at 11:25:28AM -0500, Stefan Berger wrote:
> 
> 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 at 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;

Please consider
https://lore.kernel.org/lkml/20211211104527.7cp4mbznjpcijfqx@wittgenstein
otherwise looks ok.