Re: ACL patch

[Andrew Doran <ad%netbsd.org@localhost>]

On Sat, May 02, 2020 at 07:10:29PM -0400, Christos Zoulas wrote:

> Hi,
> 
> The following patch ports the FreeBSD FFS ACLS (both posix1e and nfs4) to
> NetBSD. Comments? I will let this sit for a week or so and then commit it
> if I don't hear screams.
> 
> [it is ~24K lines, so not posted inline]
> 
> https://www.netbsd.org/~christos/acl.diff

	+++ sbin/tunefs/tunefs.8	
	...
	+.It Fl n Cm enable | disable
	+Turn on/off the administrative NFSv4 ACL enable flag.

Is that handled?  I don't see it.

	--- share/mk/bsd.own.mk	27 Apr 2020 03:15:12 -0000	1.1187
	+++ share/mk/bsd.own.mk	2 May 2020 23:06:42 -0000
	@@ -164,7 +164,9 @@ EXTERNAL_GDB_SUBDIR=		/does/not/exist
	#
	 .if ${MACHINE_ARCH} == "x86_64" || ${MACHINE_ARCH} == "i386" || \
	     ${MACHINE_ARCH} == "powerpc64" || ${MACHINE_ARCH} == "powerpc" || \
	-    ${MACHINE_CPU} == "aarch64" || ${MACHINE_CPU} == "arm"
	+    ${MACHINE_CPU} == "aarch64" || ${MACHINE_CPU} == "arm" || \
	+    ${MACHINE_CPU} == "sparc64" || ${MACHINE_CPU} == "sparc" || \
	+    ${MACHINE_CPU} == "mips"

Seems unrelated.

	@@ -739,10 +1206,6 @@ genfs_can_chmod(enum vtype type, kauth_c
	 {
	 	int error;
 
	-	/* The user must own the file. */
	-	if (kauth_cred_geteuid(cred) != cur_uid)
	-		return (EPERM);
	-
 	/*
 	 * Unprivileged users can't set the sticky bit on files.
 	 */
	@@ -762,6 +1225,12 @@ genfs_can_chmod(enum vtype type, kauth_c
	 			return (EPERM);
	 	}
 
	+	/*
	+	 * Deny setting setuid if we are not the file owner.
	+	 */
	+	if ((new_mode & S_ISUID) && cur_uid != kauth_cred_geteuid(cred))
	+		return (EPERM);
	+

Are you sure this is correct?

Maybe I missed it but I didn't see a way for cache_lookup_linked() and
cache_have_id() to know a vnode has ACLs.  The presence of ACLs means those
routines can't do their imitation of VOP_ACCESS() and need to fail so that
the lookup is handled by VOP_LOOKUP().  To handle that on a per-vnode basis
you'd want to flag the "has ACLs" fact when an inode is loaded, and do the
same when an ACL is added to an in-core inode.  I'll look into it over the
next few days unless you want to take care of it.

Cheers,
Andrew


> Best,
> 
> 
> christos