[PATCH] APPARMOR: Fix NULL Pointer dereference while __add_new_profile

[PATCH] APPARMOR: Fix NULL Pointer dereference while __add_new_profile

[wzt.wzt]

In aa_replace_profiles(), if __lookup_parent() path failed, policy is set 
to NULL and goto audit label, old_profile and rename_profile are both NULL,
__add_new_profile is called, the parameter policy is NULL, it will cause
NULL pointer dereference via __list_add_profile(&policy->profiles, profile);

Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>

---
 security/apparmor/policy.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 52cc865..832d9e9 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -940,6 +940,8 @@ static int replacement_allowed(struct aa_profile *profile, int noreplace,
 static void __add_new_profile(struct aa_namespace *ns, struct aa_policy *policy,
 			      struct aa_profile *profile)
 {
+	if (!policy)
+		return ;
 	if (policy != &ns->base)
 		/* released on profile replacement or free_profile */
 		profile->parent = aa_get_profile((struct aa_profile *) policy);
-- 
1.6.5.3

Re: [PATCH] APPARMOR: Fix NULL Pointer dereference while __add_new_profile

[John Johansen]

On 11/26/2010 07:18 AM, wzt.wzt@gmail.com wrote:
> In aa_replace_profiles(), if __lookup_parent() path failed, policy is set 
> to NULL and goto audit label, old_profile and rename_profile are both NULL,
> __add_new_profile is called, the parameter policy is NULL, it will cause
> NULL pointer dereference via __list_add_profile(&policy->profiles, profile);
> 
> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
> 
NAK, we can't just fail adding the profile to the policy here.  If this
was an error we would need to either return an error or handle it before
the this function was called.

In this case when __lookup_parent fails it sets error = -ENOENT before jumping
to audit:

The way the audit routines work is that they will return the error passed
into them with a few exceptions.  If in complain mode it can override,
the apparmor set eperm and eaccess error codes, and it can return errors that
occurred during auditing.

So in this case the error condition is guaranteed to be set and
__add_new_profile will never get called.

Currently this isn't very clear in the code and it could use a comment, or
maybe even some reworking so that the failure path calls audit_policy directly

> ---
>  security/apparmor/policy.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index 52cc865..832d9e9 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -940,6 +940,8 @@ static int replacement_allowed(struct aa_profile *profile, int noreplace,
>  static void __add_new_profile(struct aa_namespace *ns, struct aa_policy *policy,
>  			      struct aa_profile *profile)
>  {
> +	if (!policy)
> +		return ;
>  	if (policy != &ns->base)
>  		/* released on profile replacement or free_profile */
>  		profile->parent = aa_get_profile((struct aa_profile *) policy);