[apparmor] [PATCH 2/2] apparmor: Allow filtering based on secmark policy

[Matthew Garrett]

On Fri, May 18, 2018 at 12:06 PM Matthew Garrett <mjg59 at google.com> wrote:
> +static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32
secid,
> +                          struct common_audit_data *sa, struct sock *sk)
> +{
> +       int i, ret;
> +       struct aa_perms perms = { };
> +
> +       if (profile->secmark_count == 0)
> +               return 0;
> +
> +       for (i = 0; i < profile->secmark_count; i++) {
> +               if (!profile->secmark[i].secid) {
> +                       ret = apparmor_secmark_init(&profile->secmark[i]);
> +                       if (ret)
> +                               return ret;
> +               }
> +
> +               if (profile->secmark[i].secid == secid) {
> +                       if (profile->secmark[i].deny)
> +                               perms.deny = ALL_PERMS_MASK;
> +                       else
> +                               perms.allow = ALL_PERMS_MASK;
> +
> +                       if (profile->secmark[i].audit)
> +                               perms.audit = ALL_PERMS_MASK;
> +               }
> +       }
> +
> +       aa_apply_modes_to_perms(profile, &perms);
> +
> +       return aa_check_perms(profile, &perms, request, sa, audit_net_cb);
> +}

So, specifically, my question is about whether this is doing the right
thing. If someone writes a rule that's just:

network

then secmark_count should be 0 and the rule will grant full network access.
If someone has:

network label=foo

then secmark_count should be > 0, and the secmark policy checking will pass
for packets with a label of foo and fail otherwise. For

network
deny network label=foo

then secmark_count should be > 0, and packets with a label of foo will get
perms.deny set to ALL_PERMS_MASK. However, something with a label of bar
will end up with aa_apply_modes_to_perms being called without perms.allow
having been set, and I think that's going to do the wrong thing. The "easy"
thing to do would be to default to perms.allow = ALL_PERMS_MASK if nothing
matches, but that would mean:

network label=foo

on its own would allow non-foo packets to pass. So I think what actually
needs to happen here is for a bare network statement to imply a wildcard
secmark label? If I keep the secmark_count == 0 special case, I think this
can then be done in the parser - any parser version that understands the
label=foo syntax would then also generate a special wildcard secmark
statement in response to a bare network permission. Thoughts?