Return-Path: Received: from fieldses.org ([173.255.197.46]:33551 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753253AbbIWNPd (ORCPT ); Wed, 23 Sep 2015 09:15:33 -0400 Date: Wed, 23 Sep 2015 09:15:32 -0400 From: "J. Bruce Fields" To: Andreas Gruenbacher Cc: LKML , linux-fsdevel , linux-nfs@vger.kernel.org, linux-api@vger.kernel.org, linux-cifs@vger.kernel.org, linux-security-module@vger.kernel.org, Andreas Gruenbacher Subject: Re: [RFC v7 25/41] richacl: Isolate the owner and group classes Message-ID: <20150923131532.GB10464@fieldses.org> References: <1441448856-13478-1-git-send-email-agruenba@redhat.com> <1441448856-13478-26-git-send-email-agruenba@redhat.com> <20150922160637.GC15838@fieldses.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: On Wed, Sep 23, 2015 at 03:11:45PM +0200, Andreas Gruenbacher wrote: > 2015-09-22 18:06 GMT+02:00 J. Bruce Fields : > > On Sat, Sep 05, 2015 at 12:27:20PM +0200, Andreas Gruenbacher wrote: > >> When applying the file masks to an acl, we need to ensure that no > >> process gets more permissions than allowed by its file mask. > >> > >> This may require inserting an owner@ deny ace to ensure this if the > >> owner mask contains fewer permissions than the group or other mask. For > >> example, when applying mode 0466 to the following acl: > >> > >> everyone@:rw::allow > >> > >> A deny ace needs to be inserted so that the owner won't get elevated > >> write access: > >> > >> owner@:w::deny > >> everyone@:rw::allow > >> > >> Likewise, we may need to insert group class deny aces if the group mask > >> contains fewer permissions than the other mask. For example, when > >> applying mode 0646 to the following acl: > >> > >> owner@:rw::allow > >> everyone@:rw::allow > >> > >> A deny ace needs to be inserted so that the owning group won't get > >> elevated write access: > >> > >> owner@:rw::allow > >> group@:w::deny > >> everyone@:rw::allow > >> > >> Signed-off-by: Andreas Gruenbacher > >> --- > >> fs/richacl_compat.c | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 236 insertions(+) > >> > >> diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c > >> index 30bdc95..412844c 100644 > >> --- a/fs/richacl_compat.c > >> +++ b/fs/richacl_compat.c > >> @@ -494,3 +494,239 @@ richacl_set_other_permissions(struct richacl_alloc *alloc) > >> richace_change_mask(alloc, &ace, other_mask); > >> return 0; > >> } > >> + > >> +/** > >> + * richacl_max_allowed - maximum permissions that anybody is allowed > >> + */ > >> +static unsigned int > >> +richacl_max_allowed(struct richacl *acl) > >> +{ > >> + struct richace *ace; > >> + unsigned int allowed = 0; > >> + > >> + richacl_for_each_entry_reverse(ace, acl) { > >> + if (richace_is_inherit_only(ace)) > >> + continue; > >> + if (richace_is_allow(ace)) > >> + allowed |= ace->e_mask; > >> + else if (richace_is_deny(ace)) { > >> + if (richace_is_everyone(ace)) > >> + allowed &= ~ace->e_mask; > >> + } > >> + } > >> + return allowed; > >> +} > >> + > >> +/** > >> + * richacl_isolate_owner_class - limit the owner class to the owner file mask > >> + * @alloc: acl and number of allocated entries > >> + * > >> + * POSIX requires that after a chmod, the owner class is granted no more > >> + * permissions than the owner file permission bits. For richacls, this > >> + * means that the owner class must not be granted any permissions that the > >> + * owner mask does not include. > >> + * > >> + * When we apply file masks to an acl which grant more permissions to the group > >> + * or other class than to the owner class, we may end up in a situation where > >> + * the owner is granted additional permissions from other aces. For example, > >> + * given this acl: > >> + * > >> + * everyone:rwx::allow > >> + * > >> + * when file masks corresponding to mode 0466 are applied, after > >> + * richacl_propagate_everyone() and __richacl_apply_masks(), we end up with: > >> + * > >> + * owner@:r::allow > >> + * everyone@:rw::allow > > > > Are you sure? I didn't think richacl_apply_masks actually creates an > > owner@ entry in this case. Which is OK, just delete the owner@ ace from > > here and the following example and it still makes sense, I think. > > Hmm, the example can be fixed by applying more 0406 here instead of 0466. > > > (But: thanks in general for the examples in these comments, they're > > extremely helpful.) > > Yes, I think without them, the code cannot be reviewed properly. > > > I'd find it simpler to follow without the a_entries + a_count condition, > > maybe something like this (untested): > > > > [...] > > Great, let me further simplify this to: Works for me! (And feel free to add a Reviewed-by:.) --b. > > static int > richacl_isolate_owner_class(struct richacl_alloc *alloc) > { > struct richacl *acl = alloc->acl; > unsigned int deny = richacl_max_allowed(acl) & ~acl->a_owner_mask; > > if (deny) { > struct richace *ace; > > /* > * Figure out if we can update an existig OWNER@ DENY entry. > */ > richacl_for_each_entry(ace, acl) { > if (richace_is_inherit_only(ace)) > continue; > if (richace_is_allow(ace)) > break; > if (richace_is_owner(ace)) { > return richace_change_mask(alloc, &ace, > ace->e_mask | deny); > } > } > > /* Insert an owner@ deny entry at the front. */ > ace = acl->a_entries; > if (richacl_insert_entry(alloc, &ace)) > return -1; > ace->e_type = RICHACE_ACCESS_DENIED_ACE_TYPE; > ace->e_flags = RICHACE_SPECIAL_WHO; > ace->e_mask = deny; > ace->e_id.special = RICHACE_OWNER_SPECIAL_ID; > } > return 0; > } > > Thanks, > Andreas