Re: [RFC v7 25/41] richacl: Isolate the owner and group classes

["J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>]

On Wed, Sep 23, 2015 at 03:11:45PM +0200, Andreas Gruenbacher wrote:
> 2015-09-22 18:06 GMT+02:00 J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>:
> > On Sat, Sep 05, 2015 at 12:27:20PM +0200, Andreas Gruenbacher wrote:
> >> When applying the file masks to an acl, we need to ensure that no
> >> process gets more permissions than allowed by its file mask.
> >>
> >> This may require inserting an owner@ deny ace to ensure this if the
> >> owner mask contains fewer permissions than the group or other mask.  For
> >> example, when applying mode 0466 to the following acl:
> >>
> >>    everyone@:rw::allow
> >>
> >> A deny ace needs to be inserted so that the owner won't get elevated
> >> write access:
> >>
> >>    owner@:w::deny
> >>    everyone@:rw::allow
> >>
> >> Likewise, we may need to insert group class deny aces if the group mask
> >> contains fewer permissions than the other mask.  For example, when
> >> applying mode 0646 to the following acl:
> >>
> >>    owner@:rw::allow
> >>    everyone@:rw::allow
> >>
> >> A deny ace needs to be inserted so that the owning group won't get
> >> elevated write access:
> >>
> >>    owner@:rw::allow
> >>    group@:w::deny
> >>    everyone@:rw::allow
> >>
> >> Signed-off-by: Andreas Gruenbacher <agruen-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> ---
> >>  fs/richacl_compat.c | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 236 insertions(+)
> >>
> >> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> >> index 30bdc95..412844c 100644
> >> --- a/fs/richacl_compat.c
> >> +++ b/fs/richacl_compat.c
> >> @@ -494,3 +494,239 @@ richacl_set_other_permissions(struct richacl_alloc *alloc)
> >>               richace_change_mask(alloc, &ace, other_mask);
> >>       return 0;
> >>  }
> >> +
> >> +/**
> >> + * richacl_max_allowed  -  maximum permissions that anybody is allowed
> >> + */
> >> +static unsigned int
> >> +richacl_max_allowed(struct richacl *acl)
> >> +{
> >> +     struct richace *ace;
> >> +     unsigned int allowed = 0;
> >> +
> >> +     richacl_for_each_entry_reverse(ace, acl) {
> >> +             if (richace_is_inherit_only(ace))
> >> +                     continue;
> >> +             if (richace_is_allow(ace))
> >> +                     allowed |= ace->e_mask;
> >> +             else if (richace_is_deny(ace)) {
> >> +                     if (richace_is_everyone(ace))
> >> +                             allowed &= ~ace->e_mask;
> >> +             }
> >> +     }
> >> +     return allowed;
> >> +}
> >> +
> >> +/**
> >> + * richacl_isolate_owner_class  -  limit the owner class to the owner file mask
> >> + * @alloc:   acl and number of allocated entries
> >> + *
> >> + * POSIX requires that after a chmod, the owner class is granted no more
> >> + * permissions than the owner file permission bits.  For richacls, this
> >> + * means that the owner class must not be granted any permissions that the
> >> + * owner mask does not include.
> >> + *
> >> + * When we apply file masks to an acl which grant more permissions to the group
> >> + * or other class than to the owner class, we may end up in a situation where
> >> + * the owner is granted additional permissions from other aces.  For example,
> >> + * given this acl:
> >> + *
> >> + *    everyone:rwx::allow
> >> + *
> >> + * when file masks corresponding to mode 0466 are applied, after
> >> + * richacl_propagate_everyone() and __richacl_apply_masks(), we end up with:
> >> + *
> >> + *    owner@:r::allow
> >> + *    everyone@:rw::allow
> >
> > Are you sure?  I didn't think richacl_apply_masks actually creates an
> > owner@ entry in this case.  Which is OK, just delete the owner@ ace from
> > here and the following example and it still makes sense, I think.
> 
> Hmm, the example can be fixed by applying more 0406 here instead of 0466.
> 
> > (But: thanks in general for the examples in these comments, they're
> > extremely helpful.)
> 
> Yes, I think without them, the code cannot be reviewed properly.
> 
> > I'd find it simpler to follow without the  a_entries + a_count condition,
> > maybe something like this (untested):
> >
> > [...]
> 
> Great, let me further simplify this to:

Works for me!  (And feel free to add a Reviewed-by:.)

--b.

> 
> static int
> richacl_isolate_owner_class(struct richacl_alloc *alloc)
> {
>         struct richacl *acl = alloc->acl;
>         unsigned int deny = richacl_max_allowed(acl) & ~acl->a_owner_mask;
> 
>         if (deny) {
>                 struct richace *ace;
> 
>                 /*
>                  * Figure out if we can update an existig OWNER@ DENY entry.
>                  */
>                 richacl_for_each_entry(ace, acl) {
>                         if (richace_is_inherit_only(ace))
>                                 continue;
>                         if (richace_is_allow(ace))
>                                 break;
>                         if (richace_is_owner(ace)) {
>                                 return richace_change_mask(alloc, &ace,
>                                                            ace->e_mask | deny);
>                         }
>                 }
> 
>                 /* Insert an owner@ deny entry at the front. */
>                 ace = acl->a_entries;
>                 if (richacl_insert_entry(alloc, &ace))
>                         return -1;
>                 ace->e_type = RICHACE_ACCESS_DENIED_ACE_TYPE;
>                 ace->e_flags = RICHACE_SPECIAL_WHO;
>                 ace->e_mask = deny;
>                 ace->e_id.special = RICHACE_OWNER_SPECIAL_ID;
>         }
>         return 0;
> }
> 
> Thanks,
> Andreas