Date: Tue, 31 Oct 2006 15:40:50 +0100 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: arch@FreeBSD.org Subject: Re: New in-kernel privilege API: priv(9) Message-ID: <20061031144050.GC13359@garage.freebsd.pl> In-Reply-To: <20061031092122.D96078@fledge.watson.org> References: <20061031092122.D96078@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--Km1U/tdNT/EmXiR1
Content-Type: text/plain; charset=iso-8859-2
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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 t=
he "Reviewed by:" list. :-) This is, of course, a set of highly sensitive =
security-related=20
> changes, and having detailed reviews is very important.
[...]
Here are few nits I found:
> Index: sys/fs/hpfs/hpfs_vnops.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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 !=3D VNOVAL || vap->va_mtime.tv_sec !=3D VNOVA=
L) {
> if (vp->v_mount->mnt_flag & MNT_RDONLY)
> return (EROFS);
> - if (cred->cr_uid !=3D hp->h_uid &&
> - (error =3D suser_cred(cred, SUSER_ALLOWJAIL)) &&
> - ((vap->va_vaflags & VA_UTIMES_NULL) =3D=3D 0 ||
> - (error =3D VOP_ACCESS(vp, VWRITE, cred, td))))
> - return (error);
> + if (vap->va_vaflags & VA_UTIMES_NULL) {
> + error =3D VOP_ACCESS(vp, VADMIN, cred, td);
> + if (error)
> + error =3D VOP_ACCESS(vp, VWRITE, cred, td);
> + } else
> + error =3D 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 =3D pmp->pm_devvp;
> - vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> - error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> - td->td_ucred, td);
> - if (error) {
> - VOP_UNLOCK(devvp, 0, td);
> - return (error);
> - }
> + devvp =3D pmp->pm_devvp;
> + vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> + error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> + td->td_ucred, td);
> + if (error)
> + error =3D 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 =3D pmp->pm_devvp;
vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
td->td_ucred, td);
VOP_UNLOCK(devvp, 0, td);
if (!error)
error =3D 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 =3D VREAD;
> - if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> - accessmode |=3D VWRITE;
> - error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> - if (error) {
> - vput(devvp);
> - return (error);
> - }
> + accessmode =3D VREAD;
> + if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> + accessmode |=3D VWRITE;
> + error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> + if (error)
> + error =3D 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 =3D suser(td)) !=3D 0)
> - return (error);
> + error =3D 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 =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> - td->td_ucred, td)) !=3D 0) {
> - VOP_UNLOCK(devvp, 0, td);
> - return (error);
> - }
> + vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> + error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> + td->td_ucred, td);
> + if (error)
> + error =3D 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 =3D VREAD;
> - if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> - accessmode |=3D VWRITE;
> - if ((error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td)) !=3D 0=
) {
> - vput(devvp);
> - return (error);
> - }
> + accessmode =3D VREAD;
> + if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> + accessmode |=3D VWRITE;
> + error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> + if (error)
> + error =3D 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 @@
>=20
> /* If mount by non-root, then verify that user has necessary
> * permissions on the device. */
> - if (suser(td)) {
> - accessmode =3D VREAD;
> - if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> - accessmode |=3D VWRITE;
> - if ((error =3D VOP_ACCESS(devvp,
> - accessmode, td->td_ucred, td)) !=3D 0) {
> - vput(devvp);
> - return (error);
> - }
> + accessmode =3D VREAD;
> + if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> + accessmode |=3D VWRITE;
> + error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> + if (error)
> + error =3D 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);
>=20
> ronly =3D ((XFS_MTOVFS(mp)->vfs_flag & VFS_RDONLY) !=3D 0);
> - if (suser(td)) {
> - accessmode =3D VREAD;
> - if (!ronly)
> - accessmode |=3D VWRITE;
> - if ((error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td))!=3D 0){
> - vput(devvp);
> - return (error);
> - }
> + accessmode =3D VREAD;
> + if (!ronly)
> + accessmode |=3D VWRITE;
> + error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> + if (error)
> + error =3D 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 @@
> }
> }
>=20
> +/*
> + * Check with permission for a specific privilege is granted within jail=
=2E 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) !=3D 0))
> - ATM_RETERR(EPERM);
> + if (td !=3D 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;
>=20
> - error =3D suser(req->td);
> + error =3D 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 =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> - td->td_ucred, td)) !=3D 0) {
> - VOP_UNLOCK(devvp, 0, td);
> - return (error);
> - }
> + vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY, td);
> + error =3D VOP_ACCESS(devvp, VREAD | VWRITE,
> + td->td_ucred, td);
> + if (error)
> + error =3D 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 =3D VREAD;
> - if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> - accessmode |=3D VWRITE;
> - if ((error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td))!=3D 0){
> - vput(devvp);
> - return (error);
> - }
> + accessmode =3D VREAD;
> + if ((mp->mnt_flag & MNT_RDONLY) =3D=3D 0)
> + accessmode |=3D VWRITE;
> + error =3D VOP_ACCESS(devvp, accessmode, td->td_ucred, td);
> + if (error)
> + error =3D priv_check(td, PRIV_VFS_MOUNT_PERM);
s/if (error)/if (!error)/
--=20
Pawel Jakub Dawidek http://www.wheel.pl
pjd@FreeBSD.org http://www.FreeBSD.org
FreeBSD committer Am I Evil? Yes, I Am!
--Km1U/tdNT/EmXiR1
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (FreeBSD)
iD8DBQFFR2ByForvXbEpPzQRAvBrAJ9u+xKNpzsGRMrg85XSsT8wP+v0CgCgtPRk
h3KCt3DFXZfzw2awaUx1B/Y=
=sXNL
-----END PGP SIGNATURE-----
--Km1U/tdNT/EmXiR1--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061031144050.GC13359>