Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1296796 > unrolled thread

[PATCH] IMA: policy can be updated zero times

Started bySasha Levin <sasha.levin@oracle.com>
First post2015-12-22 15:00 +0100
Last post2015-12-22 23:20 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] IMA: policy can be updated zero times Sasha Levin <sasha.levin@oracle.com> - 2015-12-22 15:00 +0100
    Re: [PATCH] IMA: policy can be updated zero times Mimi Zohar <zohar@linux.vnet.ibm.com> - 2015-12-22 21:00 +0100
      Re: [PATCH] IMA: policy can be updated zero times Sasha Levin <sasha.levin@oracle.com> - 2015-12-22 23:00 +0100
        Re: [PATCH] IMA: policy can be updated zero times Petko Manolov <petkan@mip-labs.com> - 2015-12-23 12:50 +0100
          Re: [PATCH] IMA: policy can be updated zero times Mimi Zohar <zohar@linux.vnet.ibm.com> - 2015-12-23 13:30 +0100
            Re: [Linux-ima-devel] [PATCH] IMA: policy can be updated zero times Mimi Zohar <zohar@linux.vnet.ibm.com> - 2015-12-23 13:50 +0100
      Re: [PATCH] IMA: policy can be updated zero times Petko Manolov <petkan@mip-labs.com> - 2015-12-22 23:20 +0100

#1296796 — [PATCH] IMA: policy can be updated zero times

FromSasha Levin <sasha.levin@oracle.com>
Date2015-12-22 15:00 +0100
Subject[PATCH] IMA: policy can be updated zero times
Message-ID<qIvDQ-4ET-5@gated-at.bofh.it>
Commit "IMA: policy can now be updated multiple times" assumed that the
policy would be updated at least once.

If there are zero updates, the temporary list head object will get added
to the policy list, and later dereferenced as an IMA policy object, which
means that invalid memory will be accessed.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 security/integrity/ima/ima_policy.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ba5d2fc..9b958b8 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -431,6 +431,9 @@ void ima_update_policy(void)
 {
 	struct list_head *first, *last, *policy;
 
+	if (list_empty(&ima_temp_rules))
+		return;
+
 	/* append current policy with the new rules */
 	first = (&ima_temp_rules)->next;
 	last = (&ima_temp_rules)->prev;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1297030

FromMimi Zohar <zohar@linux.vnet.ibm.com>
Date2015-12-22 21:00 +0100
Message-ID<qIBge-8do-13@gated-at.bofh.it>
In reply to#1296796
On Tue, 2015-12-22 at 08:51 -0500, Sasha Levin wrote:
> Commit "IMA: policy can now be updated multiple times" assumed that the
> policy would be updated at least once.
> 
> If there are zero updates, the temporary list head object will get added
> to the policy list, and later dereferenced as an IMA policy object, which
> means that invalid memory will be accessed.
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  security/integrity/ima/ima_policy.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index ba5d2fc..9b958b8 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -431,6 +431,9 @@ void ima_update_policy(void)
>  {
>  	struct list_head *first, *last, *policy;
> 
> +	if (list_empty(&ima_temp_rules))
> +		return;
> +
>  	/* append current policy with the new rules */
>  	first = (&ima_temp_rules)->next;
>  	last = (&ima_temp_rules)->prev;

Thanks, Sasha.  By the time ima_update_policy() is called
ima_release_policy() has already output the policy update status
message.  I guess an empty policy could be considered a valid policy.
Could you add a msg indicating that the new policy was empty?

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297083

FromSasha Levin <sasha.levin@oracle.com>
Date2015-12-22 23:00 +0100
Message-ID<qID8m-TR-27@gated-at.bofh.it>
In reply to#1297030
On 12/22/2015 04:40 PM, Petko Manolov wrote:
>> Thanks, Sasha.  By the time ima_update_policy() is called
>> >ima_release_policy() has already output the policy update status
>> >message.  I guess an empty policy could be considered a valid policy.
>> >Could you add a msg indicating that the new policy was empty?
> 
> As far as I can say we can't get to ima_update_policy() with empty ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case of an empty rule.  I'll double check it tomorrow, but please you do that too.

This is based on an actual crash rather than code analysis.


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297397

FromPetko Manolov <petkan@mip-labs.com>
Date2015-12-23 12:50 +0100
Message-ID<qIQ5B-Cl-21@gated-at.bofh.it>
In reply to#1297083
On 15-12-22 16:50:01, Sasha Levin wrote:
> On 12/22/2015 04:40 PM, Petko Manolov wrote:
> >> Thanks, Sasha.  By the time ima_update_policy() is called
> >> >ima_release_policy() has already output the policy update status
> >> >message.  I guess an empty policy could be considered a valid policy.
> >> >Could you add a msg indicating that the new policy was empty?
> > 
> > As far as I can say we can't get to ima_update_policy() with empty 
> > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case 
> > of an empty rule.  I'll double check it tomorrow, but please you do that 
> > too.
> 
> This is based on an actual crash rather than code analysis.

I was able to reproduce the crash with: echo "" > /sys/kernel/security/ima/policy

It turns out ima_parse_add_rule() returns 1, even though the string is empty 
This logic may be part of "empty policy is a valid policy" or something else.  
As it is more dangerous to change the behavior at this point i assume your patch 
is the right solution for the problem.

Acked-by: Petko Manolov <petkan@mip-labs.com>

Mimi, shall we change ima_parse_add_rule's behavior in the future or it's too 
much work?


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297414

FromMimi Zohar <zohar@linux.vnet.ibm.com>
Date2015-12-23 13:30 +0100
Message-ID<qIQIi-158-7@gated-at.bofh.it>
In reply to#1297397
On Wed, 2015-12-23 at 13:47 +0200, Petko Manolov wrote:

> On 15-12-22 16:50:01, Sasha Levin wrote:
> > On 12/22/2015 04:40 PM, Petko Manolov wrote:
> > >> Thanks, Sasha.  By the time ima_update_policy() is called
> > >> >ima_release_policy() has already output the policy update status
> > >> >message.  I guess an empty policy could be considered a valid policy.
> > >> >Could you add a msg indicating that the new policy was empty?
> > > 
> > > As far as I can say we can't get to ima_update_policy() with empty 
> > > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case 
> > > of an empty rule.  I'll double check it tomorrow, but please you do that 
> > > too.
> > 
> > This is based on an actual crash rather than code analysis.
> 
> I was able to reproduce the crash with: echo "" > /sys/kernel/security/ima/policy
> 
> It turns out ima_parse_add_rule() returns 1, even though the string is empty 
> This logic may be part of "empty policy is a valid policy" or something else.  
> As it is more dangerous to change the behavior at this point i assume your patch 
> is the right solution for the problem.
> 
> Acked-by: Petko Manolov <petkan@mip-labs.com>
> 
> Mimi, shall we change ima_parse_add_rule's behavior in the future or it's too 
> much work?

ima_parse_add_rules() has no way of knowing if the policy as a whole is
valid.  I would define a new function in ima_policy.c to return the
number of rules being added and call it at the beginning of
ima_release_policy() before the status message.  That way the number of
rules added can be included in the status message.

For now, the function could just return have rules or no rules, instead
of the number of rules.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297443 — Re: [Linux-ima-devel] [PATCH] IMA: policy can be updated zero times

FromMimi Zohar <zohar@linux.vnet.ibm.com>
Date2015-12-23 13:50 +0100
SubjectRe: [Linux-ima-devel] [PATCH] IMA: policy can be updated zero times
Message-ID<qIR1F-1bM-35@gated-at.bofh.it>
In reply to#1297414
On Wed, 2015-12-23 at 07:24 -0500, Mimi Zohar wrote:
> On Wed, 2015-12-23 at 13:47 +0200, Petko Manolov wrote:
> 
> > On 15-12-22 16:50:01, Sasha Levin wrote:
> > > On 12/22/2015 04:40 PM, Petko Manolov wrote:
> > > >> Thanks, Sasha.  By the time ima_update_policy() is called
> > > >> >ima_release_policy() has already output the policy update status
> > > >> >message.  I guess an empty policy could be considered a valid policy.
> > > >> >Could you add a msg indicating that the new policy was empty?
> > > > 
> > > > As far as I can say we can't get to ima_update_policy() with empty 
> > > > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case 
> > > > of an empty rule.  I'll double check it tomorrow, but please you do that 
> > > > too.
> > > 
> > > This is based on an actual crash rather than code analysis.
> > 
> > I was able to reproduce the crash with: echo "" > /sys/kernel/security/ima/policy
> > 
> > It turns out ima_parse_add_rule() returns 1, even though the string is empty 
> > This logic may be part of "empty policy is a valid policy" or something else.  
> > As it is more dangerous to change the behavior at this point i assume your patch 
> > is the right solution for the problem.
> > 
> > Acked-by: Petko Manolov <petkan@mip-labs.com>
> > 
> > Mimi, shall we change ima_parse_add_rule's behavior in the future or it's too 
> > much work?
> 
> ima_parse_add_rules() has no way of knowing if the policy as a whole is
> valid.  I would define a new function in ima_policy.c to return the
> number of rules being added and call it at the beginning of
> ima_release_policy() before the status message.  That way the number of
> rules added can be included in the status message.
> 
> For now, the function could just return have rules or no rules, instead
> of the number of rules.

Sasha, could you make your fix a separate function (above
ima_update_policy) and call it from ima_release_policy()?

Thanks!

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297096

FromPetko Manolov <petkan@mip-labs.com>
Date2015-12-22 23:20 +0100
Message-ID<qID8m-TR-29@gated-at.bofh.it>
In reply to#1297030
On December 22, 2015 9:56:28 PM GMT+02:00, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>On Tue, 2015-12-22 at 08:51 -0500, Sasha Levin wrote:
>> Commit "IMA: policy can now be updated multiple times" assumed that
>the
>> policy would be updated at least once.
>> 
>> If there are zero updates, the temporary list head object will get
>added
>> to the policy list, and later dereferenced as an IMA policy object,
>which
>> means that invalid memory will be accessed.
>> 
>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>> ---
>>  security/integrity/ima/ima_policy.c |    3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/security/integrity/ima/ima_policy.c
>b/security/integrity/ima/ima_policy.c
>> index ba5d2fc..9b958b8 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -431,6 +431,9 @@ void ima_update_policy(void)
>>  {
>>  	struct list_head *first, *last, *policy;
>> 
>> +	if (list_empty(&ima_temp_rules))
>> +		return;
>> +
>>  	/* append current policy with the new rules */
>>  	first = (&ima_temp_rules)->next;
>>  	last = (&ima_temp_rules)->prev;
>
>Thanks, Sasha.  By the time ima_update_policy() is called
>ima_release_policy() has already output the policy update status
>message.  I guess an empty policy could be considered a valid policy.
>Could you add a msg indicating that the new policy was empty?


As far as I can say we can't get to ima_update_policy() with empty ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case of an empty rule.  I'll double check it tomorrow, but please you do that too.


cheers,
Petko




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web