linux-kernel - Re: [PATCH v12 23/26] ima: Show owning user namespace's uid and gid when displaying policy

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: <20220522175452.GB24519@mail.hallyn.com>
Date:   Sun, 22 May 2022 12:54:52 -0500
From:   "Serge E. Hallyn" <serge@...lyn.com>
To:     Stefan Berger <stefanb@...ux.ibm.com>
Cc:     linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
        serge@...lyn.com, christian.brauner@...ntu.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,
        jpenumak@...hat.com
Subject: Re: [PATCH v12 23/26] ima: Show owning user namespace's uid and gid
 when displaying policy

On Wed, Apr 20, 2022 at 10:06:30AM -0400, Stefan Berger wrote:
> Show the uid and gid values relative to the user namespace that is
> currently active. The effect of this changes is that when one displays

When you say "is currently active", in my mind it's not clear whether you
mean in the process which opened the seq_file, or is active in the ima_ns,
or the reader (which might I guess be differenet still).  The code of
course does make it clear.  Can you change it to say "the user namespace
which opened the policy_show file" or something like that?

Also, s/The effect of this changes/The effect of this change/.

> the policy from the user namespace that originally set the policy,
> the same uid and gid values are shown in the policy as those that were
> used when the policy was set.
> 
> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@...ux.ibm.com>
> 

Reviewed-by: Serge Hallyn <serge@...lyn.com>

> ---
> v9:
>   - use seq_user_ns and from_k{g,u}id_munged()
> ---
>  security/integrity/ima/ima_policy.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index eb10d895923d..4f8c50ddb777 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -2018,6 +2018,7 @@ static void ima_policy_show_appraise_algos(struct seq_file *m,
>  
>  int ima_policy_show(struct seq_file *m, void *v)
>  {
> +	struct user_namespace *user_ns = seq_user_ns(m);
>  	struct ima_rule_entry *entry = v;
>  	int i;
>  	char tbuf[64] = {0,};
> @@ -2103,7 +2104,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	}
>  
>  	if (entry->flags & IMA_UID) {
> -		snprintf(tbuf, sizeof(tbuf), "%d", __kuid_val(entry->uid));
> +		snprintf(tbuf, sizeof(tbuf),
> +			 "%d", from_kuid_munged(user_ns, entry->uid));
>  		if (entry->uid_op == &uid_gt)
>  			seq_printf(m, pt(Opt_uid_gt), tbuf);
>  		else if (entry->uid_op == &uid_lt)
> @@ -2114,7 +2116,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	}
>  
>  	if (entry->flags & IMA_EUID) {
> -		snprintf(tbuf, sizeof(tbuf), "%d", __kuid_val(entry->uid));
> +		snprintf(tbuf, sizeof(tbuf),
> +			 "%d", from_kuid_munged(user_ns, entry->uid));
>  		if (entry->uid_op == &uid_gt)
>  			seq_printf(m, pt(Opt_euid_gt), tbuf);
>  		else if (entry->uid_op == &uid_lt)
> @@ -2125,7 +2128,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	}
>  
>  	if (entry->flags & IMA_GID) {
> -		snprintf(tbuf, sizeof(tbuf), "%d", __kgid_val(entry->gid));
> +		snprintf(tbuf, sizeof(tbuf),
> +			 "%d", from_kgid_munged(user_ns, entry->gid));
>  		if (entry->gid_op == &gid_gt)
>  			seq_printf(m, pt(Opt_gid_gt), tbuf);
>  		else if (entry->gid_op == &gid_lt)
> @@ -2136,7 +2140,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	}
>  
>  	if (entry->flags & IMA_EGID) {
> -		snprintf(tbuf, sizeof(tbuf), "%d", __kgid_val(entry->gid));
> +		snprintf(tbuf, sizeof(tbuf),
> +			 "%d", from_kgid_munged(user_ns, entry->gid));
>  		if (entry->gid_op == &gid_gt)
>  			seq_printf(m, pt(Opt_egid_gt), tbuf);
>  		else if (entry->gid_op == &gid_lt)
> @@ -2147,7 +2152,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	}
>  
>  	if (entry->flags & IMA_FOWNER) {
> -		snprintf(tbuf, sizeof(tbuf), "%d", __kuid_val(entry->fowner));
> +		snprintf(tbuf, sizeof(tbuf),
> +			 "%d", from_kuid_munged(user_ns, entry->fowner));
>  		if (entry->fowner_op == &uid_gt)
>  			seq_printf(m, pt(Opt_fowner_gt), tbuf);
>  		else if (entry->fowner_op == &uid_lt)
> @@ -2158,7 +2164,8 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	}
>  
>  	if (entry->flags & IMA_FGROUP) {
> -		snprintf(tbuf, sizeof(tbuf), "%d", __kgid_val(entry->fgroup));
> +		snprintf(tbuf, sizeof(tbuf),
> +			 "%d", from_kgid_munged(user_ns, entry->fgroup));
>  		if (entry->fgroup_op == &gid_gt)
>  			seq_printf(m, pt(Opt_fgroup_gt), tbuf);
>  		else if (entry->fgroup_op == &gid_lt)
> -- 
> 2.34.1