Message-ID: <dca4e7c9-87a7-9a9e-b1f2-df16f1a45019@linux.ibm.com>
Date:   Fri, 10 Dec 2021 08:57:11 -0500
From:   Stefan Berger <stefanb@...ux.ibm.com>
To:     Christian Brauner <christian.brauner@...ntu.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 12/10/21 06:32, Christian Brauner wrote:
> On Thu, Dec 09, 2021 at 07:57:02PM -0500, Stefan Berger wrote:
>> On 12/9/21 14:11, Christian Brauner wrote:
>>>   From 1f03dc427c583d5e9ebc9ebe9de77c3c535bbebe Mon Sep 17 00:00:00 2001
>>> From: Christian Brauner <christian.brauner@...ntu.com>
>>> Date: Thu, 9 Dec 2021 20:07:02 +0100
>>> Subject: [PATCH] !!!! HERE BE DRAGONS - UNTESTED !!!!
>>>
>>> ---
>>>    security/integrity/ima/ima_fs.c | 43 +++++++++++++++++++++++++++++----
>>>    1 file changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>>> index 583462b29cb5..d5b302b925b8 100644
>>> --- a/security/integrity/ima/ima_fs.c
>>> +++ b/security/integrity/ima/ima_fs.c
>>> @@ -317,10 +317,14 @@ static ssize_t ima_read_policy(char *path)
>>>    static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>>>    				size_t datalen, loff_t *ppos)
>>>    {
>>> -	struct ima_namespace *ns = get_current_ns();
>>> +	struct ima_namespace *ns;
>>> +	struct user_namespace *user_ns;
>>>    	char *data;
>>>    	ssize_t result;
>>> +	user_ns = ima_filp_private(filp);
>>> +	ns = user_ns->ima_ns
>>> +
>>>    	if (datalen >= PAGE_SIZE)
>>>    		datalen = PAGE_SIZE - 1;
>>> @@ -373,26 +377,51 @@ static const struct seq_operations ima_policy_seqops = {
>>>    };
>>>    #endif
>>> +static struct user_namespace *ima_filp_private(struct file *filp)
>>> +{
>>> +	if (!(filp->f_flags & O_WRONLY)) {
>>> +#ifdef CONFIG_IMA_READ_POLICY
>>> +		struct seq_file *seq;
>>> +
>>> +		seq = filp->private_data;
>>> +		return seq->private;
>>> +#endif
>>> +	}
>>> +	return filp->private_data;
>>> +}
>>> +
>>>    /*
>>>     * ima_open_policy: sequentialize access to the policy file
>>>     */
>>>    static int ima_open_policy(struct inode *inode, struct file *filp)
>>>    {
>>> -	struct ima_namespace *ns = get_current_ns();
>>> +	struct user_namespace *user_ns = current_user_ns();
>>
>> Do we have to take a reference on the user namespace assuming one can open
>> the file, pass the fd down the hierarchy, and then the user namespace with
>> the opened file goes away? Or is there anything else that keeps the user
>> namespace alive?
> No, we don't. When ima_policy_open() is called we do current_user_ns()
> but that will be guaranteed to be identical to filp->f_cred->user_ns.
> And f_cred is a reference that has been taken when the vfs allocated a
> struct file for this .open call so won't go away until the last fput.
>
> My proposal is also too complicated, I think.
> (The booster is giving me the same side-effects as my second shot so
> this looks like two good days of fever and headache. So I'll use that as
> an excuse. :))
>
> Your patch series as it stands has a bit of a security issue with those
> get_current_ns() calls across differnet file/seq_file operations.
>
> You have to make an architectural decision, I think. I see two sensible
> options:
> 1. The relevant ima_ns that .open/.read/.write operate on is always taken
>     to be the ima_ns of the filesystem's userns, i.e.
>     sb->s_user_ns->ima_ns.
>     This - but I'm not an ima person - makes the most sense to me and the
>     semantics are straightforward. If I write to a file to alter some
>     policy then I expect the ima namespace of the user namespace to be
>     affected that the securityfs instance was mounted in.
> 2. The relevant ima_ns that .open/.read/.write operate on is always
>     taken to be the one of the opener. I don't really like that as that
>     gets weird if for some complicated reason the caller is not located
>     in the userns the filesystem was mounted in (weird mount propagation
>     scenario or sm). It also feels strange to operate on an ima_ns that's
>     different from s_user_ns->ima_ns in a securityfs instance.

We have this situation because one can setns() to another mount 
namespaces but the data shown by SecurityFS lives in a user namespace, 
right? And now we need to decide whether to affect the data in the user 
namespace  that did the open (option 2) or to which the SecurityFS 
belongs to (option 1). If we were to open a regular file it would be 
option 1, so we should probably not break that existing semantic and 
also choose option 1 unless there one wasn't allowed to choose the user 
namespace the SecurityFS files belonged to then it should be option 2 
but then we have file descriptor passing where 'being allowed' can 
change depending on who is reading/writing a file... Is 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?

Following man page of setns:

"   User namespaces
               A process reassociating itself with a user namespace must
               have the CAP_SYS_ADMIN capability in the target user
               namespace.  (This necessarily implies that it is only
               possible to join a descendant user namespace.)  Upon
               successfully joining a user namespace, a process is
               granted all capabilities in that namespace, regardless of
               its user and group IDs."


So if we choose option 1 maybe we have to test for this capability upon 
every read/write from/to a file?


    Stefan


>
> So I think I would propose you do sm:
>
>  From 6c8018f14f22e7bc2255dcebab96f9b8c39a8459 Mon Sep 17 00:00:00 2001
> From: Christian Brauner <christian.brauner@...ntu.com>
> Date: Fri, 10 Dec 2021 10:31:27 +0100
> Subject: [PATCH 1/2] !!!! HERE BE DRAGONS - COMPLETELY UNTESTED !!!!
>
> The relevant ima_ns that .open/.read/.write operate on is always taken to be
> the ima_ns of the filesystem's userns, i.e.  sb->s_user_ns->ima_ns. This - but
> I'm not an ima person - makes the most sense to me and the semantics are
> straightforward. If I write to a file to alter some policy then I expect the
> ima namespace of the user namespace to be affected that the securityfs instance
> was mounted in.
> ---
>   security/integrity/ima/ima_fs.c     | 30 ++++++++++++++++++-----------
>   security/integrity/ima/ima_policy.c |  8 ++++++--
>   2 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 778983fd9a73..95b7b9ec2a76 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -49,7 +49,8 @@ static ssize_t ima_show_htable_violations(struct file *filp,
>   					  char __user *buf,
>   					  size_t count, loff_t *ppos)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	struct user_namespace *user_ns = filp->f_path.mnt->mnt_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   
>   	return ima_show_htable_value(buf, count, ppos, &ns->ima_htable.violations);
>   }
> @@ -63,7 +64,8 @@ static ssize_t ima_show_measurements_count(struct file *filp,
>   					   char __user *buf,
>   					   size_t count, loff_t *ppos)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	struct user_namespace *user_ns = filp->f_path.mnt->mnt_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   
>   	return ima_show_htable_value(buf, count, ppos, &ns->ima_htable.len);
>   }
> @@ -76,7 +78,9 @@ static const struct file_operations ima_measurements_count_ops = {
>   /* returns pointer to hlist_node */
>   static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	const struct file *filp = m->file;
> +	struct user_namespace *user_ns = filp->f_path.mnt->mnt_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   	loff_t l = *pos;
>   	struct ima_queue_entry *qe;
>   
> @@ -94,7 +98,9 @@ static void *ima_measurements_start(struct seq_file *m, loff_t *pos)
>   
>   static void *ima_measurements_next(struct seq_file *m, void *v, loff_t *pos)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	const struct file *filp = m->file;
> +	struct user_namespace *user_ns = filp->f_path.mnt->mnt_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   	struct ima_queue_entry *qe = v;
>   
>   	/* lock protects when reading beyond last element
> @@ -273,9 +279,8 @@ static const struct file_operations ima_ascii_measurements_ops = {
>   	.release = seq_release,
>   };
>   
> -static ssize_t ima_read_policy(char *path)
> +static ssize_t ima_read_policy(struct ima_namespace *ns, char *path)
>   {
> -	struct ima_namespace *ns = get_current_ns();
>   	void *data = NULL;
>   	char *datap;
>   	size_t size;
> @@ -317,7 +322,8 @@ static ssize_t ima_read_policy(char *path)
>   static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>   				size_t datalen, loff_t *ppos)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	struct user_namespace *user_ns = file->f_path.mnt->mnt_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   	char *data;
>   	ssize_t result;
>   
> @@ -340,7 +346,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>   		goto out_free;
>   
>   	if (data[0] == '/') {
> -		result = ima_read_policy(data);
> +		result = ima_read_policy(ns, data);
>   	} else if (ima_appraise & IMA_APPRAISE_POLICY) {
>   		pr_err("signed policy file (specified as an absolute pathname) required\n");
>   		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> @@ -378,7 +384,8 @@ static const struct seq_operations ima_policy_seqops = {
>    */
>   static int ima_open_policy(struct inode *inode, struct file *filp)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	struct user_namespace *user_ns = inode->i_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   
>   	if (!(filp->f_flags & O_WRONLY)) {
>   #ifndef	CONFIG_IMA_READ_POLICY
> @@ -386,7 +393,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>   #else
>   		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
>   			return -EACCES;
> -		if (!mac_admin_ns_capable(ns->user_ns))
> +		if (!mac_admin_ns_capable(user_ns))
>   			return -EPERM;
>   		return seq_open(filp, &ima_policy_seqops);
>   #endif
> @@ -405,7 +412,8 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>    */
>   static int ima_release_policy(struct inode *inode, struct file *file)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	struct user_namespace *user_ns = inode->i_sb->s_user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   	const char *cause = ns->valid_policy ? "completed" : "failed";
>   
>   	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 747dca6131d6..41e5f17ec44d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1908,7 +1908,9 @@ static const char *const mask_tokens[] = {
>   
>   void *ima_policy_start(struct seq_file *m, loff_t *pos)
>   {
> -	struct ima_namespace *ns = get_current_ns();
> +	const struct file *filp = m->file;
> +	struct user_namespace *user_ns = filp->f_path.mnt->mnt_sb->user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   	loff_t l = *pos;
>   	struct ima_rule_entry *entry;
>   	struct list_head *ima_rules_tmp;
> @@ -1928,7 +1930,9 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos)
>   void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
>   {
>   	struct ima_rule_entry *entry = v;
> -	struct ima_namespace *ns = get_current_ns();
> +	const struct file *filp = m->file;
> +	struct user_namespace *user_ns = filp->f_path.mnt->mnt_sb->user_ns;
> +	struct ima_namespace *ns = user_ns->ima_ns;
>   
>   	rcu_read_lock();
>   	entry = list_entry_rcu(entry->list.next, struct ima_rule_entry, list);

Powered by blists - more mailing lists