Patchwork [5/5] UBUNTU: SAUCE: AppArmor: Fix oops there is no tracer and doing unsafe transition.

login
register
mail settings
Submitter John Johansen
Date Nov. 10, 2009, 6:29 p.m.
Message ID <1257877753-9448-6-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/38069/
State Accepted
Headers show

Comments

John Johansen - Nov. 10, 2009, 6:29 p.m.
BugLink: http://bugs.launchpad.net/bugs/480112

SRU Justification:  This bug can cause confined process to oops at address 0.
This can occur when executing a process if the LSM_UNSAFE_PTRACE |
LSM_UNSAFE_PTRACE_CAP flags are set.  The likely hood of if/how often this
will occur depends on if ptrace is being used.

As reported by Tetsuo Handa on kernel-team mailing list:

In aa_may_change_ptraced_domain, if (!tracer) cred == NULL, and
put_cred(cred) will oops.  This will only happen on exec if the task
is marked as LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP, so should
only happen to ptraced tasks that are confined.

Fix this by returning directly from aa_may_change_ptrace_domain if
there is now tracer.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 ubuntu/apparmor/domain.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Stefan Bader - Nov. 11, 2009, 1:38 p.m.
As tracer is NULL in the exist case, ca_get_task_policy() has
not been called, so a direct exit looks ok.

John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/480112
> 
> SRU Justification:  This bug can cause confined process to oops at address 0.
> This can occur when executing a process if the LSM_UNSAFE_PTRACE |
> LSM_UNSAFE_PTRACE_CAP flags are set.  The likely hood of if/how often this
> will occur depends on if ptrace is being used.
> 
> As reported by Tetsuo Handa on kernel-team mailing list:
> 
> In aa_may_change_ptraced_domain, if (!tracer) cred == NULL, and
> put_cred(cred) will oops.  This will only happen on exec if the task
> is marked as LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP, so should
> only happen to ptraced tasks that are confined.
> 
> Fix this by returning directly from aa_may_change_ptrace_domain if
> there is now tracer.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  ubuntu/apparmor/domain.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
> index fe89ddc..12e45c6 100644
> --- a/ubuntu/apparmor/domain.c
> +++ b/ubuntu/apparmor/domain.c
> @@ -64,6 +64,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task,
>  		cred = aa_get_task_policy(tracer, &tracerp);
>  	rcu_read_unlock();
>  
> +	/* not ptraced */
> +	if (!tracer)
> +		return 0;
> +
>  	if (!tracerp)
>  		goto out;
>
Andy Whitcroft - Nov. 12, 2009, 11:28 a.m.
On Tue, Nov 10, 2009 at 10:29:13AM -0800, John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/480112
> 
> SRU Justification:  This bug can cause confined process to oops at address 0.
> This can occur when executing a process if the LSM_UNSAFE_PTRACE |
> LSM_UNSAFE_PTRACE_CAP flags are set.  The likely hood of if/how often this
> will occur depends on if ptrace is being used.
> 
> As reported by Tetsuo Handa on kernel-team mailing list:
> 
> In aa_may_change_ptraced_domain, if (!tracer) cred == NULL, and
> put_cred(cred) will oops.  This will only happen on exec if the task
> is marked as LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP, so should
> only happen to ptraced tasks that are confined.
> 
> Fix this by returning directly from aa_may_change_ptrace_domain if
> there is now tracer.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  ubuntu/apparmor/domain.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
> index fe89ddc..12e45c6 100644
> --- a/ubuntu/apparmor/domain.c
> +++ b/ubuntu/apparmor/domain.c
> @@ -64,6 +64,10 @@ static int aa_may_change_ptraced_domain(struct task_struct *task,
>  		cred = aa_get_task_policy(tracer, &tracerp);
>  	rcu_read_unlock();
>  
> +	/* not ptraced */
> +	if (!tracer)
> +		return 0;
> +
>  	if (!tracerp)
>  		goto out;
>  
> -- 
> 1.6.3.3

Hrm, now perhaps this is fixing the concern I raised in the previous
patch.  Perhaps it would be safer to simply make the put_cred()
incantation instead?

  if (cred)
    put_cred(cred);

-apw
Andy Whitcroft - Nov. 12, 2009, 11:36 a.m.
On Thu, Nov 12, 2009 at 11:28:10AM +0000, Andy Whitcroft wrote:

> Hrm, now perhaps this is fixing the concern I raised in the previous
> patch.  Perhaps it would be safer to simply make the put_cred()
> incantation instead?
> 
>   if (cred)
>     put_cred(cred);

I guess I _am_ happy that the current form would address the concern and
so in that sense:

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

I would suggest considering a more bullet proof approach as above for
your mainline efforts as its clearer that cred can only be released if
non-null, and its safer against further change in the function.

-apw
Tetsuo Handa - Nov. 12, 2009, 11:59 a.m.
Hello.

John Johansen wrote:
> As reported by Tetsuo Handa on kernel-team mailing list:
Oops. I used wrong sender address and therefore above report didn't reach to
kernel-team mailing list.



Just a comment for AppArmor for Karmic and earlier.

--- security/apparmor/path.c ---
> char *sysctl_pathname(struct ctl_table *table, char *buffer, int buflen)
> {
> 	if (buflen < 1)
> 		return NULL;
> 	buffer += --buflen;
> 	*buffer = '\0';
> 
> 	while (table) {
> 		int namelen = strlen(table->procname);

Eric W. Biederman is going to remove table->ctl_name field. Thus, future
versions will be safe to use table->procname without checking for NULL.

But, for past versions, some out-of-tree kernel module might create a table
with table->procname == NULL. Maybe AppArmor for Karmic and earlier should
prepare for NULL because parse_table()'s loop condition allows NULL procname.

> 
> 		if (buflen < namelen + 1)
> 			return NULL;
> 		buflen -= namelen + 1;
> 		buffer -= namelen;
> 		memcpy(buffer, table->procname, namelen);
> 		*--buffer = '/';
> 		table = table->parent;
> 	}
> 	if (buflen < 4)
> 		return NULL;
> 	buffer -= 4;
> 	memcpy(buffer, "/sys", 4);
> 
> 	return buffer;
> }
Stefan Bader - Nov. 12, 2009, 1:38 p.m.
Applied
John Johansen - Nov. 12, 2009, 4:27 p.m.
Andy Whitcroft wrote:
> On Thu, Nov 12, 2009 at 11:28:10AM +0000, Andy Whitcroft wrote:
> 
>> Hrm, now perhaps this is fixing the concern I raised in the previous
>> patch.  Perhaps it would be safer to simply make the put_cred()
>> incantation instead?
>>
>>   if (cred)
>>     put_cred(cred);
> 
> I guess I _am_ happy that the current form would address the concern and
> so in that sense:
> 
> Acked-by: Andy Whitcroft <apw@canonical.com>
> 
> I would suggest considering a more bullet proof approach as above for
> your mainline efforts as its clearer that cred can only be released if
> non-null, and its safer against further change in the function.
> 
Yep, what you suggest would be better.
John Johansen - Nov. 12, 2009, 4:29 p.m.
Tetsuo Handa wrote:
> Hello.
> 
> John Johansen wrote:
>> As reported by Tetsuo Handa on kernel-team mailing list:
> Oops. I used wrong sender address and therefore above report didn't reach to
> kernel-team mailing list.
> 
> 
> 
> Just a comment for AppArmor for Karmic and earlier.
> 
> --- security/apparmor/path.c ---
>> char *sysctl_pathname(struct ctl_table *table, char *buffer, int buflen)
>> {
>> 	if (buflen < 1)
>> 		return NULL;
>> 	buffer += --buflen;
>> 	*buffer = '\0';
>>
>> 	while (table) {
>> 		int namelen = strlen(table->procname);
> 
> Eric W. Biederman is going to remove table->ctl_name field. Thus, future
> versions will be safe to use table->procname without checking for NULL.
> 
> But, for past versions, some out-of-tree kernel module might create a table
> with table->procname == NULL. Maybe AppArmor for Karmic and earlier should
> prepare for NULL because parse_table()'s loop condition allows NULL procname.
> 
Thanks for pointing this out Tetsuo

Patch

diff --git a/ubuntu/apparmor/domain.c b/ubuntu/apparmor/domain.c
index fe89ddc..12e45c6 100644
--- a/ubuntu/apparmor/domain.c
+++ b/ubuntu/apparmor/domain.c
@@ -64,6 +64,10 @@  static int aa_may_change_ptraced_domain(struct task_struct *task,
 		cred = aa_get_task_policy(tracer, &tracerp);
 	rcu_read_unlock();
 
+	/* not ptraced */
+	if (!tracer)
+		return 0;
+
 	if (!tracerp)
 		goto out;