From 6a5abb1ee5351d36de3b8589f8bf23fd2dfbb6da Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sun, 2 Feb 2020 16:34:57 +0000 Subject: [PATCH] Provide O_SEARCH O_SEARCH is defined by POSIX [0] to open a directory for searching, skipping permissions checks on the directory itself after the initial open(). This is close to the semantics we've historically applied for O_EXEC on a directory, which is UB according to POSIX. Conveniently, O_SEARCH on a file is also explicitly undefined behavior according to POSIX, so O_EXEC would be a fine choice. The spec goes on to state that O_SEARCH and O_EXEC need not be distinct values, but they're not defined to be the same value. This was pointed out as an incompatibility with other systems that had made its way into libarchive, which had assumed that O_EXEC was an alias for O_SEARCH. This defines compatibility O_SEARCH/FSEARCH (equivalent to O_EXEC and FEXEC respectively) and expands our UB for O_EXEC on a directory. O_EXEC on a directory is checked in vn_open_vnode already, so for completeness we add a NOEXECCHECK when O_SEARCH has been specified on the top-level fd and do not re-check that when descending in namei. [0] https://pubs.opengroup.org/onlinepubs/9699919799/ Reviewed by: kib Differential Revision: https://reviews.freebsd.org/D23247 --- .../netbsd-tests/lib/libc/c063/t_o_search.c | 78 ++++++++++++++++++- lib/libc/sys/open.2 | 23 +++++- lib/libc/tests/c063/Makefile | 3 +- .../opensolaris/uts/common/fs/zfs/zfs_vnops.c | 12 ++- sys/fs/devfs/devfs_vnops.c | 4 +- sys/fs/fuse/fuse_vnops.c | 4 +- sys/fs/nfsclient/nfs_clvnops.c | 3 +- sys/fs/smbfs/smbfs_vnops.c | 3 +- sys/fs/tmpfs/tmpfs_vnops.c | 2 +- sys/kern/vfs_cache.c | 6 +- sys/kern/vfs_lookup.c | 19 ++++- sys/kern/vfs_subr.c | 12 +++ sys/sys/fcntl.h | 2 + sys/sys/namei.h | 3 +- sys/sys/vnode.h | 2 + 15 files changed, 151 insertions(+), 25 deletions(-) diff --git a/contrib/netbsd-tests/lib/libc/c063/t_o_search.c b/contrib/netbsd-tests/lib/libc/c063/t_o_search.c index 10e4607ab1a..0bda9911de5 100644 --- a/contrib/netbsd-tests/lib/libc/c063/t_o_search.c +++ b/contrib/netbsd-tests/lib/libc/c063/t_o_search.c @@ -33,9 +33,10 @@ __RCSID("$NetBSD: t_o_search.c,v 1.5 2017/01/10 22:25:01 christos Exp $"); #include -#include +#include #include +#include #include #include #include @@ -50,7 +51,7 @@ __RCSID("$NetBSD: t_o_search.c,v 1.5 2017/01/10 22:25:01 christos Exp $"); * until a decision is reached about the semantics of O_SEARCH and a * non-broken implementation is available. */ -#if (O_MASK & O_SEARCH) != 0 +#if defined(__FreeBSD__) || (O_MASK & O_SEARCH) != 0 #define USE_O_SEARCH #endif @@ -257,11 +258,79 @@ ATF_TC_BODY(o_search_notdir, tc) int fd; ATF_REQUIRE(mkdir(DIR, 0755) == 0); +#ifndef __FreeBSD__ ATF_REQUIRE((dfd = open(FILE, O_CREAT|O_RDWR|O_SEARCH, 0644)) != -1); +#else + ATF_REQUIRE((dfd = open(FILE, O_CREAT|O_SEARCH, 0644)) != -1); +#endif ATF_REQUIRE((fd = openat(dfd, BASEFILE, O_RDWR, 0)) == -1); ATF_REQUIRE(errno == ENOTDIR); } +#ifdef USE_O_SEARCH +ATF_TC(o_search_nord); +ATF_TC_HEAD(o_search_nord, tc) +{ + atf_tc_set_md_var(tc, "descr", "See that openat succeeds with no read permission"); + atf_tc_set_md_var(tc, "require.user", "unprivileged"); +} +ATF_TC_BODY(o_search_nord, tc) +{ + int dfd, fd; + + ATF_REQUIRE(mkdir(DIR, 0755) == 0); + ATF_REQUIRE((fd = open(FILE, O_CREAT|O_RDWR, 0644)) != -1); + ATF_REQUIRE(close(fd) == 0); + + ATF_REQUIRE(chmod(DIR, 0100) == 0); + ATF_REQUIRE((dfd = open(DIR, O_SEARCH, 0)) != -1); + + ATF_REQUIRE(faccessat(dfd, BASEFILE, W_OK, 0) != -1); + + ATF_REQUIRE(close(dfd) == 0); +} + +ATF_TC(o_search_getdents); +ATF_TC_HEAD(o_search_getdents, tc) +{ + atf_tc_set_md_var(tc, "descr", "See that O_SEARCH forbids getdents"); +} +ATF_TC_BODY(o_search_getdents, tc) +{ + char buf[1024]; + int dfd; + + ATF_REQUIRE(mkdir(DIR, 0755) == 0); + ATF_REQUIRE((dfd = open(DIR, O_SEARCH, 0)) != -1); + ATF_REQUIRE(getdents(dfd, buf, sizeof(buf)) < 0); + ATF_REQUIRE(close(dfd) == 0); +} + +ATF_TC(o_search_revokex); +ATF_TC_HEAD(o_search_revokex, tc) +{ + atf_tc_set_md_var(tc, "descr", "See that *at behaves after chmod -x"); + atf_tc_set_md_var(tc, "require.user", "unprivileged"); +} +ATF_TC_BODY(o_search_revokex, tc) +{ + int dfd, fd; + struct stat sb; + + ATF_REQUIRE(mkdir(DIR, 0755) == 0); + ATF_REQUIRE((fd = open(FILE, O_CREAT|O_RDWR, 0644)) != -1); + ATF_REQUIRE(close(fd) == 0); + + ATF_REQUIRE((dfd = open(DIR, O_SEARCH, 0)) != -1); + + /* Drop permissions. The kernel must still not check the exec bit. */ + ATF_REQUIRE(chmod(DIR, 0000) == 0); + ATF_REQUIRE(fstatat(dfd, BASEFILE, &sb, 0) == 0); + + ATF_REQUIRE(close(dfd) == 0); +} +#endif /* USE_O_SEARCH */ + ATF_TP_ADD_TCS(tp) { @@ -276,6 +345,11 @@ ATF_TP_ADD_TCS(tp) ATF_TP_ADD_TC(tp, o_search_unpriv_flag2); #endif ATF_TP_ADD_TC(tp, o_search_notdir); +#ifdef USE_O_SEARCH + ATF_TP_ADD_TC(tp, o_search_nord); + ATF_TP_ADD_TC(tp, o_search_getdents); + ATF_TP_ADD_TC(tp, o_search_revokex); +#endif return atf_no_error(); } diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2 index 95996230558..59397d76be7 100644 --- a/lib/libc/sys/open.2 +++ b/lib/libc/sys/open.2 @@ -28,7 +28,7 @@ .\" @(#)open.2 8.2 (Berkeley) 11/16/93 .\" $FreeBSD$ .\" -.Dd September 28, 2019 +.Dd January 31, 2020 .Dt OPEN 2 .Os .Sh NAME @@ -166,6 +166,7 @@ O_RDONLY open for reading only O_WRONLY open for writing only O_RDWR open for reading and writing O_EXEC open for execute only +O_SEARCH open for search only, an alias for O_EXEC O_NONBLOCK do not block on open O_APPEND append on each write O_CREAT create file if it does not exist @@ -326,6 +327,19 @@ If the specified path is absolute, allows arbitrary prefix that ends up at the topping directory, after which all further resolved components must be under it. .Pp +When +.Fa fd +is opened with +.Dv O_SEARCH , +execute permissions are checked at open time. +The +.Fa fd +may not be used for any read operations like +.Xr getdirentries 2 . +The primary use for this descriptor will be as the lookup descriptor for the +.Fn *at +family of functions. +.Pp If successful, .Fn open returns a non-negative integer, termed a file descriptor. @@ -518,9 +532,12 @@ An attempt was made to open a descriptor with an illegal combination of .Dv O_RDONLY , .Dv O_WRONLY , -.Dv O_RDWR +or +.Dv O_RDWR , and -.Dv O_EXEC . +.Dv O_EXEC +or +.Dv O_SEARCH . .It Bq Er EBADF The .Fa path diff --git a/lib/libc/tests/c063/Makefile b/lib/libc/tests/c063/Makefile index 05da6ea212a..3be612dcaa4 100644 --- a/lib/libc/tests/c063/Makefile +++ b/lib/libc/tests/c063/Makefile @@ -1,7 +1,5 @@ # $FreeBSD$ -#TODO: t_o_search - NETBSD_ATF_TESTS_C= faccessat_test NETBSD_ATF_TESTS_C+= fchmodat_test NETBSD_ATF_TESTS_C+= fchownat_test @@ -11,6 +9,7 @@ NETBSD_ATF_TESTS_C+= linkat_test NETBSD_ATF_TESTS_C+= mkdirat_test NETBSD_ATF_TESTS_C+= mkfifoat_test NETBSD_ATF_TESTS_C+= mknodat_test +NETBSD_ATF_TESTS_C+= o_search_test NETBSD_ATF_TESTS_C+= openat_test NETBSD_ATF_TESTS_C+= readlinkat_test NETBSD_ATF_TESTS_C+= renameat_test diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c index 1c2856a224b..26cce46bc92 100644 --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c @@ -1543,10 +1543,14 @@ zfs_lookup(vnode_t *dvp, char *nm, vnode_t **vpp, struct componentname *cnp, * Check accessibility of directory. */ if (!cached) { - error = zfs_zaccess(zdp, ACE_EXECUTE, 0, B_FALSE, cr); - if (error != 0) { - ZFS_EXIT(zfsvfs); - return (error); + if ((cnp->cn_flags & NOEXECCHECK) != 0) { + cnp->cn_flags &= ~NOEXECCHECK; + } else { + error = zfs_zaccess(zdp, ACE_EXECUTE, 0, B_FALSE, cr); + if (error != 0) { + ZFS_EXIT(zfsvfs); + return (error); + } } } diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c index 8f60bfc414c..05b30a06302 100644 --- a/sys/fs/devfs/devfs_vnops.c +++ b/sys/fs/devfs/devfs_vnops.c @@ -946,8 +946,8 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_unlock) if ((flags & ISDOTDOT) && (dvp->v_vflag & VV_ROOT)) return (EIO); - error = VOP_ACCESS(dvp, VEXEC, cnp->cn_cred, td); - if (error) + error = vn_dir_check_exec(dvp, cnp); + if (error != 0) return (error); if (cnp->cn_namelen == 1 && *pname == '.') { diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c index f1b0fcbe7bd..05431a77065 100644 --- a/sys/fs/fuse/fuse_vnops.c +++ b/sys/fs/fuse/fuse_vnops.c @@ -1006,7 +1006,9 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) if (islastcn && vfs_isrdonly(mp) && (nameiop != LOOKUP)) return EROFS; - if ((err = fuse_internal_access(dvp, VEXEC, td, cred))) + if ((cnp->cn_flags & NOEXECCHECK) != 0) + cnp->cn_flags &= ~NOEXECCHECK; + else if ((err = fuse_internal_access(dvp, VEXEC, td, cred))) return err; if (flags & ISDOTDOT) { diff --git a/sys/fs/nfsclient/nfs_clvnops.c b/sys/fs/nfsclient/nfs_clvnops.c index 63b1ab76975..9c929e9af3b 100644 --- a/sys/fs/nfsclient/nfs_clvnops.c +++ b/sys/fs/nfsclient/nfs_clvnops.c @@ -1195,7 +1195,8 @@ nfs_lookup(struct vop_lookup_args *ap) } NFSUNLOCKNODE(np); - if ((error = VOP_ACCESS(dvp, VEXEC, cnp->cn_cred, td)) != 0) + error = vn_dir_check_exec(dvp, cnp); + if (error != 0) return (error); error = cache_lookup(dvp, vpp, cnp, &nctime, &ncticks); if (error > 0 && error != ENOENT) diff --git a/sys/fs/smbfs/smbfs_vnops.c b/sys/fs/smbfs/smbfs_vnops.c index 3a0c207f685..026fab8d000 100644 --- a/sys/fs/smbfs/smbfs_vnops.c +++ b/sys/fs/smbfs/smbfs_vnops.c @@ -1199,7 +1199,8 @@ smbfs_lookup(ap) islastcn = flags & ISLASTCN; if (islastcn && (mp->mnt_flag & MNT_RDONLY) && (nameiop != LOOKUP)) return EROFS; - if ((error = VOP_ACCESS(dvp, VEXEC, cnp->cn_cred, td)) != 0) + error = vn_dir_check_exec(dvp, cnp); + if (error != 0) return error; smp = VFSTOSMBFS(mp); dnp = VTOSMB(dvp); diff --git a/sys/fs/tmpfs/tmpfs_vnops.c b/sys/fs/tmpfs/tmpfs_vnops.c index 570c33c3349..088258918d1 100644 --- a/sys/fs/tmpfs/tmpfs_vnops.c +++ b/sys/fs/tmpfs/tmpfs_vnops.c @@ -90,7 +90,7 @@ tmpfs_lookup1(struct vnode *dvp, struct vnode **vpp, struct componentname *cnp) *vpp = NULLVP; /* Check accessibility of requested node as a first step. */ - error = VOP_ACCESS(dvp, VEXEC, cnp->cn_cred, cnp->cn_thread); + error = vn_dir_check_exec(dvp, cnp); if (error != 0) goto out; diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c index fecf749578e..f0026e1be18 100644 --- a/sys/kern/vfs_cache.c +++ b/sys/kern/vfs_cache.c @@ -2141,9 +2141,7 @@ vfs_cache_lookup(struct vop_lookup_args *ap) int error; struct vnode **vpp = ap->a_vpp; struct componentname *cnp = ap->a_cnp; - struct ucred *cred = cnp->cn_cred; int flags = cnp->cn_flags; - struct thread *td = cnp->cn_thread; *vpp = NULL; dvp = ap->a_dvp; @@ -2155,8 +2153,8 @@ vfs_cache_lookup(struct vop_lookup_args *ap) (cnp->cn_nameiop == DELETE || cnp->cn_nameiop == RENAME)) return (EROFS); - error = VOP_ACCESS(dvp, VEXEC, cred, td); - if (error) + error = vn_dir_check_exec(dvp, cnp); + if (error != 0) return (error); error = cache_lookup(dvp, vpp, cnp, NULL, NULL); diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index 265fd724d0f..9e55c142645 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -308,6 +308,7 @@ namei(struct nameidata *ndp) struct vnode *dp; /* the directory we are searching */ struct iovec aiov; /* uio for reading symbolic links */ struct componentname *cnp; + struct file *dfp; struct thread *td; struct proc *p; cap_rights_t rights; @@ -445,10 +446,22 @@ namei(struct nameidata *ndp) AUDIT_ARG_ATFD1(ndp->ni_dirfd); if (cnp->cn_flags & AUDITVNODE2) AUDIT_ARG_ATFD2(ndp->ni_dirfd); - error = fgetvp_rights(td, ndp->ni_dirfd, - &rights, &ndp->ni_filecaps, &dp); - if (error == EINVAL) + /* + * Effectively inlined fgetvp_rights, because we need to + * inspect the file as well as grabbing the vnode. + */ + error = fget_cap_locked(fdp, ndp->ni_dirfd, &rights, + &dfp, &ndp->ni_filecaps); + if (error != 0 || dfp->f_ops == &badfileops || + dfp->f_vnode == NULL) { error = ENOTDIR; + } else { + dp = dfp->f_vnode; + vrefact(dp); + + if ((dfp->f_flag & FSEARCH) != 0) + cnp->cn_flags |= NOEXECCHECK; + } #ifdef CAPABILITIES /* * If file descriptor doesn't have all rights, diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c index 6f9d37d03cc..f9f1f0a541c 100644 --- a/sys/kern/vfs_subr.c +++ b/sys/kern/vfs_subr.c @@ -6377,3 +6377,15 @@ __mnt_vnode_markerfree_lazy(struct vnode **mvp, struct mount *mp) mtx_unlock(&mp->mnt_listmtx); mnt_vnode_markerfree_lazy(mvp, mp); } + +int +vn_dir_check_exec(struct vnode *vp, struct componentname *cnp) +{ + + if ((cnp->cn_flags & NOEXECCHECK) != 0) { + cnp->cn_flags &= ~NOEXECCHECK; + return (0); + } + + return (VOP_ACCESS(vp, VEXEC, cnp->cn_cred, cnp->cn_thread)); +} diff --git a/sys/sys/fcntl.h b/sys/sys/fcntl.h index 55ba4220afe..0febbcec5cd 100644 --- a/sys/sys/fcntl.h +++ b/sys/sys/fcntl.h @@ -119,9 +119,11 @@ typedef __pid_t pid_t; #if __POSIX_VISIBLE >= 200809 #define O_DIRECTORY 0x00020000 /* Fail if not directory */ #define O_EXEC 0x00040000 /* Open for execute only */ +#define O_SEARCH O_EXEC #endif #ifdef _KERNEL #define FEXEC O_EXEC +#define FSEARCH O_SEARCH #endif #if __POSIX_VISIBLE >= 200809 diff --git a/sys/sys/namei.h b/sys/sys/namei.h index bd5198bd936..602d7eff28b 100644 --- a/sys/sys/namei.h +++ b/sys/sys/namei.h @@ -161,7 +161,8 @@ struct nameidata { #define AUDITVNODE2 0x08000000 /* audit the looked up vnode information */ #define TRAILINGSLASH 0x10000000 /* path ended in a slash */ #define NOCAPCHECK 0x20000000 /* do not perform capability checks */ -#define PARAMASK 0x3ffffe00 /* mask of parameter descriptors */ +#define NOEXECCHECK 0x40000000 /* do not perform exec check on dir */ +#define PARAMASK 0x7ffffe00 /* mask of parameter descriptors */ /* * Namei results flags diff --git a/sys/sys/vnode.h b/sys/sys/vnode.h index 809427d00d1..19c4930263d 100644 --- a/sys/sys/vnode.h +++ b/sys/sys/vnode.h @@ -953,6 +953,8 @@ int vn_chown(struct file *fp, uid_t uid, gid_t gid, struct ucred *active_cred, void vn_fsid(struct vnode *vp, struct vattr *va); +int vn_dir_check_exec(struct vnode *vp, struct componentname *cnp); + #define VOP_UNLOCK_FLAGS(vp, flags) ({ \ struct vnode *_vp = (vp); \ int _flags = (flags); \