Patchwork [2/2] UBUNTU: SAUCE: AppArmor: allow newer tools to load policy on older kernels

login
register
mail settings
Submitter John Johansen
Date Sept. 17, 2010, 4:02 p.m.
Message ID <1284739355-17542-3-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/65085/
State Accepted
Delegated to: Leann Ogasawara
Headers show

Comments

John Johansen - Sept. 17, 2010, 4:02 p.m.
BugLink: http://bugs.launchpad.net/bugs/639758

Remove an unnecessary restriction from the AppArmor network capability patch
When a newer version of the tools is used with an older kernel, it may build
in extra rules for newer networking protocols that the older kernel does
not know about.

The older kernel can safely discard the extra rules as it should never
create sockets using the newer protocols, and the LSM hooks will
never pass requests matching these rules.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/policy_unpack.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)
Tetsuo Handa - Sept. 17, 2010, 11:54 p.m.
John Johansen wrote:
>  		for (i = 0; i < size; i++) {
> +			/* discard extraneous rules that this kernel will
> +			 * never request
> +			 */
> +			if (size > AF_MAX) {

Do you want to discard all rules rather than extraneous rules?
I think this should be (i >= AF_MAX) rather than (size > AF_MAX).

> +				u16 tmp;
> +				if (!unpack_u16(e, &tmp, NULL) ||
> +				    !unpack_u16(e, &tmp, NULL) ||
> +				    !unpack_u16(e, &tmp, NULL))
> +					goto fail;
> +				continue;
> +			}
>  			if (!unpack_u16(e, &profile->net.allow[i], NULL))
>  				goto fail;
>  			if (!unpack_u16(e, &profile->net.audit[i], NULL))
John Johansen - Sept. 21, 2010, 8:32 a.m.
On 09/17/2010 04:54 PM, Tetsuo Handa wrote:
> John Johansen wrote:
>>   		for (i = 0; i<  size; i++) {
>> +			/* discard extraneous rules that this kernel will
>> +			 * never request
>> +			 */
>> +			if (size>  AF_MAX) {
>
> Do you want to discard all rules rather than extraneous rules?
> I think this should be (i>= AF_MAX) rather than (size>  AF_MAX).
>
>> +				u16 tmp;
>> +				if (!unpack_u16(e,&tmp, NULL) ||
>> +				    !unpack_u16(e,&tmp, NULL) ||
>> +				    !unpack_u16(e,&tmp, NULL))
>> +					goto fail;
>> +				continue;
>> +			}
>>   			if (!unpack_u16(e,&profile->net.allow[i], NULL))
>>   				goto fail;
>>   			if (!unpack_u16(e,&profile->net.audit[i], NULL))

sigh, yes. I can't believe I did that :(

thanks Tetsuo
Tim Gardner - Sept. 21, 2010, 11:31 a.m.
On 09/21/2010 04:32 PM, John Johansen wrote:
> On 09/17/2010 04:54 PM, Tetsuo Handa wrote:
>> John Johansen wrote:
>>>    		for (i = 0; i<   size; i++) {
>>> +			/* discard extraneous rules that this kernel will
>>> +			 * never request
>>> +			 */
>>> +			if (size>   AF_MAX) {
>>
>> Do you want to discard all rules rather than extraneous rules?
>> I think this should be (i>= AF_MAX) rather than (size>   AF_MAX).
>>
>>> +				u16 tmp;
>>> +				if (!unpack_u16(e,&tmp, NULL) ||
>>> +				    !unpack_u16(e,&tmp, NULL) ||
>>> +				    !unpack_u16(e,&tmp, NULL))
>>> +					goto fail;
>>> +				continue;
>>> +			}
>>>    			if (!unpack_u16(e,&profile->net.allow[i], NULL))
>>>    				goto fail;
>>>    			if (!unpack_u16(e,&profile->net.audit[i], NULL))
>
> sigh, yes. I can't believe I did that :(
>
> thanks Tetsuo
>

So, whats the impact? Does this mean that we're dropping all AA rules?

rtg
Tetsuo Handa - Sept. 21, 2010, 2:13 p.m.
Tim Gardner wrote:
> So, whats the impact? Does this mean that we're dropping all AA rules?

At first, I thought the impact of this error is

  When a profile with address family which currently running kernel does not
  know is loaded, loading the profile will succeed but all networking
  permissions are discarded. Therefore, currently running kernel will reject
  all socket operations (e.g. socket(), bind(), sendmsg()) for all families
  (except AF_UNIX and AF_NETLINK) with -EACCES unless the process is
  unconfined. This means that networking applications (e.g. firefox, cupsd,
  dhclient) which will be confined by profiles won't work properly.

But after reading security/apparmor/net.c , it changed to:

  No impact at all because Maverick kernel does not provide networking
  mediation functionality.

What? Excuse me, John. I assumed that networking mediation functionality is
included into Maverick kernel. But according to
http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-maverick.git;a=blob;f=security/apparmor/net.c;hb=HEAD
(as of "ALSA: seq/oss - Fix double-free at error path of snd_seq_oss_open()"),
I can't find a line that stores error code to sa.aad.error within audit_net().
This means that sa.aad.error is always 0 and therefore aa_net_perm() will
always return 0 (rather than -EACCESS) no matter how "net_allowed_af" is
specified.

I hope I'm missing something...
John Johansen - Sept. 21, 2010, 4:13 p.m.
On 09/21/2010 04:31 AM, Tim Gardner wrote:
> On 09/21/2010 04:32 PM, John Johansen wrote:
>> On 09/17/2010 04:54 PM, Tetsuo Handa wrote:
>>> John Johansen wrote:
>>>> for (i = 0; i< size; i++) {
>>>> + /* discard extraneous rules that this kernel will
>>>> + * never request
>>>> + */
>>>> + if (size> AF_MAX) {
>>>
>>> Do you want to discard all rules rather than extraneous rules?
>>> I think this should be (i>= AF_MAX) rather than (size> AF_MAX).
>>>
>>>> + u16 tmp;
>>>> + if (!unpack_u16(e,&tmp, NULL) ||
>>>> + !unpack_u16(e,&tmp, NULL) ||
>>>> + !unpack_u16(e,&tmp, NULL))
>>>> + goto fail;
>>>> + continue;
>>>> + }
>>>> if (!unpack_u16(e,&profile->net.allow[i], NULL))
>>>> goto fail;
>>>> if (!unpack_u16(e,&profile->net.audit[i], NULL))
>>
>> sigh, yes. I can't believe I did that :(
>>
>> thanks Tetsuo
>>
>
> So, whats the impact? Does this mean that we're dropping all AA rules?
>
No. It means we will drop network rules if the tools are built against
a newer kernel tree that has added a new address family.  Against the
current tools everything works.

To load policy the user has to be an unconfined root, at which point
they can load modules and do other nasties so there isn't a potential
escalation out of this.  It should just potentially affect machines on
upgrade.

So we need to SRU a patch for this but it is not release critical, but I have the patch and after I take a second look at it to make sure it is right this time.  I will kick it out this morning.
John Johansen - Sept. 21, 2010, 4:29 p.m.
On 09/21/2010 07:13 AM, Tetsuo Handa wrote:
> Tim Gardner wrote:
>> So, whats the impact? Does this mean that we're dropping all AA rules?
>
> At first, I thought the impact of this error is
>
>    When a profile with address family which currently running kernel does not
>    know is loaded, loading the profile will succeed but all networking
>    permissions are discarded. Therefore, currently running kernel will reject
>    all socket operations (e.g. socket(), bind(), sendmsg()) for all families
>    (except AF_UNIX and AF_NETLINK) with -EACCES unless the process is
>    unconfined. This means that networking applications (e.g. firefox, cupsd,
>    dhclient) which will be confined by profiles won't work properly.
>
> But after reading security/apparmor/net.c , it changed to:
>
>    No impact at all because Maverick kernel does not provide networking
>    mediation functionality.
>
> What? Excuse me, John. I assumed that networking mediation functionality is
> included into Maverick kernel. But according to
> http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-maverick.git;a=blob;f=security/apparmor/net.c;hb=HEAD
> (as of "ALSA: seq/oss - Fix double-free at error path of snd_seq_oss_open()"),
> I can't find a line that stores error code to sa.aad.error within audit_net().
> This means that sa.aad.error is always 0 and therefore aa_net_perm() will
> always return 0 (rather than -EACCESS) no matter how "net_allowed_af" is
> specified.
>
> I hope I'm missing something...

Unfortunately, no.  The error is being set but dropped in the audit fn, it
seems I broke it during the auditing update, and that the regression test
suite for networking is broken.  I'll get the SRU patch out immediately.

Patch

diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 6b0637b..9c51b03 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -575,10 +575,18 @@  static struct aa_profile *unpack_profile(struct aa_ext *e)
 
 	size = unpack_array(e, "net_allowed_af");
 	if (size) {
-		if (size > AF_MAX)
-			goto fail;
-
 		for (i = 0; i < size; i++) {
+			/* discard extraneous rules that this kernel will
+			 * never request
+			 */
+			if (size > AF_MAX) {
+				u16 tmp;
+				if (!unpack_u16(e, &tmp, NULL) ||
+				    !unpack_u16(e, &tmp, NULL) ||
+				    !unpack_u16(e, &tmp, NULL))
+					goto fail;
+				continue;
+			}
 			if (!unpack_u16(e, &profile->net.allow[i], NULL))
 				goto fail;
 			if (!unpack_u16(e, &profile->net.audit[i], NULL))