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

[Eric Chiang]

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