| Message ID | 1441448856-13478-27-git-send-email-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(+)