Message-ID: <4CF4CF9C.30605@canonical.com>
Date: Tue, 30 Nov 2010 02:19:08 -0800
From: John Johansen <john.johansen@canonical.com>
Organization: Canonical
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101027 Thunderbird/3.1.6
MIME-Version: 1.0
To: wzt.wzt@gmail.com
CC: linux-kernel@vger.kernel.org, apparmor@lists.ubuntu.com,
        linux-security-module@vger.kernel.org
Subject: Re: [PATCH] APPARMOR: Fix NULL Pointer dereference while __add_new_profile
References: <20101126151847.GA2721@localhost.localdomain>
In-Reply-To: <20101126151847.GA2721@localhost.localdomain>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2137
Lines: 49

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);

--
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/