[PATCH] Basic support for checking NFSv4 ACLs in Linux

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

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
+		 */
+		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;
+
+                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;
+
+                d_ptr = (wholen /4)*4;
+                if (wholen % 4 != 0)
+                        d_ptr += 4;
+
+                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;

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

[Bruno Haible]

[CCing Andreas Grünbacher.]

Hi Ondrej,

Thank you very much for working on this, so consistently for a month.
Like you said: "it might not be perfect, but it's far better than what we
have now in there (i.e. nothing)". Therefore I'd like to have your changes
included, for the benefit of coreutils and of other programs that use the
Gnulib 'copy-file' module.

My question regarding the big picture has been answered by
<https://lists.nongnu.org/archive/html/acl-devel/2022-11/msg00004.html>,
so I'm OK to have this code in Gnulib now.

Your changes are more than 60 lines of code; that is legally relevant for
copyright purposes. GNU Gnulib (like GNU coreutils) requires a copyright
assignment to the FSF. I'll write to you separately about that.

Then, the code would need to be reformatted to use similar indentation
and coding style as the rest of Gnulib:
  - Untabify (spaces instead of tabs),
  - Indent by 2 spaces, with braces of compound statements going to
    separate lines,
  - Use separate lines in
         if (condition)
           statement
  - Use space before opening parenthesis in a function call.
  - Use line breaks to limit line length to 80, if possible.
  See <https://www.gnu.org/prep/standards/html_node/Formatting.html>

And finally, it would be useful if you could add a new file
doc/acl-nfsv4.txt where you describe the environment and commands
which should be used to verify that other contributors (in the future)
don't introduce regressions in your new code. This is as long as we
don't have automated unit tests (like tests/test-file-has-acl.sh,
tests/test-set-mode-acl.sh, tests/test-copy-acl.sh). Similar to what
you did for <https://bugzilla.redhat.com/show_bug.cgi?id=2136452>.

Bruno

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

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

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

[Ondrej Valousek]

Hi Andreas,

- I do not quite understand. I did not bother copying the who value into extra buffer hence I am using strncmp instead of strcmp and therefore can't use strlen. The checks are sufficient there.
- I will put your ROUNDUP code in there.
- Is anyone willing to help me with the linker problem I mentioned earlier? I.e. currently the code pulls dependency on libacl which is not needed and I do not know how to solve it in the config/make script?
Thanks,

Ondrej

-----Original Message-----
From: Andreas Grünbacher <agruen@gnu.org> 
Sent: pátek 25. listopadu 2022 11:17
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Cc: bug-gnulib@gnu.org
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

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://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.gnu.org%2Farchive%2Fhtml%2Fbug-gnulib%2F2022-11%2Fmsg00064.html&amp;
> data=05%7C01%7Condrej.valousek.xm%40renesas.com%7Ceb21a75eff044241a375
> 08dacece459b%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638049682570
> 686986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJ
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=dpFqzx2rTjKH92%2
> FRUTIa8vq5It5800uipgIUDiJSEHA%3D&amp;reserved=0
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.gnu.org%2Farchive%2Fhtml%2Fbug-gnulib%2F2022-11%2Fmsg00068.html&amp;
> data=05%7C01%7Condrej.valousek.xm%40renesas.com%7Ceb21a75eff044241a375
> 08dacece459b%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638049682570
> 686986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJ
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=pm3jn8ByCywefLEp
> xQXkRBbho3VS%2BgrIvsgDPCs9mZQ%3D&amp;reserved=0
>
> > +                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

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

[Andreas Grünbacher]

Am Mo., 28. Nov. 2022 um 08:29 Uhr schrieb Ondrej Valousek
<ondrej.valousek.xm@renesas.com>:
> Hi Andreas,
>
> - I do not quite understand. I did not bother copying the who value into extra buffer hence I am using strncmp instead of strcmp and therefore can't use strlen. The checks are sufficient there.

Please ask yourself if a who string of "@" with wholen == 1 would
match the following test:

  strncmp(bufp, ACE4_WHO_OWNER, wholen) == 0

Thanks,
Andreas

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

Hi Bruno,
Thanks for the input, I think it should be OK now, please take a look.
Thanks,
Ondrej

---
 doc/acl-nfsv4.txt  | 17 +++++++++
 lib/acl-internal.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/acl-internal.h |  3 ++
 lib/file-has-acl.c | 24 ++++++++----
 4 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 doc/acl-nfsv4.txt
diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt
new file mode 100644
index 000000000..71352f58d
--- /dev/null
+++ b/doc/acl-nfsv4.txt
@@ -0,0 +1,17 @@
+General introduction:
+   https://linux.die.net/man/5/nfs4_acl
+
+The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server supporting ACLs
+will support this kind of ACLs (note the difference from POSIX draft ACLs)
+
+The ACLs can be obtained via the nfsv4-acl-tools, i.e.
+
+$ nfs4_getfacl <file>
+
+# file: <file>
+A::OWNER@:rwaDxtTnNcCy
+A::GROUP@:rwaDxtTnNcy
+A::EVERYONE@:rwaDxtTnNcy
+
+Gnulib is aiming to only provide a basic support of these, i.e. recognize trivial
+and non-trivial ACLs
diff --git a/lib/acl-internal.c b/lib/acl-internal.c
index be244c67a..4c65dffcc 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,103 @@ 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
+#  define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
+
+int
+acl_nfs4_nontrivial (char *xattr, int len)
+{
+  int      bufs = len;
+  uint32_t num_aces = ntohl (*((uint32_t*)(xattr))), /* Grab the number of aces in the acl */
+           num_a_aces = 0,
+           num_d_aces = 0;
+  char *bufp = xattr;
+
+  bufp += 4;  /* sizeof(uint32_t); */
+  bufs -= 4;
+
+  for (uint32_t ace_n = 0; num_aces > ace_n ; ace_n++)
+    {
+      int      d_ptr;
+      uint32_t flag,
+               wholen,
+               type;
+
+      /* 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
+       */
+      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;
+
+      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))
+          &&  wholen == 6)
+        {
+          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)
+            && (wholen == 9))
+          num_a_aces++;
+        else
+          return 1;
+
+      d_ptr = ROUNDUP (wholen, 4);
+      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));
+
+}
+
 # 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..5da7c115c 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..171023488 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,22 @@ 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)
+                /* looks like trivial ACL, but we need to investigate further */
+                return acl_nfs4_nontrivial (xattr, ret);
+        }
       if (ret < 0)
         return - acl_errno_valid (errno);
       return ret;

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

[Bruno Haible]

Hi Ondrej,

> Thanks for the input, I think it should be OK now, please take a look.

Yes, the patch is now in a form that we could commit into Gnulib. Thank you!
For the semantics, I trust your testing and the discussions you had with
Andreas Grünbacher.

The other point is the copyright assignment to the FSF. I started a
private mail exchange with you on this topic a week ago. Please reply
if something is not clear.

Bruno

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

[Bruno Haible]

Hi Ondrej,

> Thanks for the input, I think it should be OK now, please take a look.

The paperwork being complete now, I have pushed the change for you,
with this ChangeLog entry. Thanks!!


2022-12-22  Ondrej Valousek  <ondrej.valousek.xm@renesas.com>

	file-has-acl: Basic support for checking NFSv4 ACLs in Linux.
	* lib/acl-internal.h (acl_nfs4_nontrivial): New declaration.
	* lib/acl-internal.c (acl_nfs4_nontrivial): New function.
	* lib/file-has-acl.c: Include <arpa/inet.h>.
	(XATTR_NAME_NFSV4_ACL, TRIVIAL_NFS4_ACL_MAX_LENGTH): New macros.
	(file_has_acl): Test for NFSv4 ACLs.
	* doc/acl-nfsv4.txt: New file.

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

I had some trouble using that patch in GNU Emacs, as emacsclient 
wouldn't link. I solved this by moving the new function from 
acl-internal.c to file-has-acl.c and making it static (no reason to 
pubilsh it that I know of). While doing this I did a more careful review 
of the code and came up with the attached patch. Some of this is just to 
use the usual GNU style; hope you don't mind.

Ondrej, please give this a try in your environment, as my NFSv4 
environment at work doesn't allow ACLs so I can't easily test it. Thanks.

[-- Attachment #2: 0001-file-has-acl-improve-recent-NFSv4-support.patch --]
[-- Type: text/x-patch, Size: 13460 bytes --]

From 35bd46f0c816948dc1a0430c8ba8b10a01167320 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 23 Dec 2022 15:18:29 -0800
Subject: [PATCH] file-has-acl: improve recent NFSv4 support
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This fixes a link failure with emacsclient on GNU/Linux.  This
program wants file_has_acl but none of the other ACL primitives,
so it doesn’t link acl-internal.o; this way it doesn’t need to
link with -lacl.  While I was at it I reviewed the recent changes,
fixed some unlikely overflow bugs, and adjusted to GNU style.
* doc/acl-nfsv4.txt: Remove.  Its contents are now in a
comment in lib/file-has-acl.c.
* lib/acl-internal.c, lib/acl-internal.h: Move recent changes
relating to acl_nfs4_nontrivial to lib/file-has-acl.c, so that
there is no trouble linking programs that need only file_has_acl.
* lib/file-has-acl.c (acl_nfs4_nontrivial): Move here from
lib/acl-internal.c, so that we needn't link -lacl in
programs that want only file_has_acl, such as emacsclient.
Do not assume a char buffer is aligned for uint32_t.
Check more carefully for buffer read overrun.
Allow up to 6 ACEs, since other code does; but check
that they’re distinct.  Avoid integer overflow.
Use memcmp rather than strncmp to compare memory blocks.
(file_has_acl): Preserve initial errno instead of setting to 0.
Allocate a bit more room for trivial ACL buffer.
Use EINVAL for botchedk NFSv4 ACLs (which shouldn’t happen).
---
 ChangeLog          |  25 +++++++++
 doc/acl-nfsv4.txt  |  17 ------
 lib/acl-internal.c | 100 -----------------------------------
 lib/acl-internal.h |   3 --
 lib/file-has-acl.c | 129 +++++++++++++++++++++++++++++++++++++++------
 5 files changed, 138 insertions(+), 136 deletions(-)
 delete mode 100644 doc/acl-nfsv4.txt

diff --git a/ChangeLog b/ChangeLog
index 562845795d..3b6c6cf98e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,28 @@
+2022-12-23  Paul Eggert  <eggert@cs.ucla.edu>
+
+	file-has-acl: improve recent NFSv4 support
+	This fixes a link failure with emacsclient on GNU/Linux.  This
+	program wants file_has_acl but none of the other ACL primitives,
+	so it doesn’t link acl-internal.o; this way it doesn’t need to
+	link with -lacl.  While I was at it I reviewed the recent changes,
+	fixed some unlikely overflow bugs, and adjusted to GNU style.
+	* doc/acl-nfsv4.txt: Remove.  Its contents are now in a
+	comment in lib/file-has-acl.c.
+	* lib/acl-internal.c, lib/acl-internal.h: Move recent changes
+	relating to acl_nfs4_nontrivial to lib/file-has-acl.c, so that
+	there is no trouble linking programs that need only file_has_acl.
+	* lib/file-has-acl.c (acl_nfs4_nontrivial): Move here from
+	lib/acl-internal.c, so that we needn't link -lacl in
+	programs that want only file_has_acl, such as emacsclient.
+	Do not assume a char buffer is aligned for uint32_t.
+	Check more carefully for buffer read overrun.
+	Allow up to 6 ACEs, since other code does; but check
+	that they’re distinct.  Avoid integer overflow.
+	Use memcmp rather than strncmp to compare memory blocks.
+	(file_has_acl): Preserve initial errno instead of setting to 0.
+	Allocate a bit more room for trivial ACL buffer.
+	Use EINVAL for botchedk NFSv4 ACLs (which shouldn’t happen).
+
 2022-12-22  Paul Eggert  <eggert@cs.ucla.edu>
 
 	posix_spawnp-tests: fix filename typo
diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt
deleted file mode 100644
index 71352f58d0..0000000000
--- a/doc/acl-nfsv4.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-General introduction:
-   https://linux.die.net/man/5/nfs4_acl
-
-The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server supporting ACLs
-will support this kind of ACLs (note the difference from POSIX draft ACLs)
-
-The ACLs can be obtained via the nfsv4-acl-tools, i.e.
-
-$ nfs4_getfacl <file>
-
-# file: <file>
-A::OWNER@:rwaDxtTnNcCy
-A::GROUP@:rwaDxtTnNcy
-A::EVERYONE@:rwaDxtTnNcy
-
-Gnulib is aiming to only provide a basic support of these, i.e. recognize trivial
-and non-trivial ACLs
diff --git a/lib/acl-internal.c b/lib/acl-internal.c
index 4c65dffcc6..be244c67a2 100644
--- a/lib/acl-internal.c
+++ b/lib/acl-internal.c
@@ -25,9 +25,6 @@
 
 #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.
@@ -125,103 +122,6 @@ 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
-#  define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
-
-int
-acl_nfs4_nontrivial (char *xattr, int len)
-{
-  int      bufs = len;
-  uint32_t num_aces = ntohl (*((uint32_t*)(xattr))), /* Grab the number of aces in the acl */
-           num_a_aces = 0,
-           num_d_aces = 0;
-  char *bufp = xattr;
-
-  bufp += 4;  /* sizeof(uint32_t); */
-  bufs -= 4;
-
-  for (uint32_t ace_n = 0; num_aces > ace_n ; ace_n++)
-    {
-      int      d_ptr;
-      uint32_t flag,
-               wholen,
-               type;
-
-      /* 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
-       */
-      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;
-
-      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))
-          &&  wholen == 6)
-        {
-          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)
-            && (wholen == 9))
-          num_a_aces++;
-        else
-          return 1;
-
-      d_ptr = ROUNDUP (wholen, 4);
-      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));
-
-}
-
 # 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 5da7c115c6..94553fab25 100644
--- a/lib/acl-internal.h
+++ b/lib/acl-internal.h
@@ -146,9 +146,6 @@ 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 171023488c..676523ba82 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -29,14 +29,97 @@
 
 #include "acl-internal.h"
 
-#if GETXATTR_WITH_POSIX_ACLS
+#if USE_ACL && GETXATTR_WITH_POSIX_ACLS
+# include <string.h>
+# include <arpa/inet.h>
 # 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
+
+enum {
+  /* ACE4_ACCESS_ALLOWED_ACE_TYPE = 0x00000000, */
+  ACE4_ACCESS_DENIED_ACE_TYPE  = 0x00000001,
+  ACE4_IDENTIFIER_GROUP        = 0x00000040
+};
+
+/* Return 1 if given ACL in XDR format is non-trivial, 0 if it is trivial.
+   -1 upon failure to determine it.  Possibly change errno.  Assume that
+   the ACL is valid, except avoid undefined behavior even if invalid.
+
+   See <https://linux.die.net/man/5/nfs4_acl>.  The NFSv4 acls are
+   defined in Internet RFC 7530 and as such, every NFSv4 server
+   supporting ACLs should support NFSv4 ACLs (they differ from from
+   POSIX draft ACLs).  The ACLs can be obtained via the
+   nfsv4-acl-tools, e.g., the nfs4_getfacl command.  Gnulib provides
+   only basic support of NFSv4 ACLs, i.e., recognize trivial vs
+   nontrivial ACLs.  */
+
+static int
+acl_nfs4_nontrivial (uint32_t *xattr, ssize_t nbytes)
+{
+  enum { BYTES_PER_NETWORK_UINT = 4};
+
+  /* Grab the number of aces in the acl.  */
+  nbytes -= BYTES_PER_NETWORK_UINT;
+  if (nbytes < 0)
+    return -1;
+  uint32_t num_aces = ntohl (*xattr++);
+  if (6 < num_aces)
+    return 1;
+  int ace_found = 0;
+
+  for (int ace_n = 0; ace_n < num_aces; ace_n++)
+    {
+      /* Get the acl type and flag.  Skip the mask; it's too risky to
+         test it and it does not seem to be needed.  Get the wholen.  */
+      nbytes -= 4 * BYTES_PER_NETWORK_UINT;
+      if (nbytes < 0)
+        return -1;
+      uint32_t type = ntohl (xattr[0]);
+      uint32_t flag = ntohl (xattr[1]);
+      uint32_t wholen = ntohl (xattr[3]);
+      xattr += 4;
+      int64_t wholen4 = wholen;
+      wholen4 = ((wholen4 + (BYTES_PER_NETWORK_UINT))
+                 & ~ (BYTES_PER_NETWORK_UINT - 1));
+
+      /* Trivial ACLs have only ACE4_ACCESS_ALLOWED_ACE_TYPE or
+         ACE4_ACCESS_DENIED_ACE_TYPE.  */
+      if (ACE4_ACCESS_DENIED_ACE_TYPE < type)
+        return 1;
+
+      /* RFC 7530 says FLAG should be 0, but be generous to NetApp and
+         also accept the group flag.  */
+      if (flag & ~ACE4_IDENTIFIER_GROUP)
+        return 1;
+
+      /* Get the who string.  Check NBYTES - WHOLEN4 before storing
+         into NBYTES, to avoid truncation on conversion.  */
+      if (nbytes - wholen4 < 0)
+        return -1;
+      nbytes -= wholen4;
+
+      /* For a trivial ACL, max 6 (typically 3) ACEs, 3 allow, 3 deny.
+         Check that there is at most one ACE of each TYPE and WHO.  */
+      int who2
+        = (wholen == 6 && memcmp (xattr, "OWNER@", 6) == 0 ? 0
+           : wholen == 6 && memcmp (xattr, "GROUP@", 6) == 0 ? 2
+           : wholen == 9 && memcmp (xattr, "EVERYONE@", 9) == 0 ? 4
+           : -1);
+      if (who2 < 0)
+        return 1;
+      int ace_found_bit = 1 << (who2 | type);
+      if (ace_found & ace_found_bit)
+        return 1;
+      ace_found |= ace_found_bit;
+
+      xattr = (uint32_t *) ((char *) xattr + wholen4);
+    }
+
+  return 0;
+}
 #endif
 
 /* Return 1 if NAME has a nontrivial access control list,
@@ -56,6 +139,7 @@ file_has_acl (char const *name, struct stat const *sb)
 # if GETXATTR_WITH_POSIX_ACLS
 
       ssize_t ret;
+      int initial_errno = errno;
 
       ret = getxattr (name, XATTR_NAME_POSIX_ACL_ACCESS, NULL, 0);
       if (ret < 0 && errno == ENODATA)
@@ -73,20 +157,33 @@ file_has_acl (char const *name, struct stat const *sb)
         }
 
       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;
+        {
+          /* Check for NFSv4 ACLs.  The max length of a trivial
+             ACL is 6 words for owner, 6 for group, 7 for everyone,
+             all times 2 because there are both allow and deny ACEs.
+             There are 6 words for owner because of type, flag, mask,
+             wholen, "OWNER@"+pad and similarly for group; everyone is
+             another word to hold "EVERYONE@".  */
+          uint32_t xattr[2 * (6 + 6 + 7)];
+
+          ret = getxattr (name, XATTR_NAME_NFSV4_ACL, xattr, sizeof xattr);
+          if (ret < 0)
+            switch (errno)
+              {
+              case ENODATA: return 0;
+              case ERANGE : return 1; /* ACL must be nontrivial.  */
+              }
           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)
-                /* looks like trivial ACL, but we need to investigate further */
-                return acl_nfs4_nontrivial (xattr, ret);
+            {
+              /* It looks like a trivial ACL, but investigate further.  */
+              ret = acl_nfs4_nontrivial (xattr, ret);
+              if (ret < 0)
+                {
+                  errno = EINVAL;
+                  return ret;
+                }
+              errno = initial_errno;
+            }
         }
       if (ret < 0)
         return - acl_errno_valid (errno);
-- 
2.25.1

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

[Ondrej Valousek]

Hi Paul,

Thanks for this, few comments:
- I do not know which code referenced deny ACE for "EVERYONE@". In reality, there should be none as there is no need. But I agree it is probably harmless.

- I would still reference the who strings indirectly, either via macros (i.e. ACE4_WHO_XXXX) or enum using the names in my original code - because that's the way they are defined in Linux kernel, so it's clear what we are referring to the same thing

- I still think the best would be to keep the code in lib/acl-internal.c as other (similar) code exists there already. I admit however that I have no clue how to resolve the linker problem, tried various options, did not work.

- I also think it would be good we expose the new function in the header file. It might be not be necessary now, but I can well imagine we might want to reuse it somewhere else.

Otherwise, I have tested it on various ACL enabled NFSv4 servers (Linux/ Solaris/ Netapp) and it seems to work OK.

Thanks,
Ondrej

-----Original Message-----
From: Paul Eggert <eggert@cs.ucla.edu> 
Sent: sobota 24. prosince 2022 0:32
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Cc: Gnulib bugs <bug-gnulib@gnu.org>
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

I had some trouble using that patch in GNU Emacs, as emacsclient wouldn't link. I solved this by moving the new function from acl-internal.c to file-has-acl.c and making it static (no reason to pubilsh it that I know of). While doing this I did a more careful review of the code and came up with the attached patch. Some of this is just to use the usual GNU style; hope you don't mind.

Ondrej, please give this a try in your environment, as my NFSv4 environment at work doesn't allow ACLs so I can't easily test it. Thanks.

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

[Paul Eggert]

On 12/24/22 05:00, Ondrej Valousek wrote:

> I do not know which code referenced deny ACE for "EVERYONE@".
This was already in file-has-acl.c under the ACE_GETACL code, used in 
Solaris 10 and 11 for ZFS and NFSv4. Its commentary talks about "6 entries".

> - I would still reference the who strings indirectly, either via macros (i.e. ACE4_WHO_XXXX) or enum using the names in my original code - because that's the way they are defined in Linux kernel, so it's clear what we are referring to the same thing

Where in the Linux kernel is this done? The only instance of "EVERYONE@" 
that I see is in linux/fs/nfsd/nfs4acl.c, and there's no specific name 
given to the string "EVERYONE@". And I see no name for the string in RFC 
7530. I'm not opposed to giving the string a name but would like to know 
where the name comes from so that we can cite it. In contrast, 
ACE4_ACCESS_DENIED_ACE_TYPE does appear in RFC 7530 so names like that 
are citable

.

> - I still think the best would be to keep the code in lib/acl-internal.c as other (similar) code exists there already. I admit however that I have no clue how to resolve the linker problem, tried various options, did not work.

Yes, the main point for putting the trivial-ACL detection into 
file-has-acl.c rather than acl-internal.c is so that programs like GNU 
ls don't have to link with -lacl. These programs need to know only 
whether the ACL is trivial.

For now Gnulib and its users don't need acl_nfs4_nontrivial to be extern 
so let's keep it static. We can always change it later if need be. (Part 
of the appeal of Gnulib is that it's a source code library so these 
sorts of changes are not a big deal.) It's a specialized function so 
there's a chance it'll never need to be public as-is.

Thanks for reviewing the code changes.

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

Some static checking helped find an off-by-one bug that I introduced to 
your Gnulib patch. The bug caused file_has_acl to sometimes incorrectly 
return -1 when given a nontrivial ACL in which a WHOLEN is a multiple of 
4. Sorry about that. I installed the attached further patch to fix it.

[-- Attachment #2: 0001-file-has-acl-fix-recently-introduced-NFSv4-bug.patch --]
[-- Type: text/x-patch, Size: 2042 bytes --]

From d65e5a8ba77595a598c9ddb8dfa09c4aea732659 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 27 Dec 2022 20:00:58 -0800
Subject: [PATCH] file-has-acl: fix recently-introduced NFSv4 bug

* lib/file-has-acl.c (acl_nfs4_nontrivial): Fix off-by-one
error when rounding WHOLEN up to next multiple of 4.
Pacify GCC 12.2.1 -Wcast-align.
---
 ChangeLog          | 5 +++++
 lib/file-has-acl.c | 9 +++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 12c14a2e7a..be0fb22078 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2022-12-27  Paul Eggert  <eggert@cs.ucla.edu>
 
+	file-has-acl: fix recently-introduced NFSv4 bug
+	* lib/file-has-acl.c (acl_nfs4_nontrivial): Fix off-by-one
+	error when rounding WHOLEN up to next multiple of 4.
+	Pacify GCC 12.2.1 -Wcast-align.
+
 	Add --pull, --gen options to build-aux/bootstrap
 	This supports a single bootstrap script with --pull and --gen
 	options, as an alternative to separate autogen.sh and autopull.sh
diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index 676523ba82..7876edc4f0 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -81,9 +81,10 @@ acl_nfs4_nontrivial (uint32_t *xattr, ssize_t nbytes)
       uint32_t flag = ntohl (xattr[1]);
       uint32_t wholen = ntohl (xattr[3]);
       xattr += 4;
-      int64_t wholen4 = wholen;
-      wholen4 = ((wholen4 + (BYTES_PER_NETWORK_UINT))
-                 & ~ (BYTES_PER_NETWORK_UINT - 1));
+      int whowords = (wholen / BYTES_PER_NETWORK_UINT
+                      + (wholen % BYTES_PER_NETWORK_UINT != 0));
+      int64_t wholen4 = whowords;
+      wholen4 *= BYTES_PER_NETWORK_UINT;
 
       /* Trivial ACLs have only ACE4_ACCESS_ALLOWED_ACE_TYPE or
          ACE4_ACCESS_DENIED_ACE_TYPE.  */
@@ -115,7 +116,7 @@ acl_nfs4_nontrivial (uint32_t *xattr, ssize_t nbytes)
         return 1;
       ace_found |= ace_found_bit;
 
-      xattr = (uint32_t *) ((char *) xattr + wholen4);
+      xattr += whowords;
     }
 
   return 0;
-- 
2.25.1

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

[Ondrej Valousek]

Thanks Paul,
I was actually wondering why did not you reuse the suggestion from Andreas:
#  define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
As it seemed to work pretty well to me (+ it makes obvious what we do here).
Anyhow, my 2 cents 😊

-----Original Message-----
From: Paul Eggert <eggert@cs.ucla.edu> 
Sent: středa 28. prosince 2022 5:13
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Cc: Gnulib bugs <bug-gnulib@gnu.org>
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

Some static checking helped find an off-by-one bug that I introduced to your Gnulib patch. The bug caused file_has_acl to sometimes incorrectly return -1 when given a nontrivial ACL in which a WHOLEN is a multiple of 4. Sorry about that. I installed the attached further patch to fix it.

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

[Paul Eggert]

On 12/28/22 01:07, Ondrej Valousek wrote:
> I was actually wondering why did not you reuse the suggestion from Andreas:
> #  define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))

It didn't work when the + overflowed, and it assumed two's complement. 
The latter assumption is pretty safe nowadays (and will be required by 
C23, finally!) but I have a thing about portability. Although these 
issues are fixable the particular situation here (where values close to 
2**32-1 really do represent 2**32 bytes, and where gcc incorrectly 
complains about adding aligned byte counts to aligned pointers) 
suggested special code.

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

Hi Bruno,
Could you review one more time?
Thanks,
Ondrej

---
 doc/acl-nfsv4.txt  | 17 +++++++++
 lib/acl-internal.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/acl-internal.h |  3 ++
 lib/file-has-acl.c | 24 ++++++++----
 4 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 doc/acl-nfsv4.txt

diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt
new file mode 100644
index 000000000..71352f58d
--- /dev/null
+++ b/doc/acl-nfsv4.txt
@@ -0,0 +1,17 @@
+General introduction:
+   https://linux.die.net/man/5/nfs4_acl
+
+The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server supporting ACLs
+will support this kind of ACLs (note the difference from POSIX draft ACLs)
+
+The ACLs can be obtained via the nfsv4-acl-tools, i.e.
+
+$ nfs4_getfacl <file>
+
+# file: <file>
+A::OWNER@:rwaDxtTnNcCy
+A::GROUP@:rwaDxtTnNcy
+A::EVERYONE@:rwaDxtTnNcy
+
+Gnulib is aiming to only provide a basic support of these, i.e. recognize trivial
+and non-trivial ACLs
diff --git a/lib/acl-internal.c b/lib/acl-internal.c
index be244c67a..b4172fe6d 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,104 @@ 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
+# define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
+
+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
+       */
+      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;
+
+      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)) &&
+           wholen == 6)
+        {
+          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)
+              && (wholen == 9))
+            num_a_aces++;
+          else
+            return 1;
+
+      d_ptr = ROUNDUP (wholen,4);
+      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..b674c7702 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..82830ba4a 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,22 @@ 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)
+                /* looks like trivial ACL, but we need to investigate further */
+                return acl_nfs4_nontrivial (xattr, ret);
+        }
       if (ret < 0)
         return - acl_errno_valid (errno);
       return ret;

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

[Bruno Haible]

Hi Ondrej,

Still some formatting issues. But now, with formatting nearly right, I'm able
to spot other things too.

> +# 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
> +# define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))

These are all inside a '# if' / '# endif' pair, so the preprocessor statement
indentation level is 2. Please add one more space between '#' and 'define' here.

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

'ret' is already initialized to 0 above. One of the two initializations is
redundant.

Hmm... is the variable 'ret' used at all? You should routinely compile
with "gcc -Wall"; this catches unused variables, usually.

> +  bufp += 4;  /* sizeof(uint32_t); */
> +  bufs -= 4;
> +
> +  for (uint32_t ace_n = 0; num_aces > ace_n ; ace_n++)
> +    {
> +      int d_ptr;

There are more variables which can be moved into the 'for' loop. Generally,
moving variables into the smallest scope that they need provides for a better
readability of the code. If I'm not mistaken, here, it applies to the variables
wholen, flag, type.

> +
> +      /* 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
> +       */
> +      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;
> +
> +      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)) &&
> +           wholen == 6)

Please move the || and && operators to the next line:

         if (((strncmp (bufp,ACE4_WHO_OWNER,wholen) == 0)
              || (strncmp (bufp,ACE4_WHO_GROUP,wholen) == 0))
             && wholen == 6)

Add add a space after each comma in an argument list.

> +        {
> +          if (type == ACE4_ACCESS_ALLOWED_ACE_TYPE)
> +            num_a_aces++;
> +          if (type == ACE4_ACCESS_DENIED_ACE_TYPE)
> +            num_d_aces++;
> +        } else

Please put the closing brace and the 'else' on separate lines.

> +          if ((strncmp (bufp,ACE4_WHO_EVERYONE,wholen) == 0)
> +              && (type == ACE4_ACCESS_ALLOWED_ACE_TYPE)
> +              && (wholen == 9))
> +            num_a_aces++;
> +          else
> +            return 1;
> +
> +      d_ptr = ROUNDUP (wholen,4);
> +      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;

You can simplify this: Knowing that boolean expressions have the value 1
for true and 0 for false in C, you can write it as

     return !((num_a_aces <= 3) && (num_d_aces <= 2)
              && (num_a_aces + num_d_aces == num_aces));

which is smaller and simpler.

> diff --git a/lib/acl-internal.h b/lib/acl-internal.h
> index 94553fab2..b674c7702 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);

Please add a space after the comma.

> diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
> index e02f0626a..82830ba4a 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

Please indent the last three of these lines by one more space, after the '#'.

Bruno

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

Hi Bruno,
Just sending the modified patch (fixed formatting + few suggestions from Andreas included)
Does it look OK now?
Thanks,
Ondrej

---
 doc/acl-nfsv4.txt  | 17 +++++++++
 lib/acl-internal.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/acl-internal.h |  3 ++
 lib/file-has-acl.c | 24 ++++++++----
 m4/acl.m4          |  5 ++-
 5 files changed, 135 insertions(+), 9 deletions(-)
 create mode 100644 doc/acl-nfsv4.txt

diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt
new file mode 100644
index 000000000..552a62ca3
--- /dev/null
+++ b/doc/acl-nfsv4.txt
@@ -0,0 +1,17 @@
+General introduction:
+   https://linux.die.net/man/5/nfs4_acl
+
+The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server supporting ACLs
+will support this kind of ACLs (note the difference from POSIX draft ACLs)
+
+The ACLs can be obtained via the nfsv4-acl-tools, i.e.
+
+$ nfs4_getfacl <file>
+
+# file: <file>
+A::OWNER@:rwaDxtTnNcCy
+A::GROUP@:rwaDxtTnNcy
+A::EVERYONE@:rwaDxtTnNcy
+
+GNULib is aiming to only provide a basic support of these, i.e. recognize trivial
+and non-trivial ACLs
diff --git a/lib/acl-internal.c b/lib/acl-internal.c
index be244c67a..86df38854 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,98 @@ 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
+#define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
+
+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
+     */
+    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;
+
+    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)) &&
+         wholen == 6 )
+      {
+        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) &&
+            (wholen == 9))
+          num_a_aces++;
+        else
+          return 1;
+
+    d_ptr = ROUNDUP(wholen,4);
+    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..8e43dd70f 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,22 @@ 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;
diff --git a/m4/acl.m4 b/m4/acl.m4
index 8909442d7..9445edf9c 100644
--- a/m4/acl.m4
+++ b/m4/acl.m4
@@ -197,7 +197,10 @@ AC_DEFUN([gl_FILE_HAS_ACL],
          [gl_cv_getxattr_with_posix_acls=yes])])
   fi
   if test "$gl_cv_getxattr_with_posix_acls" = yes; then
-    LIB_HAS_ACL=
+    dnl We need to pull libacl back to make linker happy, but strictly
+    dnl speaking, we do not need it
+    LIB_HAS_ACL=$LIB_ACL
+    gl_need_lib_has_acl=1
     AC_DEFINE([GETXATTR_WITH_POSIX_ACLS], 1,
       [Define to 1 if getxattr works with XATTR_NAME_POSIX_ACL_ACCESS
        and XATTR_NAME_POSIX_ACL_DEFAULT.])

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

[Bruno Haible]

Hi Ondrej,

Ondrej Valousek wrote:
> Just sending the modified patch (fixed formatting + few suggestions from Andreas included)
> Does it look OK now?

Some formatting issues, still. Details below.

> diff --git a/doc/acl-nfsv4.txt b/doc/acl-nfsv4.txt
> new file mode 100644
> index 000000000..552a62ca3
> --- /dev/null
> +++ b/doc/acl-nfsv4.txt

Thanks for adding this file. It will help maintenance.

> @@ -0,0 +1,17 @@
> +General introduction:
> +   https://linux.die.net/man/5/nfs4_acl
> +
> +The NFSv4 acls are defined in RFC7530 and as such, every NFSv4 server supporting ACLs
> +will support this kind of ACLs (note the difference from POSIX draft ACLs)
> +
> +The ACLs can be obtained via the nfsv4-acl-tools, i.e.
> +
> +$ nfs4_getfacl <file>
> +
> +# file: <file>
> +A::OWNER@:rwaDxtTnNcCy
> +A::GROUP@:rwaDxtTnNcy
> +A::EVERYONE@:rwaDxtTnNcy
> +
> +GNULib is aiming to only provide a basic support of these, i.e. recognize trivial

Please spell it 'Gnulib', not 'GNULib'.

> +and non-trivial ACLs
> diff --git a/lib/acl-internal.c b/lib/acl-internal.c
> index be244c67a..86df38854 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>

We try to keep a visible indentation level for preprocessor directives/
statements/instructions. The indentation level is indicated by spaces
after the '#'. So, here please insert a space before 'include <string.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,98 @@ 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

In these preprocessor statements, we are at preprocessor indentation
level 2. So please change '#define' to '#  define' here.

> +/* ACE flag values */

What does this comment belong to? The two lines above or the line below?
If it's the lines above, then the comment should be moved up. If it's
the line below, a blank line should be removed.

> +
> +#define ACE4_IDENTIFIER_GROUP          0x00000040
> +#define ROUNDUP(x, y)                  (((x) + (y) - 1) & - (y))
> +
> +int
> +acl_nfs4_nontrivial (char *xattr, int len)
> +{
> +  int ret = 0,wholen,bufs = len;

Please define one variable per line. When reading the code, we want to see
quickly where a certain variable is declared, and that is significantly
harder if multiple variables are declared on the same line, especially
with initializations.

> +  uint32_t flag,type,num_aces = ntohl(*((uint32_t*)(xattr))); /* Grab the number of aces in the acl */

Likewise. Also, please add a space after 'ntohl' because it's a function call.

> +  char *bufp = xattr;
> +  int num_a_aces = 0, num_d_aces = 0;

Please define one variable per line.

> +  ret = 0;
> +  bufp += 4;  /* sizeof(uint32_t); */
> +  bufs -= 4;
> +
> +  for(uint32_t ace_n = 0; num_aces > ace_n ; ace_n++) {

Please add a space after 'for'. Also put the opening brace on a separate line.

> +    int d_ptr;
> +
> +    /* Get the acl type */
> +    if(bufs <= 0) 

Please add a space after 'if'.

> +      return -1;
> +
> +    type = ntohl(*((uint32_t*)bufp));

Please add a space after 'ntohl' because it's a function call.

> +    bufp += 4;
> +    bufs -= 4;
> +    if (bufs <= 0)
> +      return -1;
> +
> +    flag = ntohl(*((uint32_t*)bufp));

Likewise.

> +    /* As per RFC 7530, the flag should be 0, but we are just generous to Netapp
> +     * and also accept the Group flag
> +     */
> +    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;
> +
> +    if (bufs <= 0) 
> +      return -1;
> +
> +    wholen = ntohl(*((uint32_t*)bufp));

Likewise.

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

Please move the operator || to the next line. This allows reading the multi-line
expressions faster, just by looking at the first few characters of each line.

> +        (strncmp(bufp,ACE4_WHO_GROUP,wholen) == 0)) &&

Likewise, please move the operator && to the next line.

> +         wholen == 6 )

You can remove the space before the closing parenthesis.

> +      {
> +        if(type == ACE4_ACCESS_ALLOWED_ACE_TYPE)

Please add a space after 'if'.

> +          num_a_aces++;
> +        if(type == ACE4_ACCESS_DENIED_ACE_TYPE)

Likewise.

> +          num_d_aces++;
> +      } else 

Please put the closing brace and the 'else' on separate lines.

> +        if ((strncmp(bufp,ACE4_WHO_EVERYONE,wholen) == 0) &&

Please move the operator && to the next line.

> +            (type == ACE4_ACCESS_ALLOWED_ACE_TYPE) &&

Likewise.

> +            (wholen == 9))
> +          num_a_aces++;
> +        else
> +          return 1;
> +
> +    d_ptr = ROUNDUP(wholen,4);

Please add a space after ROUNDUP, since it's an invocation of a function-like
macro.

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

Please move the operator && to the next line.

> +         ( num_a_aces + num_d_aces == num_aces) ? 0 : 1;

You can remove the space after the opening parenthesis.

> +
> +}
> +
>  # 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..8e43dd70f 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

Please add spaces after '#', to indicate the preprocessor statement
indentation level.

>  #endif
>  
>  /* Return 1 if NAME has a nontrivial access control list,
> @@ -67,6 +72,22 @@ 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

This 'else' statement and the following lines should be indented by 2 more
spaces.

> +            if (ret > 0)
> +              return acl_nfs4_nontrivial(xattr,ret);

Please add spaces before the opening parenthesis and after the comma.

> +              /* looks like trivial ACL, but we need to investigate further */

It looks like this comment refers to the previous line? Then move it up one line.

> +        }
>        if (ret < 0)
>          return - acl_errno_valid (errno);
>        return ret;
> diff --git a/m4/acl.m4 b/m4/acl.m4
> index 8909442d7..9445edf9c 100644
> --- a/m4/acl.m4
> +++ b/m4/acl.m4
> @@ -197,7 +197,10 @@ AC_DEFUN([gl_FILE_HAS_ACL],
>           [gl_cv_getxattr_with_posix_acls=yes])])
>    fi
>    if test "$gl_cv_getxattr_with_posix_acls" = yes; then
> -    LIB_HAS_ACL=
> +    dnl We need to pull libacl back to make linker happy, but strictly
> +    dnl speaking, we do not need it
> +    LIB_HAS_ACL=$LIB_ACL
> +    gl_need_lib_has_acl=1
>      AC_DEFINE([GETXATTR_WITH_POSIX_ACLS], 1,
>        [Define to 1 if getxattr works with XATTR_NAME_POSIX_ACL_ACCESS
>         and XATTR_NAME_POSIX_ACL_DEFAULT.])
> 

I see nothing in the changes above that needs to pull in libacl. Therefore
please leave the change to m4/acl.m4 out for the moment. We'll need to discuss
this part in a separate thread; as first part of the thread please describe
the problem (link error?) that you are having — since Andreas said that he
doesn't get a link error, and I haven't seen a link error regarding libacl
in years either.

Thanks!

Bruno

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

Hi Andreas/all
Would you support the following version of the patch?
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 */
+        char *bufp = xattr;
+        int num_a_aces = 0, num_d_aces = 0;
+
+        ret = 0;
+        d_ptr = 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));
+
+                d_ptr = 3*sizeof(u_int32_t); /* we skip flag and mask - not relevant for our needs */
+                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);
+
+                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..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;

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

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

[Ondrej Valousek]

> * 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.
Do we really have to do this?
I mean from RFC8881:
" 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."

... I take it these 3 ACEs should always represent mode bits.

Or if you really wish I can shamelessly steal the AIX code there and put smth like this:
If (aceMask & ~(ACE4_READ_DATA | ACE4_LIST_DIRECTORY
                                 | ACE4_WRITE_DATA | ACE4_ADD_FILE
                                 | ACE4_EXECUTE)) == 0)) return 1;

Thanks,
Ondrej

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

[Andreas Grünbacher]

Am Di., 15. Nov. 2022 um 10:17 Uhr schrieb Ondrej Valousek
<ondrej.valousek.xm@renesas.com>:
> > * 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.
> Do we really have to do this?

Of course. For example, an ACL that grants ACE4_WRITE_ACL to anyone
beyond the OWNER@ cannot be represented by the file mode permission
bits.

Andreas

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

[Andreas Grünbacher]

Am Di., 15. Nov. 2022 um 10:17 Uhr schrieb Ondrej Valousek
<ondrej.valousek.xm@renesas.com>:
> I mean from RFC8881:
> " 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."
>
> ... I take it these 3 ACEs should always represent mode bits.

The NFSv4 specification is /very/ bad at specifying the interaction
between the acl and mode attributes. For example, consider an ACL like
"A::EVERYONE@:rwaDx" for a directory. This would correspond to a mode
attribute of "------rwx" according to the above statement, but the ACL
really grants "rwx" access to everyone including the owner and the
owning group, which would equate to a mode attribute of "rwxrwxrwx".
(Remember that the lower three mode bits indicate the permissions of
"others", which excludes the owner and the owning group, so
"------rwx" is not the same as "rwxrwxrwx".)

Andreas

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

[Ondrej Valousek]

> This would correspond to a mode attribute of "------rwx" according to the above statement, 
Well I do not think so as the RFC also states that EVERYONE@ means literally everyone (including group and owner), so the above example would indeed return rwxrwxrwx.

Anyhow, would the code I offered do the job? If not, can you suggest a better one?
Thanks,

Ondrej

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

[Andreas Grünbacher]

Am Di., 15. Nov. 2022 um 13:46 Uhr schrieb Ondrej Valousek
<ondrej.valousek.xm@renesas.com>:
> > This would correspond to a mode attribute of "------rwx" according to the above statement,
> Well I do not think so as the RFC also states that EVERYONE@ means literally everyone (including group and owner), so the above example would indeed return rwxrwxrwx.

Well, I've explained it to you, but I can't understand it for you.

> Anyhow, would the code I offered do the job?

You mean the AIX code? That's at least missing ACE4_DELETE_CHILD for
directories, ACE4_WRITE_ACL for OWNER@, and a bunch of permissions
which are probably always on on UNIX systems (ACE4_READ_ATTRIBUTES,
ACE4_SYNCHRONIZE). I'm not really sure what to do about
ACE4_READ_NAMED_ATTRS and ACE4_WRITE_NAMED_ATTRS; those are not the
same as Linux extended attributes. This will need a bit of testing
against various NFSv4 servers to give reasonable results.

Andreas

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

[Ondrej Valousek]

> I'm not really sure what to do about ACE4_READ_NAMED_ATTRS and ACE4_WRITE_NAMED_ATTRS; those are not the same as Linux extended attributes. This will need a bit of testing against various NFSv4 servers to give reasonable results.

Can we just commit the code as-is without the extra mask checks for now?
Frankly speaking:
- I still do not understand why we should be doing it, I perceive it as an unnecessary extra potentially fragile code
- the existing code just "works" well to me. Tried various scenarios on Netapp and Linux NFS server. It just works.
- I understand it might not be perfect, but I think even you will agree that it's far better than what we have now in there (i.e. nothing)
- since most of the heavy lifting has already been done, we can patch the masks later on (if needed) as it should be fairly easy job once we know exactly what to do there.

I am happy to incorporate the other suggestions you had (thanks for them), but I have reached the point that I can't invest more time into it.
Thanks,

Ondrej

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

[Paul Eggert]

On 11/14/22 00:16, Ondrej Valousek wrote:
> The disadvantage is that it pulls back dependency on libacl for some reason (the code does not
> use libacl at all

Yes, this needs to be fixed somehow, as part of the point of this Gnulib 
module is to not require programs like GNU ls to dynamically link to libacl.

It's also bad that GNU ls dynamically links to libpcre2, because 
libselinux requires libpcre2(!) but I digress.

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

[Andreas Grünbacher]

Am Di., 15. Nov. 2022 um 03:45 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:
> On 11/14/22 00:16, Ondrej Valousek wrote:
> > The disadvantage is that it pulls back dependency on libacl for some reason (the code does not
> > use libacl at all
>
> Yes, this needs to be fixed somehow, as part of the point of this Gnulib
> module is to not require programs like GNU ls to dynamically link to libacl.

Looks like an operator error; there's nothing in that patch that would
cause an additional dependency.

> It's also bad that GNU ls dynamically links to libpcre2, because
> libselinux requires libpcre2(!) but I digress.

Ouch.

Thanks,
Andreas

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

[Ondrej Valousek]

Hi Andreas,

> Historically, I've suggested taking care of these kinds of things in the richacl project, but that effort has been shot down upstream, and that project has been dead for a long time.

I think I have complete picture now. I also think that eventually I can eventually fix gnulib (hopefully the gnulib folks stay supportive) as there are already some pieces of code for AIX that I could possibly reuse.

Once I have something I'll kindly ask you for your opinion again.

Thanks,
Ondrej

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

[Paul Eggert]

On 2022-11-11 00:40, Ondrej Valousek wrote:
> I think I have complete picture now. I also think that eventually I can eventually fix gnulib (hopefully the gnulib folks stay supportive) as there are already some pieces of code for AIX that I could possibly reuse.

Thanks for taking this on.

What a mess, huh? I hope the changes to Gnulib will be as simple and 
easy to audit/understand as possible (sorry that this will mean more 
work for you...).

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

As per suggestions from Andreas, I am attaching a simplified version of the patch implementing basic
checks for non-trivial NFSv4 acls

---
 lib/file-has-acl.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index e02f0626a..37b3aaa30 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_LENGTH 80
 #endif
 
 /* Return 1 if NAME has a nontrivial access control list,
@@ -67,6 +72,14 @@ 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_LENGTH];
+
+	  ret = getxattr (name, XATTR_NAME_NFSV4_ACL, xattr, TRIVIAL_NFS4_ACL_LENGTH);
+	  if (ret < 0 && errno == ENODATA)
+              ret = 0;
+	  else ret = errno == ERANGE;  /* we won't fit into the buffer, so non-trivial ACL is presented */
+      }  
       if (ret < 0)
         return - acl_errno_valid (errno);
       return ret;
-- 
2.37.3

[PATCH] Basic support for checking NFSv4 ACLs in Linux

[Ondrej Valousek]

---
 lib/file-has-acl.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c
index e02f0626a..a6144e52e 100644
--- a/lib/file-has-acl.c
+++ b/lib/file-has-acl.c
@@ -32,6 +32,10 @@
 #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
 #endif
 
 /* Return 1 if NAME has a nontrivial access control list,
@@ -67,6 +71,26 @@ 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 */
+	  ret = getxattr (name, XATTR_NAME_NFSV4_ACL, NULL, 0);
+	  if (ret < 0 && errno == ENODATA)
+              ret = 0;
+	  else if (ret > 0) {
+              char *xattr;
+	      xattr = malloc(ret);
+	      if (!xattr) {
+		ret = -1;
+	      } else {
+                ret = getxattr (name, XATTR_NAME_NFSV4_ACL, xattr, ret);
+		if (ret < 0) ret = -1;
+		else {
+		    u_int32_t num_aces = (u_int32_t)ntohl(*((u_int32_t*)(xattr))); /* Grab the number of aces in the acl */
+		    ret = num_aces > 3;
+		}
+		free(xattr);
+	     }
+	  }
+      }  
       if (ret < 0)
         return - acl_errno_valid (errno);
       return ret;
-- 
2.37.3

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

[Bruno Haible]

Hi Ondrej,

Thanks for the proposed patch.

But before we go into details, I would like to understand the big picture:

  1) What is the scenario in which the patch should make a difference?
     An NFSv4 volume exported from a different machine, and mounted on Linux,
     right?
     Which OSes are supported for the exporting machine?
     Which configurations need to be applied on the exporting machine?
     Which mount options need to be used on the Linux machine?

  2) When I look at the acl-2.3.1 source code [1], I get the impression that
     the Linux kernel exports the ACL attributes also for NFS. Is this true?
     To which extend does the 'acl' package support NFSv4 ACLs? To which
     extent do the maintainers of this package plan to do so?

Bruno

[1] https://git.savannah.nongnu.org/cgit/acl.git/

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

[Ondrej Valousek]

[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]

Hi Bruno,
1. Yes, file_has_acl() function need to be able to detect ACLs on a NFSv4 mounted volume.
Such a volume has ACLs in special format which is quite different from Posix ACLs which we only handle today.
Note that nfsv4 is standard today I.e. if you mount remote volume today on Linux via "mount server:/volume ", it will most likely will be a NFSv4 mount where gnulib can't detect acls.
2. The 'acl' package (as of today) only supports Posix ACLs and not the NFSv4 style. And besides, we do not use it anyway on modern Linux distros - see the code, instead of calling acl library, we just call getxattr() function directly.

Does it help you to understand it now?
Thanks
Ondrej

Získat Outlook pro Android<https://aka.ms/AAb9ysg>
________________________________
From: Bruno Haible <bruno@clisp.org>
Sent: Thursday, October 27, 2022 9:52:32 PM
To: bug-gnulib@gnu.org <bug-gnulib@gnu.org>
Cc: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

Hi Ondrej,

Thanks for the proposed patch.

But before we go into details, I would like to understand the big picture:

  1) What is the scenario in which the patch should make a difference?
     An NFSv4 volume exported from a different machine, and mounted on Linux,
     right?
     Which OSes are supported for the exporting machine?
     Which configurations need to be applied on the exporting machine?
     Which mount options need to be used on the Linux machine?

  2) When I look at the acl-2.3.1 source code [1], I get the impression that
     the Linux kernel exports the ACL attributes also for NFS. Is this true?
     To which extend does the 'acl' package support NFSv4 ACLs? To which
     extent do the maintainers of this package plan to do so?

Bruno

[1] https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.savannah.nongnu.org%2Fcgit%2Facl.git%2F&amp;data=05%7C01%7Condrej.valousek.xm%40renesas.com%7Cbac072d29fdd4b1bb46808dab854cc82%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638024971616993649%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=tsoTFYvHtXRm2%2FqD76hpwewYpjFD73x2QwOS9iNa8lw%3D&amp;reserved=0




[-- Attachment #2: Type: text/html, Size: 4110 bytes --]

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

[Paul Eggert]

On 2022-10-28 07:33, Ondrej Valousek wrote:
> 2. The 'acl' package (as of today) only supports Posix ACLs and not the NFSv4 style. And besides, we do not use it anyway on modern Linux distros - see the code, instead of calling acl library, we just call getxattr() function directly.

Is that the best we can do? Surely we shouldn't expect user code to 
contain gems like this:

  u_int32_t num_aces = (u_int32_t)ntohl(*((u_int32_t*)(xattr))); /* Grab 
the number of aces in the acl

Wouldn't it be better to put this sort of thing into a library with a 
well-defined API (you can call it "nfsacl" if you like) and have Gnulib 
use that library? Gnulib is supposed to be a portability library, not a 
packet sniffer.

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

[Ondrej Valousek]

Well, I sort of understand your concern.
The "problem" here is:
1. You are doing a similar stuff already - check the code for Solaris for example.
2. I am happy to try to push this code into the libacl (where it would naturally should be probably), but then we need to get rid of the (fairly recently added) code:
# if GETXATTR_WITH_POSIX_ACLS

      ssize_t ret;

      ret = getxattr (name, XATTR_NAME_POSIX_ACL_ACCESS, NULL, 0);
...
#elsif   HAVE_ACL_GET_FILE
.. and call acl_extended_file() function from libacl. I then move my code into this function.

So in nutshell I do not quite understand why we are not calling acl_extended_file() any more on Linux. Maybe to simplify code & one less dependency?
Any suggestions here?


-----Original Message-----
From: Paul Eggert <eggert@cs.ucla.edu> 
Sent: neděle 30. října 2022 19:37
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>; Bruno Haible <bruno@clisp.org>; bug-gnulib@gnu.org
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

On 2022-10-28 07:33, Ondrej Valousek wrote:
> 2. The 'acl' package (as of today) only supports Posix ACLs and not the NFSv4 style. And besides, we do not use it anyway on modern Linux distros - see the code, instead of calling acl library, we just call getxattr() function directly.

Is that the best we can do? Surely we shouldn't expect user code to contain gems like this:

  u_int32_t num_aces = (u_int32_t)ntohl(*((u_int32_t*)(xattr))); /* Grab the number of aces in the acl

Wouldn't it be better to put this sort of thing into a library with a well-defined API (you can call it "nfsacl" if you like) and have Gnulib use that library? Gnulib is supposed to be a portability library, not a packet sniffer.

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

[Paul Eggert]

On 2022-10-31 01:05, Ondrej Valousek wrote:
> I do not quite understand why we are not calling acl_extended_file() any more on Linux

This was due to a 2015 patch by Andreas Gruenbacher:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=da6ebc941e966141e83591408545224274d0cf0f

that in hindsight was perhaps a mistake. I'll cc Andreas. Andreas, this 
thread is rooted here:

https://lists.gnu.org/r/bug-gnulib/2022-10/msg00031.html

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

[Ondrej Valousek]

Hi Andreas,
Can you please clarify this?
Note I have also submitted a possible patch to the libacl, in case we do not want to put this functionality to gnulib.
Afaik you are also maintainer of libacl... right?
Thanks
Ondrej

-----Original Message-----
From: Paul Eggert <eggert@cs.ucla.edu> 
Sent: pondělí 31. října 2022 20:37
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>; Bruno Haible <bruno@clisp.org>; bug-gnulib@gnu.org
Cc: Andreas Gruenbacher <andreas.gruenbacher@gmail.com>
Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux

On 2022-10-31 01:05, Ondrej Valousek wrote:
> I do not quite understand why we are not calling acl_extended_file() 
> any more on Linux

This was due to a 2015 patch by Andreas Gruenbacher:

https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.savannah.gnu.org%2Fcgit%2Fgnulib.git%2Fcommit%2F%3Fid%3Dda6ebc941e966141e83591408545224274d0cf0f&amp;data=05%7C01%7Condrej.valousek.xm%40renesas.com%7C8724fa8cc33f4ca6bcf008dabb773c80%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638028418039903659%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=pzxIX7lwx2LtOZOXLoAQ%2F91m37qRiiVWsyKpvtt9iIQ%3D&amp;reserved=0

that in hindsight was perhaps a mistake. I'll cc Andreas. Andreas, this thread is rooted here:

https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.gnu.org%2Fr%2Fbug-gnulib%2F2022-10%2Fmsg00031.html&amp;data=05%7C01%7Condrej.valousek.xm%40renesas.com%7C8724fa8cc33f4ca6bcf008dabb773c80%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638028418039903659%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=IIiUhPQ2MSOwvDJJho1gxZGSgiltlh5XPEYOOEPZ6Yg%3D&amp;reserved=0

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

[Andreas Grünbacher]

Am Mo., 7. Nov. 2022 um 13:45 Uhr schrieb Ondrej Valousek
<ondrej.valousek.xm@renesas.com>:
> Hi Andreas,
> Can you please clarify this?

Gnulib commit da6ebc941 ("acl: On Linux, check for acls without
libacl") switched from acl_extended_file to getxattr to avoid a
runtime dependency on libacl. I think that was mainly a benefit during
bootstrapping and in degraded environments. It's been a while, though.

> Note I have also submitted a possible patch to the libacl, in case we do not want to put this functionality to gnulib.

libacl only knows about POSIX ACLs, and that's on purpose to some
degree. The other kinds of ACLs work quite differently, such that the
POSIX ACL API doesn't match those kinds of ACLs very well. (For
example, the order of entries in POSIX ACLs doesn't matter / is
predefined, while different orders can lead to different results in
the NFSv4 ACL world.) It wouldn't make any sense to support NFSv4 ACLs
in acl_extended_file() only.

As far as support for Linux's client-side view of NFSv4 ACLs in
gnulib's file_has_acl() function goes, there is precedent for that for
other systems and kinds of ACLs, so I don't see why it shouldn't be
added there. It's a hopeless and unfixable mess already, and that
particular quirk won't change a thing.

The implementation should be changed to at least use a static buffer
on the stack that's just big enough for trivial NFSv4 ACLs. Instead of
probing the size and then reading the ACL, read the ACL right away. If
there is no ACL or if the ACL is trivial, we'll know right away. If
the ACL is too big, it must be non-trivial.

> Afaik you are also maintainer of libacl... right?
> Thanks
> Ondrej

Thanks,
Andreas