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 <agr...@gnu.org>
Sent: pátek 25. listopadu 2022 11:17
To: Ondrej Valousek <ondrej.valousek...@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 <agr...@gnu.org>:
> Am Do., 24. Nov. 2022 um 18:21 Uhr schrieb Ondrej Valousek
> <ondrej.valousek...@renesas.com>:
> > Hi GNU lib maintainers,
> >
> > Attaching the final version of patch introducing a basic checks for
> > non-trivial NFSv4 style ACLs.
> > It is substantially longer unfortunately - that's why I put most of
> > the code to acl-internal.c where I think it should be (as similar function
> > for AIX already exists there).
> > The benefit of this is fairly improved robustness of trivial ACLs detection.
> > The disadvantage is that it pulls back dependency on libacl for some
> > reason (the code does not use libacl at all so I expect some linker flag
> > like -as-needed would to the trick).
> >
> > I have tested it against Netapp and Linux based NFS servers under various
> > conditions.
> > Works OK to me. Maybe it's not perfect, but it's able to detect 99%
> > cases where ACLs returned by NFS server are not trivial (i.e. POSIX
> > mode bits does not fully represent access permissions).
> >
> > Let's hope the code is not forgotten and will be perhaps merged at some
> > stage.
> > Ondrej
> >
> >
> > diff --git a/lib/acl-internal.c b/lib/acl-internal.c index
> > be244c67a..36e29ac02 100644
> > --- a/lib/acl-internal.c
> > +++ b/lib/acl-internal.c
> > @@ -25,6 +25,9 @@
> >
> > #if USE_ACL && HAVE_ACL_GET_FILE /* Linux, FreeBSD, Mac OS X, IRIX,
> > Tru64, Cygwin >= 2.5 */
> >
> > +#include <string.h>
> > +# include <arpa/inet.h>
> > +
> > # if HAVE_ACL_TYPE_EXTENDED /* Mac OS X */
> >
> > /* ACL is an ACL, from a file, stored as type ACL_TYPE_EXTENDED.
> > @@ -122,6 +125,86 @@ acl_default_nontrivial (acl_t acl)
> > return (acl_entries (acl) > 0);
> > }
> >
> > +#define ACE4_WHO_OWNER "OWNER@"
> > +#define ACE4_WHO_GROUP "GROUP@"
> > +#define ACE4_WHO_EVERYONE "EVERYONE@"
> > +
> > +#define ACE4_ACCESS_ALLOWED_ACE_TYPE 0
> > +#define ACE4_ACCESS_DENIED_ACE_TYPE 1
> > +/* ACE flag values */
> > +
> > +#define ACE4_IDENTIFIER_GROUP 0x00000040
> > +
> > +int
> > +acl_nfs4_nontrivial (char *xattr, int len) {
> > + int ret = 0,wholen,bufs = len;
> > + uint32_t flag,type,num_aces = ntohl(*((uint32_t*)(xattr))); /*
> > Grab the number of aces in the acl */
> > + char *bufp = xattr;
> > + int num_a_aces = 0, num_d_aces = 0;
> > +
> > + ret = 0;
> > + bufp += 4; /* sizeof(uint32_t); */
> > + bufs -= 4;
> > +
> > +
> > + for(uint32_t ace_n = 0; num_aces > ace_n ; ace_n++) {
> > + int d_ptr;
> > +
> > + /* Get the acl type */
> > + if(bufs <= 0) return -1;
> > +
> > + type = ntohl(*((uint32_t*)bufp));
> > +
> > + bufp += 4;
> > + bufs -= 4;
> > + if (bufs <= 0) return -1;
> > +
> > + flag = ntohl(*((uint32_t*)bufp));
> > + /* As per RFC 7530, the flag should be 0, but we are just
> > generous to Netapp
> > + * and also accept the Group flag
> > + */
>
> We are not "generous" here, RFC 7530 requires to ignore the
> ACE4_IDENTIFIER_GROUP flag for entries with special who values like
> OWNER@, GROUP@, EVERYONE@.
>
> > + if (flag & ~ACE4_IDENTIFIER_GROUP) return 1;
> > +
> > + /* we skip mask - it's too risky to test it and it does not
> > seem to be actually needed */
> > + bufp += 2*4;
> > + bufs -= 2*4;
>
> I've already explained to you why checking the mask flags isn't optional:
>
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.gnu.org%2Farchive%2Fhtml%2Fbug-gnulib%2F2022-11%2Fmsg00064.html&
> data=05%7C01%7Condrej.valousek.xm%40renesas.com%7Ceb21a75eff044241a375
> 08dacece459b%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638049682570
> 686986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJ
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=dpFqzx2rTjKH92%2
> FRUTIa8vq5It5800uipgIUDiJSEHA%3D&reserved=0
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.gnu.org%2Farchive%2Fhtml%2Fbug-gnulib%2F2022-11%2Fmsg00068.html&
> data=05%7C01%7Condrej.valousek.xm%40renesas.com%7Ceb21a75eff044241a375
> 08dacece459b%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638049682570
> 686986%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJ
> BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pm3jn8ByCywefLEp
> xQXkRBbho3VS%2BgrIvsgDPCs9mZQ%3D&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
