diff mbox

[05/11] AppArmor: fix regression by setting default to mediate deleted files

Message ID 1271142580-26555-6-git-send-email-john.johansen@canonical.com
State Accepted
Delegated to: Andy Whitcroft
Headers show

Commit Message

John Johansen April 13, 2010, 7:09 a.m. UTC 
From: John Johansen <john.johansen@canonical.com>

OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: 8d3ffc7c845dc1277b39572016fbf3265702f4d4
BugLink: http://bugs.launchpad.net/bugs/562056

The default behavior for AppArmor used to be to mediate deleted files.
This can now be controlled on a per profile basis but the field is
not defaulting to the correct value when path_flags is not specified.

This is causing regressions in profiles expecting deleted files to
be mediated by path instead of delegated.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/policy_unpack.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Comments

Andy Whitcroft April 13, 2010, 9:02 a.m. UTC | #1 
On Tue, Apr 13, 2010 at 12:09:34AM -0700, john.johansen@canonical.com wrote:
> From: John Johansen <john.johansen@canonical.com>
> 
> OriginalAuthor: John Johansen <john.johansen@canonical.com>
> OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
> commit: 8d3ffc7c845dc1277b39572016fbf3265702f4d4
> BugLink: http://bugs.launchpad.net/bugs/562056
> 
> The default behavior for AppArmor used to be to mediate deleted files.
> This can now be controlled on a per profile basis but the field is
> not defaulting to the correct value when path_flags is not specified.
> 
> This is causing regressions in profiles expecting deleted files to
> be mediated by path instead of delegated.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/policy_unpack.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index a475d7c..0a15f41 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -507,8 +507,11 @@ static struct aa_profile *unpack_profile(struct aa_ext *e,
>  		goto fail;
>  
>  	/* path_flags is optional */
> -	unpack_u32(e, &profile->path_flags, "path_flags");
> -	profile->path_flags |= profile->flags & PFLAG_MEDIATE_DELETED;
> +	if (unpack_u32(e, &profile->path_flags, "path_flags"))
> +		profile->path_flags |= profile->flags & PFLAG_MEDIATE_DELETED;
> +	else
> +		/* default to */
> +		profile->path_flags = PFLAG_MEDIATE_DELETED;
>  
>  	/* mmap_min_addr is optional */
>  	if (unpack_u64(e, &tmp64, "mmap_min_addr")) {
> -- 

I presume this is at profile load time.  Looks sane:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
John Johansen April 13, 2010, 9:03 a.m. UTC | #2 
On 04/13/2010 02:02 AM, Andy Whitcroft wrote:
> On Tue, Apr 13, 2010 at 12:09:34AM -0700, john.johansen@canonical.com wrote:
>> From: John Johansen <john.johansen@canonical.com>
>>
>> OriginalAuthor: John Johansen <john.johansen@canonical.com>
>> OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
>> commit: 8d3ffc7c845dc1277b39572016fbf3265702f4d4
>> BugLink: http://bugs.launchpad.net/bugs/562056
>>
>> The default behavior for AppArmor used to be to mediate deleted files.
>> This can now be controlled on a per profile basis but the field is
>> not defaulting to the correct value when path_flags is not specified.
>>
>> This is causing regressions in profiles expecting deleted files to
>> be mediated by path instead of delegated.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>> ---
>>  security/apparmor/policy_unpack.c |    7 +++++--
>>  1 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
>> index a475d7c..0a15f41 100644
>> --- a/security/apparmor/policy_unpack.c
>> +++ b/security/apparmor/policy_unpack.c
>> @@ -507,8 +507,11 @@ static struct aa_profile *unpack_profile(struct aa_ext *e,
>>  		goto fail;
>>  
>>  	/* path_flags is optional */
>> -	unpack_u32(e, &profile->path_flags, "path_flags");
>> -	profile->path_flags |= profile->flags & PFLAG_MEDIATE_DELETED;
>> +	if (unpack_u32(e, &profile->path_flags, "path_flags"))
>> +		profile->path_flags |= profile->flags & PFLAG_MEDIATE_DELETED;
>> +	else
>> +		/* default to */
>> +		profile->path_flags = PFLAG_MEDIATE_DELETED;
>>  
>>  	/* mmap_min_addr is optional */
>>  	if (unpack_u64(e, &tmp64, "mmap_min_addr")) {
>> -- 
> 
> I presume this is at profile load time.  Looks sane:
> 
yes
diff mbox

Patch

diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index a475d7c..0a15f41 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -507,8 +507,11 @@  static struct aa_profile *unpack_profile(struct aa_ext *e,
 		goto fail;
 
 	/* path_flags is optional */
-	unpack_u32(e, &profile->path_flags, "path_flags");
-	profile->path_flags |= profile->flags & PFLAG_MEDIATE_DELETED;
+	if (unpack_u32(e, &profile->path_flags, "path_flags"))
+		profile->path_flags |= profile->flags & PFLAG_MEDIATE_DELETED;
+	else
+		/* default to */
+		profile->path_flags = PFLAG_MEDIATE_DELETED;
 
 	/* mmap_min_addr is optional */
 	if (unpack_u64(e, &tmp64, "mmap_min_addr")) {