Message-ID: <85b75c98-6452-9706-7549-10b416350b7d@linux.ibm.com>
Date:   Mon, 13 Dec 2021 10:33:40 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related
 variables into ima_namespace
Content-Language: en-US
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        serge@hallyn.com, containers@lists.linux.dev,
        dmitry.kasatkin@gmail.com, ebiederm@xmission.com,
        krzysztof.struczynski@huawei.com, roberto.sassu@huawei.com,
        mpeters@redhat.com, lhinds@redhat.com, lsturman@redhat.com,
        puiterwi@redhat.com, jejb@linux.ibm.com, jamjoom@us.ibm.com,
        linux-kernel@vger.kernel.org, paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
References: <20211208221818.1519628-1-stefanb@linux.ibm.com>
 <20211208221818.1519628-14-stefanb@linux.ibm.com>
 <20211209191109.o3x7nynnm52zhygz@wittgenstein>
 <0ab33fbc-8438-27b6-ff4c-0321bfc73855@linux.ibm.com>
 <20211210113244.odv2ibrifz2jzft5@wittgenstein>
 <dca4e7c9-87a7-9a9e-b1f2-df16f1a45019@linux.ibm.com>
 <20211211095026.i2gvqjy4df3sxq42@wittgenstein>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20211211095026.i2gvqjy4df3sxq42@wittgenstein>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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)) {