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

[Andreas Grünbacher]

Am Mo., 14. Nov. 2022 um 09:18 Uhr schrieb Ondrej Valousek
<ondrej.valousek.xm@renesas.com>:
> Hi Andreas/all
> Would you support the following version of the patch?

We're probably getting there ...

> 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).
> Please let me know.
> Ondrej
>
>
> diff --git a/lib/acl-internal.c b/lib/acl-internal.c
> index be244c67a..13f1dfdb3 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,81 @@ 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
> +
> +int
> +acl_nfs4_nontrivial (char *xattr, int len)
> +{
> +       int ret = 0,wholen,d_ptr,bufs = len;
> +        u_int32_t type,num_aces = (u_int32_t)ntohl(*((u_int32_t*)(xattr))); 
> /* Grab the number of aces in the acl */

The standard type you probably have in mind is called uint32_t, not u_int32_t.

ntohl() already returns a uint32_t; no need for the extra cast.

> +        char *bufp = xattr;
> +        int num_a_aces = 0, num_d_aces = 0;
> +
> +        ret = 0;
> +        d_ptr = sizeof(u_int32_t);

Please just say 4 instead of sizeof(u_int32_t).

> +        bufp += d_ptr;
> +        bufs -= d_ptr;
> +
> +
> +       for(u_int32_t ace_n = 0; num_aces > ace_n ; ace_n++) {
> +                /* Get the acl type */
> +                if(bufs <= 0) {
> +                        errno = EINVAL;
> +                        return -1;
> +                }
> +
> +                type = (u_int32_t)ntohl(*((u_int32_t*)bufp));

If the type is anything other than ALLOW or DENY, we don't have a
trivial ACL. I think an explicit check would be better here than
relying on the rest of the function to magically do the right thing.

> +
> +                d_ptr = 3*sizeof(u_int32_t); /* we skip flag and mask - not 
> relevant for our needs */

We also need to check the flag and mask attributes for values that
cannot occur in a trivial ACL:

* If any flag bits other than ACE4_IDENTIFIER_GROUP are set, we don't
have a trivial ACL.

* If an ALLOW entry has any mask bits set that don't correspond to the
UNIX rwx permissions, we don't have a trivial ACL.

* The same should probably be true for DENY entries, but there could
be implementations out there that simply deny all available
permissions in DENY entries nonetheless, so checking the DENY
permissions may turn out to be harmful here.

> +                bufp += d_ptr;
> +                bufs -= d_ptr;
> +
> +                if (bufs <= 0) {
> +                        errno = EINVAL;
> +                        return -1;
> +                }
> +
> +                wholen = (u_int32_t)ntohl(*((u_int32_t*)bufp));
> +
> +                d_ptr = sizeof(u_int32_t);
> +                bufp += d_ptr;
> +                bufs -= d_ptr;
> +
> +                /* Get the who string */
> +                if (bufs <= 0) {
> +                        errno = EINVAL;
> +                        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;
> +
> +                d_ptr = ((wholen / sizeof(u_int32_t))*sizeof(u_int32_t));
> +                if (wholen % sizeof(u_int32_t) != 0)
> +                        d_ptr += sizeof(u_int32_t);

That's just d_ptr = ROUNDUP(whole, 4) with ROUNDUP() being something like:

#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;

I'm not sure how relevant the counting actually is here; we should get
a pretty reasonable result without it. And if a server throws us an
ACL that is somehow weird but doesn't go beyond what the file mode can
express, we could perfectly well classify that as trivial.

(The ultimate test would be to check if we end up with the same file
mode permission bits, but that's slightly more complicated, doesn't
seem to be strictly necessary here, and we'd end up with
inconsistencies for servers that get it wrong, anyway.)

> +
> +}
> +
>  # 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..4d46e7a9d 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,17 @@ 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 -1;
> +         else 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;

Thanks,
Andreas