Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

[Andreas Grünbacher]

Am Fr., 25. Nov. 2022 um 10:34 Uhr schrieb Andreas Grünbacher <agruen@gnu.org>:
> Am Do., 24. Nov. 2022 um 18:21 Uhr schrieb Ondrej Valousek
> <ondrej.valousek.xm@renesas.com>:
> > Hi GNU lib maintainers,
> >
> > Attaching the final version of patch introducing a basic checks for 
> > non-trivial NFSv4 style ACLs.
> > It is substantially longer unfortunately - that's why I put most of the 
> > code to acl-internal.c where I think
> > it should be (as similar function for AIX already exists there).
> > The benefit of this is fairly improved robustness of trivial ACLs detection.
> > The disadvantage is that it pulls back dependency on libacl for some reason 
> > (the code does not
> > use libacl at all so I expect some linker flag like -as-needed would to the 
> > trick).
> >
> > I have tested it against Netapp and Linux based NFS servers under various 
> > conditions.
> > Works OK to me. Maybe it's not perfect, but it's able to detect 99% cases
> > where ACLs returned by NFS server are not trivial (i.e. POSIX mode bits 
> > does not fully
> > represent access permissions).
> >
> > Let's hope the code is not forgotten and will be perhaps merged at some 
> > stage.
> > Ondrej
> >
> >
> > diff --git a/lib/acl-internal.c b/lib/acl-internal.c
> > index be244c67a..36e29ac02 100644
> > --- a/lib/acl-internal.c
> > +++ b/lib/acl-internal.c
> > @@ -25,6 +25,9 @@
> >
> >  #if USE_ACL && HAVE_ACL_GET_FILE /* Linux, FreeBSD, Mac OS X, IRIX, Tru64, 
> > Cygwin >= 2.5 */
> >
> > +#include <string.h>
> > +# include <arpa/inet.h>
> > +
> >  # if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */
> >
> >  /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED.
> > @@ -122,6 +125,86 @@ acl_default_nontrivial (acl_t acl)
> >    return (acl_entries (acl) > 0);
> >  }
> >
> > +#define ACE4_WHO_OWNER "OWNER@"
> > +#define ACE4_WHO_GROUP "GROUP@"
> > +#define ACE4_WHO_EVERYONE "EVERYONE@"
> > +
> > +#define ACE4_ACCESS_ALLOWED_ACE_TYPE   0
> > +#define ACE4_ACCESS_DENIED_ACE_TYPE    1
> > +/* ACE flag values */
> > +
> > +#define ACE4_IDENTIFIER_GROUP           0x00000040
> > +
> > +int
> > +acl_nfs4_nontrivial (char *xattr, int len)
> > +{
> > +       int ret = 0,wholen,bufs = len;
> > +        uint32_t flag,type,num_aces = ntohl(*((uint32_t*)(xattr))); /* 
> > Grab the number of aces in the acl */
> > +        char *bufp = xattr;
> > +        int num_a_aces = 0, num_d_aces = 0;
> > +
> > +        ret = 0;
> > +        bufp += 4;  /* sizeof(uint32_t); */
> > +        bufs -= 4;
> > +
> > +
> > +       for(uint32_t ace_n = 0; num_aces > ace_n ; ace_n++) {
> > +               int d_ptr;
> > +
> > +                /* Get the acl type */
> > +                if(bufs <= 0) return -1;
> > +
> > +                type = ntohl(*((uint32_t*)bufp));
> > +
> > +               bufp += 4;
> > +                bufs -= 4;
> > +                if (bufs <= 0) return -1;
> > +
> > +               flag = ntohl(*((uint32_t*)bufp));
> > +               /* As per RFC 7530, the flag should be 0, but we are just 
> > generous to Netapp
> > +                * and also accept the Group flag
> > +                */
>
> We are not "generous" here, RFC 7530 requires to ignore the
> ACE4_IDENTIFIER_GROUP flag for entries with special who values like
> OWNER@, GROUP@, EVERYONE@.
>
> > +               if (flag & ~ACE4_IDENTIFIER_GROUP) return 1;
> > +
> > +               /* we skip mask - it's too risky to test it and it does not 
> > seem to be actually needed */
> > +                bufp += 2*4;
> > +                bufs -= 2*4;
>
> I've already explained to you why checking the mask flags isn't optional:
>
> https://lists.gnu.org/archive/html/bug-gnulib/2022-11/msg00064.html
> https://lists.gnu.org/archive/html/bug-gnulib/2022-11/msg00068.html
>
> > +                if (bufs <= 0) return -1;
> > +
> > +                wholen = ntohl(*((uint32_t*)bufp));
> > +
> > +                bufp += 4;
> > +                bufs -= 4;
> > +
> > +                /* Get the who string */
> > +                if (bufs <= 0) return -1;
> > +
> > +               /* for trivial ACL, we expect max 5 (typically 3) ACES, 3 
> > Allow, 2 deny */
> > +               if ((strncmp(bufp,ACE4_WHO_OWNER,wholen) == 0) || 
> > (strncmp(bufp,ACE4_WHO_GROUP,wholen) == 0)){
> > +                        if(type == ACE4_ACCESS_ALLOWED_ACE_TYPE) 
> > num_a_aces++;
> > +                        if(type == ACE4_ACCESS_DENIED_ACE_TYPE) 
> > num_d_aces++;
> > +                } else if ((strncmp(bufp,ACE4_WHO_EVERYONE,wholen) == 0) 
> > && (type == ACE4_ACCESS_ALLOWED_ACE_TYPE))
> > +                        num_a_aces++;
> > +                else
> > +                        return 1;
>
> Before comparing the who values (above), we must check if the buffer
> contains the entire who string (which currently only happens below).

Also, wholen == strlen() checks are missing here.

>
> > +
> > +                d_ptr = (wholen /4)*4;
> > +                if (wholen % 4 != 0)
> > +                        d_ptr += 4;
>
> Again, any reason for not using something like d_ptr = ROUNDUP(whole,
> 4) with ROUNDUP() being something like the below?
>
> #define ROUNDUP(x, y) (((x) + (y) - 1) & - (y))
>
> > +                bufp += d_ptr;
> > +                bufs -= d_ptr;
> > +
> > +                /* Make sure we aren't outside our domain */
> > +                if (bufs < 0)
> > +                        return -1;
> > +
> > +            }
> > +            return (num_a_aces <= 3) && (num_d_aces <= 2) && ( num_a_aces 
> > + num_d_aces == num_aces) ? 0 : 1;
> > +
> > +}
> > +
> >  # endif
> >
> >  #elif USE_ACL && HAVE_FACL && defined GETACL /* Solaris, Cygwin < 2.5, not 
> > HP-UX */
> > diff --git a/lib/acl-internal.h b/lib/acl-internal.h
> > index 94553fab2..88f1d8f1d 100644
> > --- a/lib/acl-internal.h
> > +++ b/lib/acl-internal.h
> > @@ -146,6 +146,9 @@ rpl_acl_set_fd (int fd, acl_t acl)
> >  #   define acl_entries rpl_acl_entries
> >  extern int acl_entries (acl_t);
> >  #  endif
> > +/* Return 1 if given ACL in XDR format is non-trivial
> > + * Return 0 if it is trivial */
> > +extern int acl_nfs4_nontrivial (char *,int);
> >
> >  #  if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */
> >  /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED.
> > diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
> > index e02f0626a..89eaf70aa 100644
> > --- a/lib/file-has-acl.c
> > +++ b/lib/file-has-acl.c
> > @@ -32,6 +32,11 @@
> >  #if GETXATTR_WITH_POSIX_ACLS
> >  # include <sys/xattr.h>
> >  # include <linux/xattr.h>
> > +# include <arpa/inet.h>
> > +#ifndef XATTR_NAME_NFSV4_ACL
> > +#define XATTR_NAME_NFSV4_ACL "system.nfs4_acl"
> > +#endif
> > +#define TRIVIAL_NFS4_ACL_MAX_LENGTH 128
> >  #endif
> >
> >  /* Return 1 if NAME has a nontrivial access control list,
> > @@ -67,6 +72,16 @@ file_has_acl (char const *name, struct stat const *sb)
> >              return 1;
> >          }
> >
> > +      if (ret < 0) { /* we might be on NFS, so try to check NFSv4 ACLs too 
> > */
> > +         char xattr[TRIVIAL_NFS4_ACL_MAX_LENGTH];
> > +
> > +         errno = 0; /* we need to reset errno set by the previous 
> > getxattr() */
> > +         ret = getxattr (name, XATTR_NAME_NFSV4_ACL, xattr, 
> > TRIVIAL_NFS4_ACL_MAX_LENGTH);
> > +         if (ret < 0 && errno == ENODATA)
> > +              ret = 0;
> > +         else if (ret < 0 && errno == ERANGE) return 1;  /* we won't fit 
> > into the buffer, so non-trivial ACL is presented */
> > +         else if (ret > 0) return acl_nfs4_nontrivial(xattr,ret);     /* 
> > looks like trivial ACL, but we need to investigate further */
> > +      }
> >        if (ret < 0)
> >          return - acl_errno_valid (errno);
> >        return ret;
> >
>
> Andreas

Andreas