richacl: Possible other write-through fix

Commit Message

Andreas Gruenbacher Sept. 25, 2015, 4:45 p.m. UTC

2015-09-24 20:33 GMT+02:00 J. Bruce Fields <[e-mail]>:
> On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
>> +int
>> +richacl_apply_masks(struct richacl **acl, kuid_t owner)
>> +{
>> +     if ((*acl)->a_flags & RICHACL_MASKED) {
>> +             struct richacl_alloc alloc = {
>> +                     .acl = richacl_clone(*acl, GFP_KERNEL),
>> +                     .count = (*acl)->a_count,
>> +             };
>> +             if (!alloc.acl)
>> +                     return -ENOMEM;
>> +
>> +             if (richacl_move_everyone_aces_down(&alloc) ||
>> +                 richacl_propagate_everyone(&alloc) ||
>> +                 __richacl_apply_masks(&alloc, owner) ||
>> +                 richacl_set_owner_permissions(&alloc) ||
>> +                 richacl_set_other_permissions(&alloc) ||
>
> Hm, I'm a little unsure about this step: it seems like the one step that
> can actually change the permissions (making them more permissive, in
> this case), whereas the others are neutral.
>
> The following two steps should fix that, but I'm not sure they do.
>
> E.g. if I have an ACL like
>
>         mask 0777, WRITE_THROUGH
>         GROUP@:r--::allow
>
> I think it should result in
>
>         OWNER@:   rwx::allow
>         GROUP@:   -wx::deny
>         GROUP@:   r--::allow
>         EVERYONE@:rwx::allow
>
> But does the GROUP@ deny actually get in there?  It looks to me like the
> denies inserted by richacl_isolate_group_class only take into account
> the group mask, not the actual permissions granted to any group class
> user.
>
> I may be mising something.

Thanks for the test case and analysis. The group@ deny ace that should be
inserted here actually doesn't get inserted. I'm attaching a fix.

---

By the way, all those scenarios can easily be tries out using the test
suite in the user-space richacl package, even without a richacl enabled
kernel. In this case, without the fix, we get:

  $ make tests/richacl-apply-masks
  $ tests/richacl-apply-masks -m 777 group@:r::allow
  group@:r::allow
  everyone@:rwpx::allow

With the fix, we are now getting:

  $ tests/richacl-apply-masks -m 777 group@:r::allow
  owner@:rwpx::allow
  group@:r::allow
  group@:wpx::deny
  everyone@:rwpx::allow

Thanks,
Andreas

Comments

J. Bruce Fields Sept. 25, 2015, 6:36 p.m. UTC | #1

On Fri, Sep 25, 2015 at 06:45:59PM +0200, Andreas Gruenbacher wrote:
> 2015-09-24 20:33 GMT+02:00 J. Bruce Fields <[e-mail]>:
> > On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> >> +int
> >> +richacl_apply_masks(struct richacl **acl, kuid_t owner)
> >> +{
> >> +     if ((*acl)->a_flags & RICHACL_MASKED) {
> >> +             struct richacl_alloc alloc = {
> >> +                     .acl = richacl_clone(*acl, GFP_KERNEL),
> >> +                     .count = (*acl)->a_count,
> >> +             };
> >> +             if (!alloc.acl)
> >> +                     return -ENOMEM;
> >> +
> >> +             if (richacl_move_everyone_aces_down(&alloc) ||
> >> +                 richacl_propagate_everyone(&alloc) ||
> >> +                 __richacl_apply_masks(&alloc, owner) ||
> >> +                 richacl_set_owner_permissions(&alloc) ||
> >> +                 richacl_set_other_permissions(&alloc) ||
> >
> > Hm, I'm a little unsure about this step: it seems like the one step that
> > can actually change the permissions (making them more permissive, in
> > this case), whereas the others are neutral.
> >
> > The following two steps should fix that, but I'm not sure they do.
> >
> > E.g. if I have an ACL like
> >
> >         mask 0777, WRITE_THROUGH
> >         GROUP@:r--::allow
> >
> > I think it should result in
> >
> >         OWNER@:   rwx::allow
> >         GROUP@:   -wx::deny
> >         GROUP@:   r--::allow
> >         EVERYONE@:rwx::allow
> >
> > But does the GROUP@ deny actually get in there?  It looks to me like the
> > denies inserted by richacl_isolate_group_class only take into account
> > the group mask, not the actual permissions granted to any group class
> > user.
> >
> > I may be mising something.
> 
> Thanks for the test case and analysis. The group@ deny ace that should be
> inserted here actually doesn't get inserted. I'm attaching a fix.

This looks correct with the fix, thanks!

One nit:

>  		if (richacl_move_everyone_aces_down(&alloc) ||
>  		    richacl_propagate_everyone(&alloc) ||
>  		    __richacl_apply_masks(&alloc, owner) ||
> +		    richacl_set_other_permissions(&alloc, &added) ||

It's still the case that this step lacks the property shared by the
other step, that it leaves permissions unchanged.  Instead it increases
permissions, then the following step fixes the problem.  That
complicates review slightly.

But I'm not sure that matters much.

Reviewed-by: J. Bruce Fields <[e-mail]>

--b.

> +		    richacl_isolate_group_class(&alloc, added) ||
>  		    richacl_set_owner_permissions(&alloc) ||
> -		    richacl_set_other_permissions(&alloc) ||
> -		    richacl_isolate_owner_class(&alloc) ||
> -		    richacl_isolate_group_class(&alloc)) {
> +		    richacl_isolate_owner_class(&alloc)) {
>  			richacl_put(alloc.acl);
>  			return -ENOMEM;
>  		}
> -- 
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 726d796..bc0bcfe 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -463,23 +463,28 @@  richacl_set_owner_permissions(struct richacl_alloc *alloc)
 
 /**
  * richacl_set_other_permissions  -  set the other permissions to the other mask
+ * @alloc:	acl and number of allocated entries
+ * @added:	permissions added for everyone@
  *
  * Change the acl so that everyone@ is granted the permissions set in the other
  * mask.  This leaves at most one efective everyone@ allow entry at the end of
- * the acl.
+ * the acl.  If everyone@ end up being granted additional permissions, these
+ * permissions are returned in @added.
  */
 static int
-richacl_set_other_permissions(struct richacl_alloc *alloc)
+richacl_set_other_permissions(struct richacl_alloc *alloc, unsigned int *added)
 {
 	struct richacl *acl = alloc->acl;
 	unsigned int x = RICHACE_POSIX_ALWAYS_ALLOWED;
 	unsigned int other_mask = acl->a_other_mask & ~x;
-	struct richace *ace = acl->a_entries + acl->a_count - 1;
+	struct richace *ace;
 
 	if (!(other_mask &&
 	      (acl->a_flags & RICHACL_WRITE_THROUGH)))
 		return 0;
 
+	*added = other_mask;
+	ace = acl->a_entries + acl->a_count - 1;
 	if (acl->a_count == 0 ||
 	    !richace_is_everyone(ace) ||
 	    richace_is_inherit_only(ace)) {
@@ -490,8 +495,10 @@  richacl_set_other_permissions(struct richacl_alloc *alloc)
 		ace->e_flags = RICHACE_SPECIAL_WHO;
 		ace->e_mask = other_mask;
 		ace->e_id.special = RICHACE_EVERYONE_SPECIAL_ID;
-	} else
+	} else {
+		*added &= ~ace->e_mask;
 		richace_change_mask(alloc, &ace, other_mask);
+	}
 	return 0;
 }
 
@@ -645,6 +652,7 @@  __richacl_isolate_who(struct richacl_alloc *alloc, struct richace *who,
 /**
  * richacl_isolate_group_class  -  limit the group class to the group file mask
  * @alloc:	acl and number of allocated entries
+ * @deny:	additional permissions to deny
  *
  * POSIX requires that after a chmod, the group class is granted no more
  * permissions than the group file permission bits.  For richacls, this
@@ -679,21 +687,20 @@  __richacl_isolate_who(struct richacl_alloc *alloc, struct richace *who,
  *    everyone@:rw::allow
  */
 static int
-richacl_isolate_group_class(struct richacl_alloc *alloc)
+richacl_isolate_group_class(struct richacl_alloc *alloc, unsigned int deny)
 {
 	struct richace who = {
 		.e_flags = RICHACE_SPECIAL_WHO,
 		.e_id.special = RICHACE_GROUP_SPECIAL_ID,
 	};
 	struct richace *ace;
-	unsigned int deny;
 
 	if (!alloc->acl->a_count)
 		return 0;
 	ace = alloc->acl->a_entries + alloc->acl->a_count - 1;
 	if (richace_is_inherit_only(ace) || !richace_is_everyone(ace))
 		return 0;
-	deny = ace->e_mask & ~alloc->acl->a_group_mask;
+	deny |= ace->e_mask & ~alloc->acl->a_group_mask;
 
 	if (deny) {
 		unsigned int n;
@@ -806,16 +813,17 @@  richacl_apply_masks(struct richacl **acl, kuid_t owner)
 			.acl = richacl_clone(*acl, GFP_KERNEL),
 			.count = (*acl)->a_count,
 		};
+		unsigned int added = 0;
+
 		if (!alloc.acl)
 			return -ENOMEM;
-
 		if (richacl_move_everyone_aces_down(&alloc) ||
 		    richacl_propagate_everyone(&alloc) ||
 		    __richacl_apply_masks(&alloc, owner) ||
+		    richacl_set_other_permissions(&alloc, &added) ||
+		    richacl_isolate_group_class(&alloc, added) ||
 		    richacl_set_owner_permissions(&alloc) ||
-		    richacl_set_other_permissions(&alloc) ||
-		    richacl_isolate_owner_class(&alloc) ||
-		    richacl_isolate_group_class(&alloc)) {
+		    richacl_isolate_owner_class(&alloc)) {
 			richacl_put(alloc.acl);
 			return -ENOMEM;
 		}