> On May 4, 2020, at 7:51 PM, Andrew Doran <ad%netbsd.org@localhost> wrote:
>>
Thanks for reviewing.
>
> +++ sbin/tunefs/tunefs.8
> ...
> +.It Fl n Cm enable | disable
> +Turn on/off the administrative NFSv4 ACL enable flag.
Fixed, this was before when:
-a = posix alls
-n = nfs alls
Now we have
-a = nfs acls
-p = posix alls
Since the nfs acls are preferred.
> 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.
Yup 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?
Indeed, there is a missing check here:
/*
* To modify the permissions on a file, must possess VADMIN
* for that file.
*/
if ((error = VOP_ACCESSX(vp, VWRITE_ACL, cred, td)) != 0)
return (error);
I have fixed it.
> 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.
Thanks, please let me know if you start working on this and I will do the same...
christos
Attachment:
signature.asc
Description: Message signed with OpenPGP