Date: Mon, 9 Oct 2000 12:49:39 -0400 (EDT) From: Robert Watson <rwatson@FreeBSD.org> To: freebsd-fs@FreeBSD.org Cc: freebsd-arch@FreeBSD.org, trustedbsd-discuss@TrustedBSD.org Subject: Re: VOP_ACCESS() and new VADMIN/VATTRIB? Message-ID: <Pine.NEB.3.96L.1001009122343.90573I-100000@fledge.watson.org> In-Reply-To: <Pine.NEB.3.96L.1000926203644.79897G-100000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
(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. Robert N M Watson robert@fledge.watson.org http://www.watson.org/~robert/ PGP key fingerprint: AF B5 5F FF A6 4A 79 37 ED 5F 55 E9 58 04 6A B1 TIS Labs at Network Associates, Safeport Network Services Index: sys/sys/vnode.h =================================================================== RCS file: /home/ncvs/src/sys/sys/vnode.h,v retrieving revision 1.130 diff -u -r1.130 vnode.h --- sys/sys/vnode.h 2000/10/04 01:29:15 1.130 +++ sys/sys/vnode.h 2000/10/09 16:29:27 @@ -218,12 +218,13 @@ /* * Modes. Some values same as Ixxx entries from inode.h for now. */ -#define VSUID 04000 /* set user id on execution */ -#define VSGID 02000 /* set group id on execution */ -#define VSVTX 01000 /* save swapped text even after use */ -#define VREAD 00400 /* read, write, execute permissions */ -#define VWRITE 00200 -#define VEXEC 00100 +#define VADMIN 010000 /* permission to administer vnode */ +#define VSUID 004000 /* set user id on execution */ +#define VSGID 002000 /* set group id on execution */ +#define VSVTX 001000 /* save swapped text even after use */ +#define VREAD 000400 /* read, write, execute permissions */ +#define VWRITE 000200 +#define VEXEC 000100 /* * Token indicating no attribute value yet assigned. Index: sys/kern/vfs_subr.c =================================================================== RCS file: /home/ncvs/src/sys/kern/vfs_subr.c,v retrieving revision 1.285 diff -u -r1.285 vfs_subr.c --- sys/kern/vfs_subr.c 2000/10/06 08:04:48 1.285 +++ sys/kern/vfs_subr.c 2000/10/09 16:29:33 @@ -3050,6 +3050,7 @@ /* Check the owner. */ if (cred->cr_uid == file_uid) { + dac_granted |= VADMIN; if (file_mode & S_IXUSR) dac_granted |= VEXEC; if (file_mode & S_IRUSR) @@ -3116,6 +3117,10 @@ if ((acc_mode & VWRITE) && ((dac_granted & VWRITE) == 0) && !cap_check_xxx(cred, NULL, CAP_DAC_WRITE, PRISON_ROOT)) cap_granted |= VWRITE; + + if ((acc_mode & VADMIN) && ((dac_granted & VADMIN) == 0) && + !cap_check_xxx(cred, NULL, CAP_FOWNER, PRISON_ROOT)) + cap_granted |= VADMIN; if ((acc_mode & (cap_granted | dac_granted)) == acc_mode) { /* XXX audit: privilege used */ Index: sys/ufs/ufs/ufs_lookup.c =================================================================== RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_lookup.c,v retrieving revision 1.40 diff -u -r1.40 ufs_lookup.c --- 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); } Index: sys/ufs/ufs/ufs_vnops.c =================================================================== RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_vnops.c,v retrieving revision 1.150 diff -u -r1.150 ufs_vnops.c --- sys/ufs/ufs/ufs_vnops.c 2000/10/04 01:29:17 1.150 +++ sys/ufs/ufs/ufs_vnops.c 2000/10/09 16:29:36 @@ -411,13 +411,17 @@ if (vp->v_mount->mnt_flag & MNT_RDONLY) return (EROFS); /* - * Privileged processes in jail() are permitted to modify - * arbitrary user flags on files, but are not permitted - * to modify system flags. + * Callers may only modify the file flags on objects they + * have VADMIN rights for. */ - if (cred->cr_uid != ip->i_uid && - (error = suser_xxx(cred, p, PRISON_ROOT))) + if ((error = VOP_ACCESS(vp, VADMIN, cred, p))) return (error); + /* + * Unprivileged processes and privileged processes in + * jail() are not permitted to set system flags. + * Privileged processes not in jail() may only set system + * flags if the securelevel <= 0. + */ if (!suser_xxx(cred, NULL, 0)) { if ((ip->i_flags & (SF_NOUNLINK | SF_IMMUTABLE | SF_APPEND)) && @@ -450,7 +454,8 @@ if (vap->va_uid != (uid_t)VNOVAL || vap->va_gid != (gid_t)VNOVAL) { if (vp->v_mount->mnt_flag & MNT_RDONLY) return (EROFS); - if ((error = ufs_chown(vp, vap->va_uid, vap->va_gid, cred, p)) != 0) + if ((error = ufs_chown(vp, vap->va_uid, vap->va_gid, cred, + p)) != 0) return (error); } if (vap->va_size != VNOVAL) { @@ -480,8 +485,15 @@ return (EROFS); if ((ip->i_flags & SF_SNAPSHOT) != 0) return (EPERM); - if (cred->cr_uid != ip->i_uid && - (error = suser_xxx(cred, p, PRISON_ROOT)) && + /* + * From utimes(2): + * If times is NULL, ... The caller must be the owner of + * the file, have permission to write the file, or be the + * super-user. + * If times is non-NULL, ... The caller must be the owner of + * the file or be the super-user. + */ + if ((error = VOP_ACCESS(vp, VADMIN, cred, p)) && ((vap->va_vaflags & VA_UTIMES_NULL) == 0 || (error = VOP_ACCESS(vp, VWRITE, cred, p)))) return (error); @@ -529,11 +541,17 @@ register struct inode *ip = VTOI(vp); int error; - if (cred->cr_uid != ip->i_uid) { - error = suser_xxx(cred, p, PRISON_ROOT); - if (error) + /* + * To modify the permissions on a file, must possess VADMIN + * for that file. + */ + if ((error = VOP_ACCESS(vp, VADMIN, cred, p))) return (error); - } + /* + * Privileged processes may set the sticky bit on non-directories, + * as well as set the setgid bit on a file with a group that the + * process is not a member of. + */ if (suser_xxx(cred, NULL, PRISON_ROOT)) { if (vp->v_type != VDIR && (mode & S_ISTXT)) return (EFTYPE); @@ -571,12 +589,18 @@ uid = ip->i_uid; if (gid == (gid_t)VNOVAL) gid = ip->i_gid; + /* + * To modify the ownership of a file, must possess VADMIN + * for that file. + */ + if ((error = VOP_ACCESS(vp, VADMIN, cred, p))) + return (error); /* - * If we don't own the file, are trying to change the owner - * of the file, or are not a member of the target group, - * the caller must be superuser or the call fails. + * To change the owner of a file, or change the group of a file + * to a group of which we are not a member, the caller must + * have privilege. */ - if ((cred->cr_uid != ip->i_uid || uid != ip->i_uid || + if ((uid != ip->i_uid || (gid != ip->i_gid && !groupmember(gid, cred))) && (error = suser_xxx(cred, p, PRISON_ROOT))) return (error); @@ -1095,15 +1119,14 @@ if (xp->i_number == ip->i_number) panic("ufs_rename: same file"); /* - * If the parent directory is "sticky", then the user must - * own the parent directory, or the destination of the rename, - * otherwise the destination may not be changed (except by - * root). This implements append-only directories. + * If the parent directory is "sticky", then the caller + * must possess VADMIN for the parent directory, or the + * destination of the rename. This implements append-only + * directories. */ if ((dp->i_mode & S_ISTXT) && - suser_xxx(tcnp->cn_cred, NULL, PRISON_ROOT) && - tcnp->cn_cred->cr_uid != dp->i_uid && - xp->i_uid != tcnp->cn_cred->cr_uid) { + VOP_ACCESS(tdvp, VADMIN, tcnp->cn_cred, p) && + VOP_ACCESS(tvp, VADMIN, tcnp->cn_cred, p)) { error = EPERM; goto bad; } To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-fs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1001009122343.90573I-100000>