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

On Wed, Sep 23, 2015 at 03:11:45PM +0200, Andreas Gruenbacher wrote:

2015-09-22 18:06 GMT+02:00 J. Bruce Fields [off-list ref]:

quoted

On Sat, Sep 05, 2015 at 12:27:20PM +0200, Andreas Gruenbacher wrote:

quoted

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 <redacted>
---
 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.

quoted

(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.

quoted

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