Re: New in-kernel privilege API: priv(9)

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@xxxxxxxxxxx http://www.FreeBSD.org
FreeBSD committer Am I Evil? Yes, I Am!

Attachment: pgpRO63ascO4W.pgp
Description: PGP signature

Relevant Pages

  • Re: Reboot says "Not genuine", but WGA tool says "Genuine"
    ... privilege, not a right. ... American president has even said citizenship for those born in America ... Could the owner not filter out anyone it wished? ... often acrimonious involving both the public and Intel employees. ...
    (microsoft.public.windowsupdate)
  • Re: Reboot says "Not genuine", but WGA tool says "Genuine"
    ... privilege, not a right. ... American president has even said citizenship for those born in America ... Could the owner not filter out anyone it wished? ... often acrimonous involving both the public and Intel employees. ...
    (microsoft.public.windowsupdate)
  • Re: Question about revoking select from PUBLIC
    ... on these tables from PUBLIC and grant select on these tables only to ... Then prepend the schema owner. ... I am sure that either I or the owner did the grant and I dont ... which had the SELECT privilege granted to PUBLIC. ...
    (comp.databases.oracle.misc)
  • Re: Reboot says "Not genuine", but WGA tool says "Genuine"
    ... repressors, and control freaks everywhere. ... privilege, not a right. ... American president has even said citizenship for those born in America ... Could the owner not filter out anyone it wished? ...
    (microsoft.public.windowsupdate)
  • Re: New in-kernel privilege API: priv(9)
    ... diff -u -r1.68 hpfs_vnops.c ... The key here is that VOP_ACCESSwill also perform a privilege check, so the use of privilege in the existing code appears to be redundant. ... % retrieving revision 1.239 ... so the superuser can override on that basis. ...
    (freebsd-arch)