VOP_ACCESS() and new VADMIN/VATTRIB?

[Terry Lambert]

Robert Watson wrote:
> (Apologies again for wide-cross posting :-).
> 
> As described in my prior e-mail, I have updated my local tree to make use
> of a new VADMIN right for testing access control requests requiring
> "ownership" of a file system object to succeed.  I've attached below the
> patches required to support this change in UFS/FFS, although I have not
> yet updated the other file systems.  I'd like to go ahead and commit this
> support, as it is required for mandatory access control (centralizing the
> VADMIN decision means MAC policies can block VADMIN requests centrally in
> VOP_ACCESS() for file system that support labeling, rather than having MAC
> checks scattered through reems of file system code). 
> 
> However, I'd like to get someone (or several someone's :-) to review the
> code for correctness.  In particular, I'm concerned about VFS locking
> issues: VOP_ACCESS()  requires an exclusive lock on its vp argument, which
> is good since VOP_GETACL() and label retrieval functions will require a
> lock in UFS, but the requirement for a lock to test ip->i_uid directly
> wasn't explicit previously as part of the locking protocol.  I believe
> I've managed to demonstrate to myself that in locations where the VADMIN
> VOP_ACCESS() test occurs, a lock will always be held on the pertinent
> vnodes, but I'd like confirmation.  If it wasn't require before, the code
> was probably buggy anyway, but those bugs will become far more visible in
> a world where VOP_ACCESS() could involve a blocking call to access ACLs or
> label information.

I don't believe there are any locking issues; however, I do have
some problems with the direction this code is going.  It seems
to me that these patches centralize some code, at the expense of
future ability to reuse and/or maintain the code, without assuming
a default vaccess() based implementation.


> --- sys/ufs/ufs/ufs_lookup.c	2000/09/18 16:13:01	1.40
> +++ sys/ufs/ufs/ufs_lookup.c	2000/10/09 16:29:34
> @@ -476,9 +476,8 @@
>  		 * implements append-only directories.
>  		 */
>  		if ((dp->i_mode & ISVTX) &&
> -		    suser_xxx(cred, p, PRISON_ROOT) &&
> -		    cred->cr_uid != dp->i_uid &&
> -		    VTOI(tdp)->i_uid != cred->cr_uid) {
> +		    VOP_ACCESS(vdp, VADMIN, cred, cnp->cn_proc) &&
> +		    VOP_ACCESS(tdp, VADMIN, cred, cnp->cn_proc)) {
>  			vput(tdp);
>  			return (EPERM);
>  		}

The removal of the PRISON check hides the semantics and assumes
the use of vaccess() as the default for use by VOP_ACCESS().

This means that if I derive a different FS from the UFS code, and
don't use your new code, I'm suddenly vulnerable to a credential
exploit.

One of the main reasons that I disliked the "default" stuff
when it went in was a similar hidden semantics problem it
caused by forcing me to explicitly invlude a VFSOP/VNOP for
code for which I wanted the default behaviour from a VFS
stacking layer ("EOPNOTSUPP").

The problem with this, and which I think gets exacerbated by
the credential code changes you are proposing, is that for any
VNOP or VFSOP that's unknown to a stacking layer in an arbitrary
stack, is that if it is known to an underyling layer (perhaps
the underlying layer is newer than that stacked on top of it),
there is no way to preclude exposing the underlying layers
additional semantics to a caller, even if you wish to only
expose a minimal set... you can't.

I also think that this is perhaps a case where you want to
provide an alternate VNOP array, and expose this is a
seperate VFS.  I'd argue that the capabilities code should be
approached in a similar fashion, although given the extent,
impact, and initialization problems inherent to such code,
it seems to me that it should be a seperate stacking layer
in its own right: basically the same place where I would put
the quota code, and implement read-only mounts (through an
implied stacking request).


You could easily achieve the same effect by replacing the
ufs_access call and the other calls you are changing, rather
than wiring it into UFS proper.


At the very least, I think that it's important to document
your assumptions and their impact on the semantics that they
bring in with them, should this code go forward, as is.  Any
VFS author who follows you will inherit these assumptions,
and it's encumbant on you to insure that they don't inherit
a loaded gun aimed at their foot.


I have to say that my preference is that this type of code,
which fits very well into the model of a semantics imposition
stacking layer, really wants to wait until stacking is fixed,
and to be implemented where it fits best.


					Terry Lambert
					terry at lambert.org
---
Any opinions in this posting are my own and not those of my present
or previous employers.
To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-discuss" in the body of the message