[Patch 0/2] Fixes for masking of complain mode, and rcu protected ref

[Patch 0/2] Fixes for masking of complain mode, and rcu protected ref

[John Johansen]

A couple of outstanding fixes to apparmor.

The following changes since commit b0af8dfdd67699e25083478c63eedef2e72ebd85:

  Linux 3.0-rc5 (2011-06-27 19:12:22 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jj/apparmor-dev.git for-security

John Johansen (2):
      AppArmor: Fix reference to rcu protected pointer outside of rcu_read_lock
      AppArmor: Fix masking of capabilities in complain mode

 security/apparmor/domain.c |    2 +-
 security/apparmor/lsm.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

[PATCH 1/2] AppArmor: Fix reference to rcu protected pointer outside of rcu_read_lock

[John Johansen]

The pointer returned from tracehook_tracer_task() is only valid inside
the rcu_read_lock.  However the tracer pointer obtained is being passed
to aa_may_ptrace outside of the rcu_read_lock critical section.

Mover the aa_may_ptrace test into the rcu_read_lock critical section, to
fix this.

Kernels affected: 2.6.36 - 3.0

Reported-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/domain.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index c825c6e..78adc43 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -73,7 +73,6 @@ static int may_change_ptraced_domain(struct task_struct *task,
 		cred = get_task_cred(tracer);
 		tracerp = aa_cred_profile(cred);
 	}
-	rcu_read_unlock();
 
 	/* not ptraced */
 	if (!tracer || unconfined(tracerp))
@@ -82,6 +81,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
 	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
 
 out:
+	rcu_read_unlock();
 	if (cred)
 		put_cred(cred);
 
-- 
1.7.4.1

[PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[John Johansen]

AppArmor is masking the capabilities returned by capget against the
capabilities mask in the profile.  This is wrong, in complain mode the
profile has effectively all capabilities, as the profile restrictions are
not being enforced, merely tested against to determine is an access is
known by the profile.

This can result in the wrong behavior of security conscience applications
like sshd which examine their capability set, and change their behavior
accordingly.  In this case because of the masked capability set being
returned sshd fails due to DAC checks, even when the profile is complain
mode.

Kernels affected: 2.6.36 - 3.0

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

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 3d2fd14..3783202 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -127,7 +127,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
 	*inheritable = cred->cap_inheritable;
 	*permitted = cred->cap_permitted;
 
-	if (!unconfined(profile)) {
+	if (!unconfined(profile) && !COMPLAIN_MODE(profile)) {
 		*effective = cap_intersect(*effective, profile->caps.allow);
 		*permitted = cap_intersect(*permitted, profile->caps.allow);
 	}
-- 
1.7.4.1

Re: [PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[Greg KH]

On Tue, Jun 28, 2011 at 04:56:05PM +0100, John Johansen wrote:
> AppArmor is masking the capabilities returned by capget against the
> capabilities mask in the profile.  This is wrong, in complain mode the
> profile has effectively all capabilities, as the profile restrictions are
> not being enforced, merely tested against to determine is an access is
> known by the profile.
> 
> This can result in the wrong behavior of security conscience applications
> like sshd which examine their capability set, and change their behavior
> accordingly.  In this case because of the masked capability set being
> returned sshd fails due to DAC checks, even when the profile is complain
> mode.
> 
> Kernels affected: 2.6.36 - 3.0
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>

As this should probably go to the stable tree, next time care to add a
simple:
	Cc: stable <stable@kernel.org>
to the signed-off-by: area of the patch so it gets automagically
included in the stable and longterm kernels as needed when it hits
Linus's tree?

thanks,

greg k-h

Re: [PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[James Morris]

On Tue, 28 Jun 2011, Greg KH wrote:

> As this should probably go to the stable tree, next time care to add a
> simple:
> 	Cc: stable <stable@kernel.org>
> to the signed-off-by: area of the patch so it gets automagically
> included in the stable and longterm kernels as needed when it hits
> Linus's tree?
> 

JJ, I'll wait for you to respin these before pulling them.

-- 
James Morris <jmorris@namei.org>

Re: [PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[Greg KH]

On Wed, Jun 29, 2011 at 08:55:18AM +0100, John Johansen wrote:
> On 06/29/2011 05:24 AM, Greg KH wrote:
> > On Wed, Jun 29, 2011 at 02:44:34AM +0100, John Johansen wrote:
> >> AppArmor is masking the capabilities returned by capget against the
> >> capabilities mask in the profile.  This is wrong, in complain mode the
> >> profile has effectively all capabilities, as the profile restrictions are
> >> not being enforced, merely tested against to determine if an access is
> >> known by the profile.
> >>
> >> This can result in the wrong behavior of security conscience applications
> >> like sshd which examine their capability set, and change their behavior
> >> accordingly.  In this case because of the masked capability set being
> >> returned sshd fails due to DAC checks, even when the profile is in complain
> >> mode.
> >>
> >> Kernels affected: 2.6.36 - 3.0.
> >>
> >> Signed-off-by: John Johansen <john.johansen@canonical.com>
> >> ---
> > 
> > You say that multiple kernels are affected, then why not also include
> > stable@kernel.org here as well?
> > 
> > confused,
> > 
> Sorry I should have elaborated, I think its a borderline case for stable
> release, and was as much a note to my self as anything.

Notes to self probably shouldn't be in kernel changelog entries :)

> The bug doesn't affect the enforcement of policy, only learning mode used
> for generating policy, and it is something we can work around in userspace.
> for already released kernels.
> 
> I can resend with a Cc: stable if you would like

If you don't think it should be in the stable kernel releases, that's
fine, you are the maintainer of the code.  I just found it odd that you
said it affected older kernels and yet didn't want it applied there as
well.

greg k-h

Re: [PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[John Johansen]

On 06/29/2011 05:24 AM, Greg KH wrote:
> On Wed, Jun 29, 2011 at 02:44:34AM +0100, John Johansen wrote:
>> AppArmor is masking the capabilities returned by capget against the
>> capabilities mask in the profile.  This is wrong, in complain mode the
>> profile has effectively all capabilities, as the profile restrictions are
>> not being enforced, merely tested against to determine if an access is
>> known by the profile.
>>
>> This can result in the wrong behavior of security conscience applications
>> like sshd which examine their capability set, and change their behavior
>> accordingly.  In this case because of the masked capability set being
>> returned sshd fails due to DAC checks, even when the profile is in complain
>> mode.
>>
>> Kernels affected: 2.6.36 - 3.0.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>> ---
> 
> You say that multiple kernels are affected, then why not also include
> stable@kernel.org here as well?
> 
> confused,
> 
Sorry I should have elaborated, I think its a borderline case for stable
release, and was as much a note to my self as anything.

The bug doesn't affect the enforcement of policy, only learning mode used
for generating policy, and it is something we can work around in userspace.
for already released kernels.

I can resend with a Cc: stable if you would like

Re: [PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[Greg KH]

On Wed, Jun 29, 2011 at 02:44:34AM +0100, John Johansen wrote:
> AppArmor is masking the capabilities returned by capget against the
> capabilities mask in the profile.  This is wrong, in complain mode the
> profile has effectively all capabilities, as the profile restrictions are
> not being enforced, merely tested against to determine if an access is
> known by the profile.
> 
> This can result in the wrong behavior of security conscience applications
> like sshd which examine their capability set, and change their behavior
> accordingly.  In this case because of the masked capability set being
> returned sshd fails due to DAC checks, even when the profile is in complain
> mode.
> 
> Kernels affected: 2.6.36 - 3.0.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---

You say that multiple kernels are affected, then why not also include
stable@kernel.org here as well?

confused,

greg k-h

[PATCH 2/2] AppArmor: Fix masking of capabilities in complain mode

[John Johansen]

AppArmor is masking the capabilities returned by capget against the
capabilities mask in the profile.  This is wrong, in complain mode the
profile has effectively all capabilities, as the profile restrictions are
not being enforced, merely tested against to determine if an access is
known by the profile.

This can result in the wrong behavior of security conscience applications
like sshd which examine their capability set, and change their behavior
accordingly.  In this case because of the masked capability set being
returned sshd fails due to DAC checks, even when the profile is in complain
mode.

Kernels affected: 2.6.36 - 3.0.

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

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 3d2fd14..3783202 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -127,7 +127,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
 	*inheritable = cred->cap_inheritable;
 	*permitted = cred->cap_permitted;
 
-	if (!unconfined(profile)) {
+	if (!unconfined(profile) && !COMPLAIN_MODE(profile)) {
 		*effective = cap_intersect(*effective, profile->caps.allow);
 		*permitted = cap_intersect(*permitted, profile->caps.allow);
 	}
-- 
1.7.4.1