New in-kernel privilege API: priv(9)

Pawel Jakub Dawidek pjd at FreeBSD.org
Tue Oct 31 14:42:00 UTC 2006


On Tue, Oct 31, 2006 at 09:43:45AM +0000, Robert Watson wrote:
> Among other things, I'd like to be able to add some additional names to the "Reviewed by:" list. :-)  This is, of course, a set of highly sensitive security-related 
> changes, and having detailed reviews is very important.
[...]

Here are few nits I found:

> 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.

> --- sys/fs/msdosfs/msdosfs_vfsops.c	26 Sep 2006 04:12:45 -0000	1.153
> +++ sys/fs/msdosfs/msdosfs_vfsops.c	30 Oct 2006 17:07:55 -0000
[...]
> @@ -293,17 +294,17 @@
>  			 * If upgrade to read-write by non-root, then verify
>  			 * that user has necessary permissions on the device.
>  			 */
> -			if (suser(td)) {
> -				devvp = pmp->pm_devvp;
> -				vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> -				error = VOP_ACCESS(devvp, VREAD | VWRITE,
> -						   td->td_ucred, td);
> -				if (error) {
> -					VOP_UNLOCK(devvp, 0, td);
> -					return (error);
> -				}
> +			devvp = pmp->pm_devvp;
> +			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> +			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);

> @@ -353,15 +354,15 @@
>  	 * If mount by non-root, then verify that user has necessary
>  	 * permissions on the device.
>  	 */
> -	if (suser(td)) {
> -		accessmode = VREAD;
> -		if ((mp->mnt_flag & MNT_RDONLY) == 0)
> -			accessmode |= VWRITE;
> -		error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> -		if (error) {
> -			vput(devvp);
> -			return (error);
> -		}
> +	accessmode = VREAD;
> +	if ((mp->mnt_flag & MNT_RDONLY) == 0)
> +		accessmode |= VWRITE;
> +	error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> +	if (error)
> +		error = priv_check(td, PRIV_VFS_MOUNT_PERM);
> +	if (error) {
> +		vput(devvp);
> +		return (error);

Same here.

> --- sys/fs/umapfs/umap_vfsops.c	26 Sep 2006 04:12:46 -0000	1.65
> +++ sys/fs/umapfs/umap_vfsops.c	30 Oct 2006 17:07:55 -0000
> @@ -88,8 +88,9 @@
>  	/*
>  	 * Only for root
>  	 */
> -	if ((error = suser(td)) != 0)
> -		return (error);
> +	error = priv_check(td, PRIV_VFS_MOUNT);
> +	if (error)
> +		return (ERROR);

s/ERROR/error/

> --- sys/gnu/fs/ext2fs/ext2_vfsops.c	26 Sep 2006 04:12:47 -0000	1.158
> +++ sys/gnu/fs/ext2fs/ext2_vfsops.c	30 Oct 2006 17:07:55 -0000
[...]
> @@ -197,15 +198,16 @@
>  			 * If upgrade to read-write by non-root, then verify
>  			 * that user has necessary permissions on the device.
>  			 */
> -			if (suser(td)) {
> -				vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> -				if ((error = VOP_ACCESS(devvp, VREAD | VWRITE,
> -				    td->td_ucred, td)) != 0) {
> -					VOP_UNLOCK(devvp, 0, td);
> -					return (error);
> -				}
> +			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> +			error = VOP_ACCESS(devvp, VREAD | VWRITE,
> +			    td->td_ucred, td);
> +			if (error)
> +				error = priv_check(td, PRIV_VFS_MOUNT_PERM);

Another s/if (error)/if (!error)/

> @@ -259,15 +261,18 @@
>  	/*
>  	 * If mount by non-root, then verify that user has necessary
>  	 * permissions on the device.
> +	 *
> +	 * XXXRW: VOP_ACCESS() enough?
>  	 */
> -	if (suser(td)) {
> -		accessmode = VREAD;
> -		if ((mp->mnt_flag & MNT_RDONLY) == 0)
> -			accessmode |= VWRITE;
> -		if ((error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td)) != 0) {
> -			vput(devvp);
> -			return (error);
> -		}
> +	accessmode = VREAD;
> +	if ((mp->mnt_flag & MNT_RDONLY) == 0)
> +		accessmode |= VWRITE;
> +	error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> +	if (error)
> +		error = priv_check(td, PRIV_VFS_MOUNT_PERM);

And again.

> --- sys/gnu/fs/reiserfs/reiserfs_vfsops.c	26 Sep 2006 04:12:47 -0000	1.6
> +++ sys/gnu/fs/reiserfs/reiserfs_vfsops.c	30 Oct 2006 17:07:55 -0000
> @@ -125,15 +125,15 @@
> 
>  	/* If mount by non-root, then verify that user has necessary
>  	 * permissions on the device. */
> -	if (suser(td)) {
> -		accessmode = VREAD;
> -		if ((mp->mnt_flag & MNT_RDONLY) == 0)
> -			accessmode |= VWRITE;
> -		if ((error = VOP_ACCESS(devvp,
> -		    accessmode, td->td_ucred, td)) != 0) {
> -			vput(devvp);
> -			return (error);
> -		}
> +	accessmode = VREAD;
> +	if ((mp->mnt_flag & MNT_RDONLY) == 0)
> +		accessmode |= VWRITE;
> +	error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> +	if (error)
> +		error = priv_check(td, PRIV_VFS_MOUNT_PERM);

One more.

> --- sys/gnu/fs/xfs/FreeBSD/xfs_super.c	10 Jun 2006 19:02:13 -0000	1.4
> +++ sys/gnu/fs/xfs/FreeBSD/xfs_super.c	30 Oct 2006 17:07:55 -0000
[...]
> @@ -149,14 +151,15 @@
>  	vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> 
>  	ronly = ((XFS_MTOVFS(mp)->vfs_flag & VFS_RDONLY) != 0);
> -	if (suser(td)) {
> -		accessmode = VREAD;
> -		if (!ronly)
> -			accessmode |= VWRITE;
> -		if ((error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td))!= 0){
> -			vput(devvp);
> -			return (error);
> -		}
> +	accessmode = VREAD;
> +	if (!ronly)
> +		accessmode |= VWRITE;
> +	error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> +	if (error)
> +		error = priv_check(td, PRIV_VFS_MOUNT_PERM);

Another one.

> --- sys/kern/kern_jail.c	22 Oct 2006 11:52:13 -0000	1.53
> +++ sys/kern/kern_jail.c	30 Oct 2006 17:07:55 -0000
[...]
> @@ -523,6 +524,172 @@
>  	}
>  }
> 
> +/*
> + * Check with permission for a specific privilege is granted within jail.  We
> + * have a specific list of accepted privileges; the rest are denied.
> + */
> +int
> +prison_priv_check(struct ucred *cred, int priv)
> +{
> +
> +	if (!(jailed(cred)))
> +		return (0);

Style: if (!jailed(cred))

> --- sys/netatm/atm_usrreq.c	21 Jul 2006 17:11:13 -0000	1.27
> +++ sys/netatm/atm_usrreq.c	30 Oct 2006 17:07:55 -0000
> -		if (td && (suser(td) != 0))
> -			ATM_RETERR(EPERM);
> +		if (td != 0) {

Style: s/0/NULL/

> --- sys/netinet6/udp6_usrreq.c	7 Sep 2006 18:44:54 -0000	1.68
> +++ sys/netinet6/udp6_usrreq.c	30 Oct 2006 17:07:56 -0000
[...]
> @@ -434,7 +435,8 @@
>  	struct inpcb *inp;
>  	int error;
> 
> -	error = suser(req->td);
> +	error = priv_check_cred(req->td->td_ucred, PRIV_NETINET_GETCRED,
> +	    SUSER_ALLOWJAIL);

Change from not allowing jailed root to allowing jailed root was
intentional?

> --- sys/ufs/ffs/ffs_vfsops.c	22 Oct 2006 11:52:19 -0000	1.321
> +++ sys/ufs/ffs/ffs_vfsops.c	30 Oct 2006 17:07:56 -0000
[...]
> @@ -257,15 +258,16 @@
>  			 * If upgrade to read-write by non-root, then verify
>  			 * that user has necessary permissions on the device.
>  			 */
> -			if (suser(td)) {
> -				vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> -				if ((error = VOP_ACCESS(devvp, VREAD | VWRITE,
> -				    td->td_ucred, td)) != 0) {
> -					VOP_UNLOCK(devvp, 0, td);
> -					return (error);
> -				}
> +			vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> +			error = VOP_ACCESS(devvp, VREAD | VWRITE,
> +			    td->td_ucred, td);
> +			if (error)
> +				error = priv_check(td, PRIV_VFS_MOUNT_PERM);

s/if (error)/if (!error)/

> @@ -364,14 +366,15 @@
>  	 * If mount by non-root, then verify that user has necessary
>  	 * permissions on the device.
>  	 */
> -	if (suser(td)) {
> -		accessmode = VREAD;
> -		if ((mp->mnt_flag & MNT_RDONLY) == 0)
> -			accessmode |= VWRITE;
> -		if ((error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td))!= 0){
> -			vput(devvp);
> -			return (error);
> -		}
> +	accessmode = VREAD;
> +	if ((mp->mnt_flag & MNT_RDONLY) == 0)
> +		accessmode |= VWRITE;
> +	error = VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> +	if (error)
> +		error = priv_check(td, PRIV_VFS_MOUNT_PERM);

s/if (error)/if (!error)/

-- 
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/ef032b37/attachment.pgp


More information about the freebsd-arch mailing list