lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <765cc37a8a46ab750bd6d67b1dc9ffd557923bde.camel@kernel.org>
Date:   Thu, 20 Jul 2023 06:38:09 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Andreas Grünbacher 
        <andreas.gruenbacher@...il.com>
Cc:     Chuck Lever <chuck.lever@...cle.com>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Ondrej Valousek <ondrej.valousek@...semi.com>,
        Andreas Gruenbacher <agruen@...hat.com>
Subject: Re: [PATCH] nfsd: inherit required unset default acls from
 effective set

On Thu, 2023-07-20 at 10:43 +0200, Andreas Grünbacher wrote:
> Am Mi., 19. Juli 2023 um 23:22 Uhr schrieb Andreas Grünbacher
> <andreas.gruenbacher@...il.com>:
> > 
> > Hi Jeff,
> > 
> > this patch seems useful, thanks.
> > 
> > Am Mi., 19. Juli 2023 um 19:56 Uhr schrieb Jeff Layton <jlayton@...nel.org>:
> > > A well-formed NFSv4 ACL will always contain OWNER@...OUP@...ERYONE@
> > > ACEs, but there is no requirement for inheritable entries for those
> > > entities. POSIX ACLs must always have owner/group/other entries, even for a
> > > default ACL.
> > 
> > NFSv4 ACLs actually don't *need* to have OWNER@...OUP@...ERYONE@
> > entries; that's merely a result of translating POSIX ACLs (or file
> > modes) to NFSv4 ACLs.
> > 

RFC 8881, section 6.4 says:

"The server that supports both mode and ACL must take care to
synchronize the MODE4_*USR, MODE4_*GRP, and MODE4_*OTH bits with the
ACEs that have respective who fields of "OWNER@", "GROUP@", and
"EVERYONE@". This way, the client can see if semantically equivalent
access permissions exist whether the client asks for the owner,
owner_group, and mode attributes or for just the ACL."

...so technically you're correct for a generic NFS server, but in the
Linux nfsd case, we have to have them.

> > > nfsd builds the default ACL from inheritable ACEs, but the current code
> > > just leaves any unspecified ACEs zeroed out. The result is that adding a
> > > default user or group ACE to an inode can leave it with unwanted deny
> > > entries.
> > > 
> > > For instance, a newly created directory with no acl will look something
> > > like this:
> > > 
> > >         # NFSv4 translation by server
> > >         A::OWNER@:rwaDxtTcCy
> > >         A::GROUP@:rxtcy
> > >         A::EVERYONE@:rxtcy
> > > 
> > >         # POSIX ACL of underlying file
> > >         user::rwx
> > >         group::r-x
> > >         other::r-x
> > > 
> > > ...if I then add new v4 ACE:
> > > 
> > >         nfs4_setfacl -a A:fd:1000:rwx /mnt/local/test
> > > 
> > > ...I end up with a result like this today:
> > > 
> > >         user::rwx
> > >         user:1000:rwx
> > >         group::r-x
> > >         mask::rwx
> > >         other::r-x
> > >         default:user::---
> > >         default:user:1000:rwx
> > >         default:group::---
> > >         default:mask::rwx
> > >         default:other::---
> > > 
> > >         A::OWNER@:rwaDxtTcCy
> > >         A::1000:rwaDxtcy
> > >         A::GROUP@:rxtcy
> > >         A::EVERYONE@:rxtcy
> > >         D:fdi:OWNER@:rwaDx
> > >         A:fdi:OWNER@:tTcCy
> > >         A:fdi:1000:rwaDxtcy
> > >         A:fdi:GROUP@:tcy
> > >         A:fdi:EVERYONE@:tcy
> > > 
> > > ...which is not at all expected. Adding a single inheritable allow ACE
> > > should not result in everyone else losing access.
> > > 
> > > The setfacl command solves a silimar issue by copying owner/group/other
> > > entries from the effective ACL when none of them are set:
> > > 
> > >     "If a Default ACL entry is created, and the  Default  ACL  contains  no
> > >      owner,  owning group,  or  others  entry,  a  copy of the ACL owner,
> > >      owning group, or others entry is added to the Default ACL.
> > > 
> > > Having nfsd do the same provides a more sane result (with no deny ACEs
> > > in the resulting set):
> > > 
> > >         user::rwx
> > >         user:1000:rwx
> > >         group::r-x
> > >         mask::rwx
> > >         other::r-x
> > >         default:user::rwx
> > >         default:user:1000:rwx
> > >         default:group::r-x
> > >         default:mask::rwx
> > >         default:other::r-x
> > > 
> > >         A::OWNER@:rwaDxtTcCy
> > >         A::1000:rwaDxtcy
> > >         A::GROUP@:rxtcy
> > >         A::EVERYONE@:rxtcy
> > >         A:fdi:OWNER@:rwaDxtTcCy
> > >         A:fdi:1000:rwaDxtcy
> > >         A:fdi:GROUP@:rxtcy
> > >         A:fdi:EVERYONE@:rxtcy
> > 
> > This resulting NFSv4 ACL is still rather dull; we end up with an
> > inherit-only entry for each effective entry. Those could all be
> > combined, resulting in:
> > 
> >          A:fd:OWNER@:rwaDxtTcCy
> >          A:fd:1000:rwaDxtcy
> >          A:fd:GROUP@:rxtcy
> >          A:fd:EVERYONE@:rxtcy
> > 
> > This will be the common case, so maybe matching entry pairs can either
> > be recombined or not generated in the first place as a further
> > improvement.
> > 

To be clear, this patch fixes the NFSv4->POSIX ACL translation. The
problem you're describing above is more with the POSIX->NFSv4
translation. I don't think we can make the resulting POSIX ACLs more
concise.


> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136452
> > > Reported-by: Ondrej Valousek <ondrej.valousek@...semi.com>
> > > Suggested-by: Andreas Gruenbacher <agruen@...hat.com>
> > > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > > ---
> > >  fs/nfsd/nfs4acl.c | 32 +++++++++++++++++++++++++++++---
> > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
> > > index 518203821790..64e45551d1b6 100644
> > > --- a/fs/nfsd/nfs4acl.c
> > > +++ b/fs/nfsd/nfs4acl.c
> > > @@ -441,7 +441,8 @@ struct posix_ace_state_array {
> > >   * calculated so far: */
> > > 
> > >  struct posix_acl_state {
> > > -       int empty;
> > > +       bool empty;
> > > +       unsigned char valid;
> > 
> > Hmm, "valid" is a bitmask here but it only matters whether it is zero.
> > Shouldn't a bool be good enough? Also, this variable indicates whether
> > special "who" values are present (and which), so the name "valid"
> > probably isn't the best choice.
> > 

Yep, a bool would be fine. This patch went through a bunch of different
revisions and in one, I needed know which individual fields were set. I
kept that here since the storage requirements are still the same, and it
might be more useful for debugging purposes in the future.

> > >         struct posix_ace_state owner;
> > >         struct posix_ace_state group;
> > >         struct posix_ace_state other;
> > > @@ -457,7 +458,7 @@ init_state(struct posix_acl_state *state, int cnt)
> > >         int alloc;
> > > 
> > >         memset(state, 0, sizeof(struct posix_acl_state));
> > > -       state->empty = 1;
> > > +       state->empty = true;
> > >         /*
> > >          * In the worst case, each individual acl could be for a distinct
> > >          * named user or group, but we don't know which, so we allocate
> > > @@ -624,7 +625,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >         u32 mask = ace->access_mask;
> > >         int i;
> > > 
> > > -       state->empty = 0;
> > > +       state->empty = false;
> > > 
> > >         switch (ace2type(ace)) {
> > >         case ACL_USER_OBJ:
> > > @@ -633,6 +634,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >                 } else {
> > >                         deny_bits(&state->owner, mask);
> > >                 }
> > > +               state->valid |= ACL_USER_OBJ;
> > >                 break;
> > >         case ACL_USER:
> > >                 i = find_uid(state, ace->who_uid);
> > > @@ -655,6 +657,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >                         deny_bits_array(state->users, mask);
> > >                         deny_bits_array(state->groups, mask);
> > >                 }
> > > +               state->valid |= ACL_GROUP_OBJ;
> > >                 break;
> > >         case ACL_GROUP:
> > >                 i = find_gid(state, ace->who_gid);
> > > @@ -686,6 +689,7 @@ static void process_one_v4_ace(struct posix_acl_state *state,
> > >                         deny_bits_array(state->users, mask);
> > >                         deny_bits_array(state->groups, mask);
> > >                 }
> > > +               state->valid |= ACL_OTHER;
> > >         }
> > >  }
> > > 
> > > @@ -726,6 +730,28 @@ static int nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl,
> > >                 if (!(ace->flag & NFS4_ACE_INHERIT_ONLY_ACE))
> > >                         process_one_v4_ace(&effective_acl_state, ace);
> > >         }
> > > +
> > > +       /*
> > > +        * At this point, the default ACL may have zeroed-out entries for owner,
> > > +        * group and other. That usually results in a non-sensical resulting ACL
> > > +        * that denies all access except to any ACE that was explicitly added.
> > > +        *
> > > +        * The setfacl command solves a similar problem with this logic:
> > > +        *
> > > +        * "If  a  Default  ACL  entry is created, and the Default ACL contains
> > > +        *  no owner, owning group, or others entry,  a  copy of  the  ACL
> > > +        *  owner, owning group, or others entry is added to the Default ACL."
> > > +        *
> > > +        * If none of the requisite ACEs were set, and some explicit user or group
> > > +        * ACEs were, copy the requisite entries from the effective set.
> > > +        */
> > > +       if (!default_acl_state.valid &&
> > > +           (default_acl_state.users->n || default_acl_state.groups->n)) {
> > > +               default_acl_state.owner = effective_acl_state.owner;
> > > +               default_acl_state.group = effective_acl_state.group;
> > > +               default_acl_state.other = effective_acl_state.other;
> > > +       }
> > > +
> 
> The other thing I'm wondering about is whether it would make more
> sense to fake up for missing entries individually as setfacl does:
> 
> http://git.savannah.nongnu.org/cgit/acl.git/tree/tools/do_set.c#n368
> 

Oh, I was going by the description in the manpage which seemed to
indicate that if any of those had been set in the effective ACL that we
wouldn't try to fake up anything. I actually had an earlier version that
did what you suggest here. I can change it if you think that's more
correct. It might be good to revise that description in the setfacl
manpage since it's a little unclear.

> > >         *pacl = posix_state_to_acl(&effective_acl_state, flags);
> > >         if (IS_ERR(*pacl)) {
> > >                 ret = PTR_ERR(*pacl);
> > > 
> > > ---
> > > base-commit: 9d985ab8ed33176c3c0380b7de589ea2ae51a48d
> > > change-id: 20230719-nfsd-acl-5ab61537e4e6
> > > 
> > > Best regards,
> > > --
> > > Jeff Layton <jlayton@...nel.org>
> 
> Thanks,
> Andreas

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ