Am Do., 24. Nov. 2022 um 18:21 Uhr schrieb Ondrej Valousek
<ondrej.valousek...@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
