diff mbox series

[v9,22/23] ima: Show owning user namespace's uid and gid when displaying policy

Message ID 20220125224645.79319-23-stefanb@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show
Series ima: Namespace IMA with audit support in IMA-ns | expand

Commit Message

Stefan Berger Jan. 25, 2022, 10:46 p.m. UTC
From: Stefan Berger <stefanb@linux.ibm.com>

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
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@linux.ibm.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(-)

Comments

Christian Brauner Jan. 26, 2022, 2:43 p.m. UTC | #1
On Tue, Jan 25, 2022 at 05:46:44PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> 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
> 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.

"Make sure that the uid and gid values associated with the relevant
ima policy are resolved in the user namespace of the opener of the
policy file."

is more correct. Also note, that by virtue of enforcing that securityfs
files can only ever be opened if the opener's userns is the same or an
ancestor of the userns the securityfs instance is mounted in we are
guaranteed that the uid and gid can be resolved. That's another way of
saying technically *_munged() isn't necessary but it is more correct
since we're crossing the user-kernel boundary.

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.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 47f2d1b5d156..151f418036ee 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -2002,6 +2002,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,};
> @@ -2087,7 +2088,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)
> @@ -2098,7 +2100,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)
> @@ -2109,7 +2112,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)
> @@ -2120,7 +2124,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)
> @@ -2131,7 +2136,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)
> @@ -2142,7 +2148,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.31.1
> 
>
Christian Brauner Jan. 26, 2022, 2:43 p.m. UTC | #2
On Wed, Jan 26, 2022 at 03:43:26PM +0100, Christian Brauner wrote:
> On Tue, Jan 25, 2022 at 05:46:44PM -0500, Stefan Berger wrote:
> > From: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > 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
> > 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.
> 
> "Make sure that the uid and gid values associated with the relevant
> ima policy are resolved in the user namespace of the opener of the
> policy file."
> 
> is more correct. Also note, that by virtue of enforcing that securityfs
> files can only ever be opened if the opener's userns is the same or an
> ancestor of the userns the securityfs instance is mounted in we are
> guaranteed that the uid and gid can be resolved. That's another way of
> saying technically *_munged() isn't necessary but it is more correct
> since we're crossing the user-kernel boundary.
> 
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > ---

Acked-by: Christian Brauner <brauner@kernel.org>
Stefan Berger Jan. 26, 2022, 11:13 p.m. UTC | #3
On 1/26/22 09:43, Christian Brauner wrote:
> On Tue, Jan 25, 2022 at 05:46:44PM -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> 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
>> 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.
> "Make sure that the uid and gid values associated with the relevant
> ima policy are resolved in the user namespace of the opener of the
> policy file."

I updated the text now to:

Make sure that the uid and gid values associated with the relevant IMA

policy are resolved in the user namespace of the opener of the policy

file. The effect of this changes is that when one displays 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.

>
> is more correct. Also note, that by virtue of enforcing that securityfs
> files can only ever be opened if the opener's userns is the same or an
> ancestor of the userns the securityfs instance is mounted in we are
> guaranteed that the uid and gid can be resolved. That's another way of
> saying technically *_munged() isn't necessary but it is more correct
> since we're crossing the user-kernel boundary.
>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.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 47f2d1b5d156..151f418036ee 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -2002,6 +2002,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,};
>> @@ -2087,7 +2088,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)
>> @@ -2098,7 +2100,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)
>> @@ -2109,7 +2112,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)
>> @@ -2120,7 +2124,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)
>> @@ -2131,7 +2136,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)
>> @@ -2142,7 +2148,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.31.1
>>
>>
diff mbox series

Patch

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 47f2d1b5d156..151f418036ee 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -2002,6 +2002,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,};
@@ -2087,7 +2088,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)
@@ -2098,7 +2100,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)
@@ -2109,7 +2112,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)
@@ -2120,7 +2124,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)
@@ -2131,7 +2136,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)
@@ -2142,7 +2148,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)