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

[Andreas Grünbacher]

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).

> +
> +                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