[apparmor] [PATCH V2] security/apparmor: fix matching on presence of extended attributes

[Eric Chiang]

Bumping this now that we're past the holidays :)

On Fri, Dec 21, 2018 at 11:25 AM Eric Chiang <ericchiang at google.com> wrote:
>
> Hey Seth,
>
> The proposed userland parser changes explicitly won't pass a
> zero-sized array. However, modifying the parser to pass zero-sized
> arrays causes kcalloc(0) to return ZERO_SIZE_PTR and the policy still
> works as if the array hadn't been passed at all.
>
> Note that other uses of unpack_array don't check for size == 0. If I'm
> missing an issue here (which is totally possible), we need to add
> checks to those calls too:
>
> $ ag -r -A5  '= unpack_array' security/apparmor/
> security/apparmor/policy_unpack.c
> 455:            size = unpack_array(e, NULL);
> 456-            /* currently 4 exec bits and entries 0-3 are reserved iupcx */
> 457-            if (size > 16 - 4)
> 458-                    goto fail;
> 459-            profile->file.trans.table = kcalloc(size, sizeof(char *),
> 460-                                                GFP_KERNEL);
> --
> 523:            size = unpack_array(e, NULL);
> 524-            profile->xattr_count = size;
> 525-            profile->xattrs = kcalloc(size, sizeof(char *), GFP_KERNEL);
> 526-            if (!profile->xattrs)
> 527-                    goto fail;
> 528-            for (i = 0; i < size; i++) {
> --
> 551:            size = unpack_array(e, NULL);
> 552-
> 553-            profile->secmark = kcalloc(size, sizeof(struct aa_secmark),
> 554-                                       GFP_KERNEL);
> 555-            if (!profile->secmark)
> 556-                    goto fail;
> --
> 600:            size = unpack_array(e, NULL);
> 601-            if (size > RLIM_NLIMITS)
> 602-                    goto fail;
> 603-            for (i = 0; i < size; i++) {
> 604-                    u64 tmp2 = 0;
> 605-                    int a = aa_map_resource(i);
>
>
>
> On Thu, Dec 20, 2018 at 8:30 PM Seth Arnold <seth.arnold at canonical.com> wrote:
> >
> > On Thu, Dec 20, 2018 at 01:28:38PM -0800, Eric Chiang wrote:
> > > --- a/security/apparmor/policy_unpack.c
> > > +++ b/security/apparmor/policy_unpack.c
> > > @@ -535,6 +535,24 @@ static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
> > >                       goto fail;
> > >       }
> > >
> > > +     if (unpack_nameX(e, AA_STRUCT, "xattr_keys")) {
> > > +             int i, size;
> > > +
> > > +             size = unpack_array(e, NULL);
> > > +             profile->xattr_keys_count = size;
> > > +             profile->xattr_keys = kcalloc(size, sizeof(char *), GFP_KERNEL);
> >
> > Do we need to worry about a zero-size array here?
> >
> > Thanks
> > --
> > AppArmor mailing list
> > AppArmor at lists.ubuntu.com
> > Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor