New in-kernel privilege API: priv(9)

[Pawel Jakub Dawidek]

On Tue, Oct 31, 2006 at 03:04:30PM +0000, Robert Watson wrote:
> 
> On Tue, 31 Oct 2006, Pawel Jakub Dawidek wrote:
> 
> >Here are few nits I found:
> 
> Thanks!  Comments below.
> 
> >>Index: sys/fs/hpfs/hpfs_vnops.c
> >>===================================================================
> >>RCS file: /zoo/cvsup/FreeBSD-CVS/src/sys/fs/hpfs/hpfs_vnops.c,v
> >>retrieving revision 1.68
> >>diff -u -r1.68 hpfs_vnops.c
> >>--- sys/fs/hpfs/hpfs_vnops.c	17 Jan 2006 17:29:01 -0000	1.68
> >>+++ sys/fs/hpfs/hpfs_vnops.c	30 Oct 2006 17:07:55 -0000
> >>@@ -501,11 +501,12 @@
> >> 	if (vap->va_atime.tv_sec != VNOVAL || vap->va_mtime.tv_sec != VNOVAL) {
> >> 		if (vp->v_mount->mnt_flag & MNT_RDONLY)
> >> 			return (EROFS);
> >>-		if (cred->cr_uid != hp->h_uid &&
> >>-		    (error = suser_cred(cred, SUSER_ALLOWJAIL)) &&
> >>-		    ((vap->va_vaflags & VA_UTIMES_NULL) == 0 ||
> >>-		    (error = VOP_ACCESS(vp, VWRITE, cred, td))))
> >>-			return (error);
> >>+		if (vap->va_vaflags & VA_UTIMES_NULL) {
> >>+			error = VOP_ACCESS(vp, VADMIN, cred, td);
> >>+			if (error)
> >>+				error = VOP_ACCESS(vp, VWRITE, cred, td);
> >>+		} else
> >>+			error = VOP_ACCESS(vp, VADMIN, cred, td);
> >
> >Eliminating privilege check here was intentional? Doesn't it change functionality? I found the same check in few other places, so it's probably intentional, but worth 
> >checking still.
> 
> Yeah, it took my quite a while to puzzle through what the security check here is supposed to accomplish, but once I did, the correct structure became more clear.  The key 
> here is that VOP_ACCESS() will also perform a privilege check, so the use of privilege in the existing code appears to be redundant.  The logic here should essentially 
> read:
> 
> - To update the time stamp to the current time (VA_UTIMES_NULL), you must
>   either be the owner or have write access.  The correct error value is
>   EACCES, so we try the write check only if the owner check fails, so that the
>   error from the write check overrides the owner result.
> 
> - To update the time stamp to any other time, you must be the owner.  The
>   error here is EPERM on failure.
> 
> Given this description, do you think the change is right?

Yes, I agree. I really hate complex ifs like the one you replaced:)

> >>+			error = VOP_ACCESS(devvp, VREAD | VWRITE,
> >>+			   td->td_ucred, td);
> >>+			if (error)
> >>+				error = priv_check(td, PRIV_VFS_MOUNT_PERM);
> >>+			if (error) {
> >> 				VOP_UNLOCK(devvp, 0, td);
> >>+				return (error);
> >> 			}
> >>+			VOP_UNLOCK(devvp, 0, td);
> >
> >This doesn't seem right to me. If VOP_ACCESS() returns an error if call priv_check(), I think you wanted to call it when it return success, no?
> >
> >I'd change it to:
> >
> >			devvp = pmp->pm_devvp;
> >			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> >			error = VOP_ACCESS(devvp, VREAD | VWRITE,
> >			   td->td_ucred, td);
> >			VOP_UNLOCK(devvp, 0, td);
> >			if (!error)
> >				error = priv_check(td, PRIV_VFS_MOUNT_PERM);
> >			if (error)
> >				return (error);
> 
> Again, this is a tricky case in the original code.  I believe the intended logic here, in English, is:
> 
> - In order to mount on a device vnode, you must be able to read and write to
>   it.  This is subject to the normal definition of read and write privilege,
>   so the superuser can override on that basis (as part of VOP_ACCESS).
> 
> - However, if you don't have read and write access, you can also use privilege
>   (PRIV_VFS_MOUNT_PERM) to override that.
> 
> This is expressed in the original code by first checking for privilege, and if that fails, then looking for read/write access.  This doesn't make a lot of sense because the 
> read/write check is also exempted by privilege, but was probably "a little faster" because suser() was cheaper than VOP_ACCESS().
> 
> The reason we call priv_check() in the error case is that we only want to check for privilege if the normal access control check fails, in which case we override the normal 
> access control error check with the result of the privilege check.
> 
> Given this description, does the new code still make sense?

After more thinking I understand and agree with your change, maybe we
can add a comment describing the behaviour as it is not obvious on first
look.

We could still move VOP_UNLOCK() right after VOP_ACCESS(), BTW.

-- 
Pawel Jakub Dawidek                       http://www.wheel.pl
pjd at FreeBSD.org                           http://www.FreeBSD.org
FreeBSD committer                         Am I Evil? Yes, I Am!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20061031/c5144fc6/attachment.pgp