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