diff mbox

[RFC,v7,26/41] richacl: Apply the file masks to a richacl

Message ID 1441448856-13478-27-git-send-email-agruenba@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Gruenbacher Sept. 5, 2015, 10:27 a.m. UTC
Put all the pieces of the acl transformation puzzle together for
computing a richacl which has the file masks "applied" so that the
standard nfsv4 access check algorithm can be used on the richacl.

Signed-off-by: Andreas Gruenbacher <agruen@kernel.org>
---
 fs/richacl_compat.c     | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/richacl.h |   3 ++
 2 files changed, 113 insertions(+)

Comments

J. Bruce Fields Sept. 22, 2015, 7:11 p.m. UTC | #1
On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> Put all the pieces of the acl transformation puzzle together for
> computing a richacl which has the file masks "applied" so that the
> standard nfsv4 access check algorithm can be used on the richacl.
> 
> Signed-off-by: Andreas Gruenbacher <agruen@kernel.org>
> ---
>  fs/richacl_compat.c     | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/richacl.h |   3 ++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 412844c..9681efe 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -730,3 +730,113 @@ richacl_isolate_group_class(struct richacl_alloc *alloc)
>  	}
>  	return 0;
>  }
> +
> +/**
> + * __richacl_apply_masks  -  apply the file masks to all aces
> + * @alloc:	acl and number of allocated entries
> + *
> + * Apply the owner mask to owner@ aces, the other mask to
> + * everyone@ aces, and the group mask to all other aces.
> + *
> + * The previous transformations have brought the acl into a
> + * form in which applying the masks will not lead to the
> + * accidental loss of permissions anymore.
> + */
> +static int
> +__richacl_apply_masks(struct richacl_alloc *alloc, kuid_t owner)
> +{
> +	struct richace *ace;
> +
> +	richacl_for_each_entry(ace, alloc->acl) {
> +		unsigned int mask;
> +
> +		if (richace_is_inherit_only(ace) || !richace_is_allow(ace))
> +			continue;
> +		if (richace_is_owner(ace) ||
> +		    (richace_is_unix_user(ace) && uid_eq(owner, ace->e_id.uid)))
> +			mask = alloc->acl->a_owner_mask;

Is treating matching user aces like owner aces what you intended to do,
and if so, why?

--b.

> +		else if (richace_is_everyone(ace))
> +			mask = alloc->acl->a_other_mask;
> +		else
> +			mask = alloc->acl->a_group_mask;
> +		if (richace_change_mask(alloc, &ace, ace->e_mask & mask))
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * richacl_apply_masks  -  apply the masks to the acl
> + *
> + * Transform @acl so that the standard NFSv4 permission check algorithm (which
> + * is not aware of file masks) will compute the same access decisions as the
> + * richacl permission check algorithm (which looks at the acl and the file
> + * masks).
> + *
> + * This algorithm is split into several steps:
> + *
> + *   - Move everyone@ aces to the end of the acl.  This simplifies the other
> + *     transformations, and allows the everyone@ allow ace at the end of the
> + *     acl to eventually allow permissions to the other class only.
> + *
> + *   - Propagate everyone@ permissions up the acl.  This transformation makes
> + *     sure that the owner and group class aces won't lose any permissions when
> + *     we apply the other mask to the everyone@ allow ace at the end of the acl.
> + *
> + *   - Apply the file masks to all aces.
> + *
> + *   - Make sure the owner is granted the owner mask permissions.
> + *
> + *   - Make sure everyone is granted the other mask permissions.
> + *
> + *   - Make sure that the owner is not granted any permissions beyond the owner
> + *     mask from group class aces or from everyone@.
> + *
> + *   - Make sure that the group class is not granted any permissions from
> + *     everyone@.
> + *
> + * The algorithm is exact except for richacls which cannot be represented as an
> + * acl alone: for example, given this acl:
> + *
> + *    group@:rw::allow
> + *
> + * when file masks corresponding to mode 0600 are applied, the owner would only
> + * get rw access if he is a member of the owning group.  This algorithm would
> + * produce an empty acl in this case.  We fix this case by modifying
> + * richacl_permission() so that the group mask is always applied to group class
> + * aces.  With this fix, the owner would not have any access (beyond the
> + * implicit permissions always granted to owners).
> + *
> + * NOTE: Depending on the acl and file masks, this algorithm can increase the
> + * number of aces by almost a factor of three in the worst case. This may make
> + * the acl too large for some purposes.
> + */
> +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) ||
> +		    richacl_isolate_owner_class(&alloc) ||
> +		    richacl_isolate_group_class(&alloc)) {
> +			richacl_put(alloc.acl);
> +			return -ENOMEM;
> +		}
> +
> +		alloc.acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
> +		richacl_put(*acl);
> +		*acl = alloc.acl;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(richacl_apply_masks);
> diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> index 832b06c..a945f3c 100644
> --- a/include/linux/richacl.h
> +++ b/include/linux/richacl.h
> @@ -332,4 +332,7 @@ extern struct richacl *richacl_inherit(const struct richacl *, int);
>  extern int richacl_permission(struct inode *, const struct richacl *, int);
>  extern struct richacl *richacl_create(struct inode *, struct inode *);
>  
> +/* richacl_compat.c */
> +extern int richacl_apply_masks(struct richacl **, kuid_t);
> +
>  #endif /* __RICHACL_H */
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 22, 2015, 8:50 p.m. UTC | #2
On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> Put all the pieces of the acl transformation puzzle together for
> computing a richacl which has the file masks "applied" so that the
> standard nfsv4 access check algorithm can be used on the richacl.
> 
> Signed-off-by: Andreas Gruenbacher <agruen@kernel.org>
> ---
>  fs/richacl_compat.c     | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/richacl.h |   3 ++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 412844c..9681efe 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -730,3 +730,113 @@ richacl_isolate_group_class(struct richacl_alloc *alloc)
>  	}
>  	return 0;
>  }
> +
> +/**
> + * __richacl_apply_masks  -  apply the file masks to all aces
> + * @alloc:	acl and number of allocated entries
> + *
> + * Apply the owner mask to owner@ aces, the other mask to
> + * everyone@ aces, and the group mask to all other aces.
> + *
> + * The previous transformations have brought the acl into a
> + * form in which applying the masks will not lead to the
> + * accidental loss of permissions anymore.
> + */
> +static int
> +__richacl_apply_masks(struct richacl_alloc *alloc, kuid_t owner)
> +{
> +	struct richace *ace;
> +
> +	richacl_for_each_entry(ace, alloc->acl) {
> +		unsigned int mask;
> +
> +		if (richace_is_inherit_only(ace) || !richace_is_allow(ace))
> +			continue;
> +		if (richace_is_owner(ace) ||
> +		    (richace_is_unix_user(ace) && uid_eq(owner, ace->e_id.uid)))
> +			mask = alloc->acl->a_owner_mask;
> +		else if (richace_is_everyone(ace))
> +			mask = alloc->acl->a_other_mask;
> +		else
> +			mask = alloc->acl->a_group_mask;
> +		if (richace_change_mask(alloc, &ace, ace->e_mask & mask))
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * richacl_apply_masks  -  apply the masks to the acl
> + *
> + * Transform @acl so that the standard NFSv4 permission check algorithm (which
> + * is not aware of file masks) will compute the same access decisions as the
> + * richacl permission check algorithm (which looks at the acl and the file
> + * masks).
> + *
> + * This algorithm is split into several steps:
> + *
> + *   - Move everyone@ aces to the end of the acl.  This simplifies the other
> + *     transformations, and allows the everyone@ allow ace at the end of the
> + *     acl to eventually allow permissions to the other class only.
> + *
> + *   - Propagate everyone@ permissions up the acl.  This transformation makes
> + *     sure that the owner and group class aces won't lose any permissions when
> + *     we apply the other mask to the everyone@ allow ace at the end of the acl.
> + *
> + *   - Apply the file masks to all aces.
> + *
> + *   - Make sure the owner is granted the owner mask permissions.
> + *
> + *   - Make sure everyone is granted the other mask permissions.
> + *
> + *   - Make sure that the owner is not granted any permissions beyond the owner
> + *     mask from group class aces or from everyone@.
> + *
> + *   - Make sure that the group class is not granted any permissions from
> + *     everyone@.
> + *
> + * The algorithm is exact except for richacls which cannot be represented as an
> + * acl alone: for example, given this acl:
> + *
> + *    group@:rw::allow
> + *
> + * when file masks corresponding to mode 0600 are applied, the owner would only
> + * get rw access if he is a member of the owning group.  This algorithm would
> + * produce an empty acl in this case.  We fix this case by modifying
> + * richacl_permission() so that the group mask is always applied to group class
> + * aces.  With this fix, the owner would not have any access (beyond the
> + * implicit permissions always granted to owners).

That's a confusing way to put it: you say the algorithm isn't exact,
then say, actually, no, it *is* exact, because of a change we made to
richacls.

Better to just say from the start "the algorithm is exact", and then
explain why.  (Actually, the term "exact" isn't really defined here:
maybe say what you mean, something like "the resulting ACL will always
grant the same permissions that the original ACL would".)

> + *
> + * NOTE: Depending on the acl and file masks, this algorithm can increase the
> + * number of aces by almost a factor of three in the worst case. This may make
> + * the acl too large for some purposes.

We should maybe give some guidelines (e.g., "this won't happen if you
always use modes that grant more to group than user, and more to other
than group (if that's true)).

--b.

> + */
> +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) ||
> +		    richacl_isolate_owner_class(&alloc) ||
> +		    richacl_isolate_group_class(&alloc)) {
> +			richacl_put(alloc.acl);
> +			return -ENOMEM;
> +		}
> +
> +		alloc.acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
> +		richacl_put(*acl);
> +		*acl = alloc.acl;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(richacl_apply_masks);
> diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> index 832b06c..a945f3c 100644
> --- a/include/linux/richacl.h
> +++ b/include/linux/richacl.h
> @@ -332,4 +332,7 @@ extern struct richacl *richacl_inherit(const struct richacl *, int);
>  extern int richacl_permission(struct inode *, const struct richacl *, int);
>  extern struct richacl *richacl_create(struct inode *, struct inode *);
>  
> +/* richacl_compat.c */
> +extern int richacl_apply_masks(struct richacl **, kuid_t);
> +
>  #endif /* __RICHACL_H */
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 23, 2015, 7:18 p.m. UTC | #3
On Tue, Sep 22, 2015 at 03:11:08PM -0400, bfields wrote:
> On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> > Put all the pieces of the acl transformation puzzle together for
> > computing a richacl which has the file masks "applied" so that the
> > standard nfsv4 access check algorithm can be used on the richacl.
> > 
> > Signed-off-by: Andreas Gruenbacher <agruen@kernel.org>
> > ---
> >  fs/richacl_compat.c     | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/richacl.h |   3 ++
> >  2 files changed, 113 insertions(+)
> > 
> > diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> > index 412844c..9681efe 100644
> > --- a/fs/richacl_compat.c
> > +++ b/fs/richacl_compat.c
> > @@ -730,3 +730,113 @@ richacl_isolate_group_class(struct richacl_alloc *alloc)
> >  	}
> >  	return 0;
> >  }
> > +
> > +/**
> > + * __richacl_apply_masks  -  apply the file masks to all aces
> > + * @alloc:	acl and number of allocated entries
> > + *
> > + * Apply the owner mask to owner@ aces, the other mask to
> > + * everyone@ aces, and the group mask to all other aces.
> > + *
> > + * The previous transformations have brought the acl into a
> > + * form in which applying the masks will not lead to the
> > + * accidental loss of permissions anymore.
> > + */
> > +static int
> > +__richacl_apply_masks(struct richacl_alloc *alloc, kuid_t owner)
> > +{
> > +	struct richace *ace;
> > +
> > +	richacl_for_each_entry(ace, alloc->acl) {
> > +		unsigned int mask;
> > +
> > +		if (richace_is_inherit_only(ace) || !richace_is_allow(ace))
> > +			continue;
> > +		if (richace_is_owner(ace) ||
> > +		    (richace_is_unix_user(ace) && uid_eq(owner, ace->e_id.uid)))
> > +			mask = alloc->acl->a_owner_mask;
> 
> Is treating matching user aces like owner aces what you intended to do,
> and if so, why?

That does look wrong to me; in an example like:

	file owner bfields
	mask 0700, not WRITE_THROUGH
	bfields:rwx::allow

The permission algorithm grants nothing to anyone, but it looks to me
like richacl_apply_masks just leaves this as

	bfields:rwx::allow

but it would give the right result (an empty/deny-all ACL) if it weren't
for this odd case here.

--b.

> 
> --b.
> 
> > +		else if (richace_is_everyone(ace))
> > +			mask = alloc->acl->a_other_mask;
> > +		else
> > +			mask = alloc->acl->a_group_mask;
> > +		if (richace_change_mask(alloc, &ace, ace->e_mask & mask))
> > +			return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + * richacl_apply_masks  -  apply the masks to the acl
> > + *
> > + * Transform @acl so that the standard NFSv4 permission check algorithm (which
> > + * is not aware of file masks) will compute the same access decisions as the
> > + * richacl permission check algorithm (which looks at the acl and the file
> > + * masks).
> > + *
> > + * This algorithm is split into several steps:
> > + *
> > + *   - Move everyone@ aces to the end of the acl.  This simplifies the other
> > + *     transformations, and allows the everyone@ allow ace at the end of the
> > + *     acl to eventually allow permissions to the other class only.
> > + *
> > + *   - Propagate everyone@ permissions up the acl.  This transformation makes
> > + *     sure that the owner and group class aces won't lose any permissions when
> > + *     we apply the other mask to the everyone@ allow ace at the end of the acl.
> > + *
> > + *   - Apply the file masks to all aces.
> > + *
> > + *   - Make sure the owner is granted the owner mask permissions.
> > + *
> > + *   - Make sure everyone is granted the other mask permissions.
> > + *
> > + *   - Make sure that the owner is not granted any permissions beyond the owner
> > + *     mask from group class aces or from everyone@.
> > + *
> > + *   - Make sure that the group class is not granted any permissions from
> > + *     everyone@.
> > + *
> > + * The algorithm is exact except for richacls which cannot be represented as an
> > + * acl alone: for example, given this acl:
> > + *
> > + *    group@:rw::allow
> > + *
> > + * when file masks corresponding to mode 0600 are applied, the owner would only
> > + * get rw access if he is a member of the owning group.  This algorithm would
> > + * produce an empty acl in this case.  We fix this case by modifying
> > + * richacl_permission() so that the group mask is always applied to group class
> > + * aces.  With this fix, the owner would not have any access (beyond the
> > + * implicit permissions always granted to owners).
> > + *
> > + * NOTE: Depending on the acl and file masks, this algorithm can increase the
> > + * number of aces by almost a factor of three in the worst case. This may make
> > + * the acl too large for some purposes.
> > + */
> > +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) ||
> > +		    richacl_isolate_owner_class(&alloc) ||
> > +		    richacl_isolate_group_class(&alloc)) {
> > +			richacl_put(alloc.acl);
> > +			return -ENOMEM;
> > +		}
> > +
> > +		alloc.acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
> > +		richacl_put(*acl);
> > +		*acl = alloc.acl;
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(richacl_apply_masks);
> > diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> > index 832b06c..a945f3c 100644
> > --- a/include/linux/richacl.h
> > +++ b/include/linux/richacl.h
> > @@ -332,4 +332,7 @@ extern struct richacl *richacl_inherit(const struct richacl *, int);
> >  extern int richacl_permission(struct inode *, const struct richacl *, int);
> >  extern struct richacl *richacl_create(struct inode *, struct inode *);
> >  
> > +/* richacl_compat.c */
> > +extern int richacl_apply_masks(struct richacl **, kuid_t);
> > +
> >  #endif /* __RICHACL_H */
> > -- 
> > 2.4.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher Sept. 23, 2015, 8:29 p.m. UTC | #4
2015-09-23 21:18 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> On Tue, Sep 22, 2015 at 03:11:08PM -0400, bfields wrote:
>> user aces like owner aces what you intended to do,
>> and if so, why?
>
> That does look wrong to me; in an example like:
>
>         file owner bfields
>         mask 0700, not WRITE_THROUGH
>         bfields:rwx::allow
>
> The permission algorithm grants nothing to anyone, but it looks to me
> like richacl_apply_masks just leaves this as
>
>         bfields:rwx::allow
>
> but it would give the right result (an empty/deny-all ACL) if it weren't
> for this odd case here.

In POSIX ACLs, only the entry that best matches the process determines
the access permissions. For the file owner, this would always be the
"user::" entry, and such an entry always exists.

In richacls, permissions from multiple entries do accumulate; the
permission check algorithm does not pick a "best match". When bfields
owns a file and a "bfields:rwx::allow" entry exists, denying rwx
access to bfields would be very surprising. It makes more sense to put
user entries that match the current owner into the owner class, and
apply the owner mask instead of the group mask. This was working in an
earlier version but apparently broke at some point.

So the result that richacl_apply_masks computes here is correct, and
the permission check algorithm needs a little fix.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 23, 2015, 8:33 p.m. UTC | #5
On Wed, Sep 23, 2015 at 10:29:40PM +0200, Andreas Gruenbacher wrote:
> 2015-09-23 21:18 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> > On Tue, Sep 22, 2015 at 03:11:08PM -0400, bfields wrote:
> >> user aces like owner aces what you intended to do,
> >> and if so, why?
> >
> > That does look wrong to me; in an example like:
> >
> >         file owner bfields
> >         mask 0700, not WRITE_THROUGH
> >         bfields:rwx::allow
> >
> > The permission algorithm grants nothing to anyone, but it looks to me
> > like richacl_apply_masks just leaves this as
> >
> >         bfields:rwx::allow
> >
> > but it would give the right result (an empty/deny-all ACL) if it weren't
> > for this odd case here.
> 
> In POSIX ACLs, only the entry that best matches the process determines
> the access permissions. For the file owner, this would always be the
> "user::" entry, and such an entry always exists.
> 
> In richacls, permissions from multiple entries do accumulate; the
> permission check algorithm does not pick a "best match". When bfields
> owns a file and a "bfields:rwx::allow" entry exists, denying rwx
> access to bfields would be very surprising.

The same could be said if there's a group-i-belong-to:rwx::allow entry,
do we make that exception too?

--b.

> It makes more sense to put
> user entries that match the current owner into the owner class, and
> apply the owner mask instead of the group mask. This was working in an
> earlier version but apparently broke at some point.
> 
> So the result that richacl_apply_masks computes here is correct, and
> the permission check algorithm needs a little fix.
> 
> Thanks,
> Andreas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher Sept. 23, 2015, 8:40 p.m. UTC | #6
2015-09-23 22:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> The same could be said if there's a group-i-belong-to:rwx::allow entry,
> do we make that exception too?

We cannot because that would be incorrect for all other group members.

Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 23, 2015, 9:05 p.m. UTC | #7
On Wed, Sep 23, 2015 at 10:40:18PM +0200, Andreas Gruenbacher wrote:
> 2015-09-23 22:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> > The same could be said if there's a group-i-belong-to:rwx::allow entry,
> > do we make that exception too?
> 
> We cannot because that would be incorrect for all other group members.

OK.  So people have to learn how the group mask works anyway, and now
they have to learn a special exception to that rule.

I don't like having this exception.  Or making the richacl->v4acl
translation dependent on the owner.

But I admit it's surprising to that an 0700 mask with
"bfields:rwx::allow" ACL denies access to a bfields-owned file.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher Sept. 23, 2015, 10:14 p.m. UTC | #8
2015-09-23 23:05 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> On Wed, Sep 23, 2015 at 10:40:18PM +0200, Andreas Gruenbacher wrote:
>> 2015-09-23 22:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
>> > The same could be said if there's a group-i-belong-to:rwx::allow entry,
>> > do we make that exception too?
>>
>> We cannot because that would be incorrect for all other group members.
>
> OK.  So people have to learn how the group mask works anyway, and now
> they have to learn a special exception to that rule.
>
> I don't like having this exception.  Or making the richacl->v4acl
> translation dependent on the owner.
>
> But I admit it's surprising to that an 0700 mask with
> "bfields:rwx::allow" ACL denies access to a bfields-owned file.

I fully understand your point. This kind of acl is one of the the
first things people will try, and nobody is going to accept when
access is denied in this case though.

Things are made worse by the fact that Windows has the concept of
owner@ or group@ entries for inheritable permissions but not for
effective ones; it will always produce and expect "bfields:rwx::allow"
type entries instead of "owner@:rwx::allow" type entries. I'm not sure
if Samba could bridge that gap.

The fact that we cannot handle entries for groups the owner is in in a
similar way is not a big deal; it's not surprising that changing the
group file mode permission bits affects group entries.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 24, 2015, 3:28 p.m. UTC | #9
On Thu, Sep 24, 2015 at 12:14:40AM +0200, Andreas Gruenbacher wrote:
> 2015-09-23 23:05 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> > On Wed, Sep 23, 2015 at 10:40:18PM +0200, Andreas Gruenbacher wrote:
> >> 2015-09-23 22:33 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> >> > The same could be said if there's a group-i-belong-to:rwx::allow entry,
> >> > do we make that exception too?
> >>
> >> We cannot because that would be incorrect for all other group members.
> >
> > OK.  So people have to learn how the group mask works anyway, and now
> > they have to learn a special exception to that rule.
> >
> > I don't like having this exception.  Or making the richacl->v4acl
> > translation dependent on the owner.
> >
> > But I admit it's surprising to that an 0700 mask with
> > "bfields:rwx::allow" ACL denies access to a bfields-owned file.
> 
> I fully understand your point. This kind of acl is one of the the
> first things people will try, and nobody is going to accept when
> access is denied in this case though.
> 
> Things are made worse by the fact that Windows has the concept of
> owner@ or group@ entries for inheritable permissions but not for
> effective ones; it will always produce and expect "bfields:rwx::allow"
> type entries instead of "owner@:rwx::allow" type entries. I'm not sure
> if Samba could bridge that gap.

I guess Samba's only choice on reading an ACL will be to split OWNER@
ACEs into inheritable and effective parts and then replace the "who" on
the latter by the current owner.


On writing do you think it should try to translate ACEs for users
matching the current owner to OWNER@ ACEs, or are you assuming it should
leave those untouched?

Sambas needs here seem most likely to be the determining factor, so I
just want to make sure I understand.

--b.

> The fact that we cannot handle entries for groups the owner is in in a
> similar way is not a big deal; it's not surprising that changing the
> group file mode permission bits affects group entries.
> 
> Thanks,
> Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Gruenbacher Sept. 24, 2015, 3:48 p.m. UTC | #10
2015-09-24 17:28 GMT+02:00 J. Bruce Fields <bfields@fieldses.org>:
> I guess Samba's only choice on reading an ACL will be to split OWNER@
> ACEs into inheritable and effective parts and then replace the "who" on
> the latter by the current owner.

Right, that's when translating from richacls to Windows ACLs.

> On writing do you think it should try to translate ACEs for users
> matching the current owner to OWNER@ ACEs, or are you assuming it should
> leave those untouched?

In the other direction, from Windows ACLs to richacls, Samba at least
shouldn't have to do that mapping. There may still be cases where it
wants to do that mapping though.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
J. Bruce Fields Sept. 24, 2015, 6:33 p.m. UTC | #11
On Sat, Sep 05, 2015 at 12:27:21PM +0200, Andreas Gruenbacher wrote:
> Put all the pieces of the acl transformation puzzle together for
> computing a richacl which has the file masks "applied" so that the
> standard nfsv4 access check algorithm can be used on the richacl.
> 
> Signed-off-by: Andreas Gruenbacher <agruen@kernel.org>
> ---
>  fs/richacl_compat.c     | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/richacl.h |   3 ++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
> index 412844c..9681efe 100644
> --- a/fs/richacl_compat.c
> +++ b/fs/richacl_compat.c
> @@ -730,3 +730,113 @@ richacl_isolate_group_class(struct richacl_alloc *alloc)
>  	}
>  	return 0;
>  }
> +
> +/**
> + * __richacl_apply_masks  -  apply the file masks to all aces
> + * @alloc:	acl and number of allocated entries
> + *
> + * Apply the owner mask to owner@ aces, the other mask to
> + * everyone@ aces, and the group mask to all other aces.
> + *
> + * The previous transformations have brought the acl into a
> + * form in which applying the masks will not lead to the
> + * accidental loss of permissions anymore.
> + */
> +static int
> +__richacl_apply_masks(struct richacl_alloc *alloc, kuid_t owner)
> +{
> +	struct richace *ace;
> +
> +	richacl_for_each_entry(ace, alloc->acl) {
> +		unsigned int mask;
> +
> +		if (richace_is_inherit_only(ace) || !richace_is_allow(ace))
> +			continue;
> +		if (richace_is_owner(ace) ||
> +		    (richace_is_unix_user(ace) && uid_eq(owner, ace->e_id.uid)))
> +			mask = alloc->acl->a_owner_mask;
> +		else if (richace_is_everyone(ace))
> +			mask = alloc->acl->a_other_mask;
> +		else
> +			mask = alloc->acl->a_group_mask;
> +		if (richace_change_mask(alloc, &ace, ace->e_mask & mask))
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * richacl_apply_masks  -  apply the masks to the acl
> + *
> + * Transform @acl so that the standard NFSv4 permission check algorithm (which
> + * is not aware of file masks) will compute the same access decisions as the
> + * richacl permission check algorithm (which looks at the acl and the file
> + * masks).
> + *
> + * This algorithm is split into several steps:
> + *
> + *   - Move everyone@ aces to the end of the acl.  This simplifies the other
> + *     transformations, and allows the everyone@ allow ace at the end of the
> + *     acl to eventually allow permissions to the other class only.
> + *
> + *   - Propagate everyone@ permissions up the acl.  This transformation makes
> + *     sure that the owner and group class aces won't lose any permissions when
> + *     we apply the other mask to the everyone@ allow ace at the end of the acl.
> + *
> + *   - Apply the file masks to all aces.
> + *
> + *   - Make sure the owner is granted the owner mask permissions.
> + *
> + *   - Make sure everyone is granted the other mask permissions.
> + *
> + *   - Make sure that the owner is not granted any permissions beyond the owner
> + *     mask from group class aces or from everyone@.
> + *
> + *   - Make sure that the group class is not granted any permissions from
> + *     everyone@.
> + *
> + * The algorithm is exact except for richacls which cannot be represented as an
> + * acl alone: for example, given this acl:
> + *
> + *    group@:rw::allow
> + *
> + * when file masks corresponding to mode 0600 are applied, the owner would only
> + * get rw access if he is a member of the owning group.  This algorithm would
> + * produce an empty acl in this case.  We fix this case by modifying
> + * richacl_permission() so that the group mask is always applied to group class
> + * aces.  With this fix, the owner would not have any access (beyond the
> + * implicit permissions always granted to owners).
> + *
> + * NOTE: Depending on the acl and file masks, this algorithm can increase the
> + * number of aces by almost a factor of three in the worst case. This may make
> + * the acl too large for some purposes.
> + */
> +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.

--b.

> +		    richacl_isolate_owner_class(&alloc) ||
> +		    richacl_isolate_group_class(&alloc)) {
> +			richacl_put(alloc.acl);
> +			return -ENOMEM;
> +		}
> +
> +		alloc.acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
> +		richacl_put(*acl);
> +		*acl = alloc.acl;
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(richacl_apply_masks);
> diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> index 832b06c..a945f3c 100644
> --- a/include/linux/richacl.h
> +++ b/include/linux/richacl.h
> @@ -332,4 +332,7 @@ extern struct richacl *richacl_inherit(const struct richacl *, int);
>  extern int richacl_permission(struct inode *, const struct richacl *, int);
>  extern struct richacl *richacl_create(struct inode *, struct inode *);
>  
> +/* richacl_compat.c */
> +extern int richacl_apply_masks(struct richacl **, kuid_t);
> +
>  #endif /* __RICHACL_H */
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 412844c..9681efe 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -730,3 +730,113 @@  richacl_isolate_group_class(struct richacl_alloc *alloc)
 	}
 	return 0;
 }
+
+/**
+ * __richacl_apply_masks  -  apply the file masks to all aces
+ * @alloc:	acl and number of allocated entries
+ *
+ * Apply the owner mask to owner@ aces, the other mask to
+ * everyone@ aces, and the group mask to all other aces.
+ *
+ * The previous transformations have brought the acl into a
+ * form in which applying the masks will not lead to the
+ * accidental loss of permissions anymore.
+ */
+static int
+__richacl_apply_masks(struct richacl_alloc *alloc, kuid_t owner)
+{
+	struct richace *ace;
+
+	richacl_for_each_entry(ace, alloc->acl) {
+		unsigned int mask;
+
+		if (richace_is_inherit_only(ace) || !richace_is_allow(ace))
+			continue;
+		if (richace_is_owner(ace) ||
+		    (richace_is_unix_user(ace) && uid_eq(owner, ace->e_id.uid)))
+			mask = alloc->acl->a_owner_mask;
+		else if (richace_is_everyone(ace))
+			mask = alloc->acl->a_other_mask;
+		else
+			mask = alloc->acl->a_group_mask;
+		if (richace_change_mask(alloc, &ace, ace->e_mask & mask))
+			return -1;
+	}
+	return 0;
+}
+
+/**
+ * richacl_apply_masks  -  apply the masks to the acl
+ *
+ * Transform @acl so that the standard NFSv4 permission check algorithm (which
+ * is not aware of file masks) will compute the same access decisions as the
+ * richacl permission check algorithm (which looks at the acl and the file
+ * masks).
+ *
+ * This algorithm is split into several steps:
+ *
+ *   - Move everyone@ aces to the end of the acl.  This simplifies the other
+ *     transformations, and allows the everyone@ allow ace at the end of the
+ *     acl to eventually allow permissions to the other class only.
+ *
+ *   - Propagate everyone@ permissions up the acl.  This transformation makes
+ *     sure that the owner and group class aces won't lose any permissions when
+ *     we apply the other mask to the everyone@ allow ace at the end of the acl.
+ *
+ *   - Apply the file masks to all aces.
+ *
+ *   - Make sure the owner is granted the owner mask permissions.
+ *
+ *   - Make sure everyone is granted the other mask permissions.
+ *
+ *   - Make sure that the owner is not granted any permissions beyond the owner
+ *     mask from group class aces or from everyone@.
+ *
+ *   - Make sure that the group class is not granted any permissions from
+ *     everyone@.
+ *
+ * The algorithm is exact except for richacls which cannot be represented as an
+ * acl alone: for example, given this acl:
+ *
+ *    group@:rw::allow
+ *
+ * when file masks corresponding to mode 0600 are applied, the owner would only
+ * get rw access if he is a member of the owning group.  This algorithm would
+ * produce an empty acl in this case.  We fix this case by modifying
+ * richacl_permission() so that the group mask is always applied to group class
+ * aces.  With this fix, the owner would not have any access (beyond the
+ * implicit permissions always granted to owners).
+ *
+ * NOTE: Depending on the acl and file masks, this algorithm can increase the
+ * number of aces by almost a factor of three in the worst case. This may make
+ * the acl too large for some purposes.
+ */
+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) ||
+		    richacl_isolate_owner_class(&alloc) ||
+		    richacl_isolate_group_class(&alloc)) {
+			richacl_put(alloc.acl);
+			return -ENOMEM;
+		}
+
+		alloc.acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
+		richacl_put(*acl);
+		*acl = alloc.acl;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(richacl_apply_masks);
diff --git a/include/linux/richacl.h b/include/linux/richacl.h
index 832b06c..a945f3c 100644
--- a/include/linux/richacl.h
+++ b/include/linux/richacl.h
@@ -332,4 +332,7 @@  extern struct richacl *richacl_inherit(const struct richacl *, int);
 extern int richacl_permission(struct inode *, const struct richacl *, int);
 extern struct richacl *richacl_create(struct inode *, struct inode *);
 
+/* richacl_compat.c */
+extern int richacl_apply_masks(struct richacl **, kuid_t);
+
 #endif /* __RICHACL_H */