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