Bug#676515: linux-2.6: AppArmor totally broken
On 06/07/2012 07:34 AM, Ben Hutchings wrote:
> On Thu, 2012-06-07 at 15:35 +0200, intrigeri@debian.org wrote:
>> Package: linux-2.6
>> Severity: normal
>> Version: 3.2.19-1
>> Tags: patch
>> X-Debbugs-CC: john.johansen@canonical.com, kees@debian.org, micah@riseup.net
>>
>> Hi,
>>
>> the AppArmor compatibility patch applied to fix #661151
>> totally breaks AppArmor support; this is a regression.
>> Details: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661151#83
>>
>> Applying the "AppArmor: compatibility patch for v5 network controll"
>> on top of the already applied one fixes the problem for me.
>> I did test with the patch that can be found there:
>> http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-precise.git;a=commit;h=d253e5fb4a6b552e7cd2a3c80934ab4f92faec97
>>
>> Please consider applying this network compatibility patch,
>> not only to fix this regression,
>> but also to get us working network mediation.
>
> So these independent patches really aren't... or else userland gets
> confused if we apply just the one.
>
Hrmmm, no they should be. That is a bug in userland that needs to be
fixed. I do know that they used to work when applied separately. It
certainly not a configuration that gets a lot of testing, but it
should have worked. I will look into it and figure out what is causing
it to break.
> Looking at the network controller patch:
>
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
> [...]
>> @@ -621,6 +622,104 @@ static int apparmor_task_setrlimit(struct task_struct *task,
>> return error;
>> }
>>
>> +static int apparmor_socket_create(int family, int type, int protocol, int kern)
>> +{
>> + struct aa_profile *profile;
>> + int error = 0;
>> +
>> + if (kern)
>> + return 0;
>
> If we don't want to restrict sockets used by the kernel, don't we need
> to store the kern flag for later use by aa_revalidate_sk()?
>
For how apparmor is generally deployed it can get away with this, the
kernel bits generally bail out earlier on the check for unconfined.
That is not to say it isn't a good idea, or that it shouldn't be done.
The fact is this patch is going to be replaced with completely rewritten
controls, that do store info on the socket, it just hasn't happened yet
due to resources and priorities (not my priorities).
>> + profile = __aa_current_profile();
>> + if (!unconfined(profile))
>> + error = aa_net_perm(OP_CREATE, profile, family, type, protocol,
>> + NULL);
>> + return error;
>> +}
> [...]
>> --- /dev/null
>> +++ b/security/apparmor/net.c
> [...]
>> +static int audit_net(struct aa_profile *profile, int op, u16 family, int type,
>> + int protocol, struct sock *sk, int error)
>> +{
> [...]
>> + } else {
>> + u16 quiet_mask = profile->net.quiet[sa.u.net.family];
>> + u16 kill_mask = 0;
>> + u16 denied = (1 << sa.aad.net.type) & ~quiet_mask;
>> +
>> + if (denied & kill_mask)
>> + audit_type = AUDIT_APPARMOR_KILL;
>> +
>> + if ((denied & quiet_mask) &&
>
> Since denied has already been masked with ~quiet_mask, this condition
> can never be true.
>
indeed
>> + AUDIT_MODE(profile) != AUDIT_NOQUIET &&
>> + AUDIT_MODE(profile) != AUDIT_ALL)
>> + return COMPLAIN_MODE(profile) ? 0 : sa.aad.error;
>> + }
>> +
>> + return aa_audit(audit_type, profile, GFP_KERNEL, &sa, audit_cb);
>> +}
> [...]
>> +/**
>> + * aa_revalidate_sk - Revalidate access to a sock
>> + * @op: operation being checked
>> + * @sk: sock being revalidated (NOT NULL)
>> + *
>> + * Returns: %0 else error if permission denied
>> + */
>> +int aa_revalidate_sk(int op, struct sock *sk)
>> +{
>> + struct aa_profile *profile;
>> + int error = 0;
>> +
>> + /* aa_revalidate_sk should not be called from interrupt context
>> + * don't mediate these calls as they are not task related
>> + */
>> + if (in_interrupt())
>> + return 0;
>
> I wonder why this is being checked at all.
>
Good question, I will have to dig into it.
>> + profile = __aa_current_profile();
>> + if (!unconfined(profile))
>> + error = aa_net_perm(op, profile, sk->sk_family, sk->sk_type,
>> + sk->sk_protocol, sk);
>> +
>> + return error;
>> +}
> [...]
>
> Ben.
>
Reply to: