Date: Mon, 6 May 2019 16:17:56 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r347189 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201905061617.x46GHu8P009148@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Mon May 6 16:17:55 2019 New Revision: 347189 URL: https://svnweb.freebsd.org/changeset/base/347189 Log: fusefs: clear SUID & SGID after a successful write by a non-owner Reported by: pjdfstest Sponsored by: The FreeBSD Foundation Modified: projects/fuse2/sys/fs/fuse/fuse_internal.c projects/fuse2/sys/fs/fuse/fuse_internal.h projects/fuse2/sys/fs/fuse/fuse_io.c projects/fuse2/sys/fs/fuse/fuse_vnops.c projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc projects/fuse2/tests/sys/fs/fusefs/default_permissions_privileged.cc Modified: projects/fuse2/sys/fs/fuse/fuse_internal.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_internal.c Mon May 6 16:17:38 2019 (r347188) +++ projects/fuse2/sys/fs/fuse/fuse_internal.c Mon May 6 16:17:55 2019 (r347189) @@ -740,6 +740,114 @@ fuse_internal_send_init(struct fuse_data *data, struct fdisp_destroy(&fdi); } +/* + * Send a FUSE_SETATTR operation with no permissions checks. If cred is NULL, + * send the request with root credentials + */ +int fuse_internal_setattr(struct vnode *vp, struct vattr *vap, + struct thread *td, struct ucred *cred) +{ + struct fuse_dispatcher fdi; + struct fuse_setattr_in *fsai; + struct mount *mp; + pid_t pid = td->td_proc->p_pid; + struct fuse_data *data; + int dataflags; + int err = 0; + enum vtype vtyp; + int sizechanged = -1; + uint64_t newsize = 0; + + mp = vnode_mount(vp); + data = fuse_get_mpdata(mp); + dataflags = data->dataflags; + + fdisp_init(&fdi, sizeof(*fsai)); + fdisp_make_vp(&fdi, FUSE_SETATTR, vp, td, cred); + if (!cred) { + fdi.finh->uid = 0; + fdi.finh->gid = 0; + } + fsai = fdi.indata; + fsai->valid = 0; + + if (vap->va_uid != (uid_t)VNOVAL) { + fsai->uid = vap->va_uid; + fsai->valid |= FATTR_UID; + } + if (vap->va_gid != (gid_t)VNOVAL) { + fsai->gid = vap->va_gid; + fsai->valid |= FATTR_GID; + } + if (vap->va_size != VNOVAL) { + struct fuse_filehandle *fufh = NULL; + + /*Truncate to a new value. */ + fsai->size = vap->va_size; + sizechanged = 1; + newsize = vap->va_size; + fsai->valid |= FATTR_SIZE; + + fuse_filehandle_getrw(vp, FWRITE, &fufh, cred, pid); + if (fufh) { + fsai->fh = fufh->fh_id; + fsai->valid |= FATTR_FH; + } + } + if (vap->va_atime.tv_sec != VNOVAL) { + fsai->atime = vap->va_atime.tv_sec; + fsai->atimensec = vap->va_atime.tv_nsec; + fsai->valid |= FATTR_ATIME; + } + if (vap->va_mtime.tv_sec != VNOVAL) { + fsai->mtime = vap->va_mtime.tv_sec; + fsai->mtimensec = vap->va_mtime.tv_nsec; + fsai->valid |= FATTR_MTIME; + } + if (vap->va_mode != (mode_t)VNOVAL) { + fsai->mode = vap->va_mode & ALLPERMS; + fsai->valid |= FATTR_MODE; + } + if (!fsai->valid) { + goto out; + } + + if ((err = fdisp_wait_answ(&fdi))) + goto out; + vtyp = IFTOVT(((struct fuse_attr_out *)fdi.answ)->attr.mode); + + if (vnode_vtype(vp) != vtyp) { + if (vnode_vtype(vp) == VNON && vtyp != VNON) { + SDT_PROBE2(fusefs, , internal, trace, 1, "FUSE: Dang! " + "vnode_vtype is VNON and vtype isn't."); + } else { + /* + * STALE vnode, ditch + * + * The vnode has changed its type "behind our back". + * There's nothing really we can do, so let us just + * force an internal revocation and tell the caller to + * try again, if interested. + */ + fuse_internal_vnode_disappear(vp); + err = EAGAIN; + } + } + if (err == 0) { + struct fuse_attr_out *fao = (struct fuse_attr_out*)fdi.answ; + fuse_internal_cache_attrs(vp, &fao->attr, fao->attr_valid, + fao->attr_valid_nsec, NULL); + } + +out: + fdisp_destroy(&fdi); + if (!err && sizechanged) { + fuse_vnode_setsize(vp, cred, newsize); + VTOFUD(vp)->flag &= ~FN_SIZECHANGE; + } + return err; +} + #ifdef ZERO_PAD_INCOMPLETE_BUFS static int isbzero(void *buf, size_t len) Modified: projects/fuse2/sys/fs/fuse/fuse_internal.h ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_internal.h Mon May 6 16:17:38 2019 (r347188) +++ projects/fuse2/sys/fs/fuse/fuse_internal.h Mon May 6 16:17:55 2019 (r347189) @@ -247,6 +247,10 @@ int fuse_internal_rename(struct vnode *fdvp, struct co void fuse_internal_vnode_disappear(struct vnode *vp); +/* setattr */ +int fuse_internal_setattr(struct vnode *vp, struct vattr *va, + struct thread *td, struct ucred *cred); + /* strategy */ /* entity creation */ Modified: projects/fuse2/sys/fs/fuse/fuse_io.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_io.c Mon May 6 16:17:38 2019 (r347188) +++ projects/fuse2/sys/fs/fuse/fuse_io.c Mon May 6 16:17:55 2019 (r347189) @@ -72,6 +72,7 @@ __FBSDID("$FreeBSD$"); #include <sys/sx.h> #include <sys/mutex.h> #include <sys/rwlock.h> +#include <sys/priv.h> #include <sys/proc.h> #include <sys/mount.h> #include <sys/vnode.h> @@ -106,6 +107,9 @@ SDT_PROVIDER_DECLARE(fusefs); */ SDT_PROBE_DEFINE2(fusefs, , io, trace, "int", "char*"); +static void +fuse_io_clear_suid_on_write(struct vnode *vp, struct ucred *cred, + struct thread *td); static int fuse_read_directbackend(struct vnode *vp, struct uio *uio, struct ucred *cred, struct fuse_filehandle *fufh); @@ -119,6 +123,43 @@ static int fuse_write_biobackend(struct vnode *vp, struct uio *uio, struct ucred *cred, struct fuse_filehandle *fufh, int ioflag, pid_t pid); +/* + * FreeBSD clears the SUID and SGID bits on any write by a non-root user. + */ +static void +fuse_io_clear_suid_on_write(struct vnode *vp, struct ucred *cred, + struct thread *td) +{ + struct fuse_data *data; + struct mount *mp; + struct vattr va; + int dataflags; + + mp = vnode_mount(vp); + data = fuse_get_mpdata(mp); + dataflags = data->dataflags; + + if (dataflags & FSESS_DEFAULT_PERMISSIONS) { + if (priv_check_cred(cred, PRIV_VFS_RETAINSUGID)) { + fuse_internal_getattr(vp, &va, cred, td); + if (va.va_mode & (S_ISUID | S_ISGID)) { + mode_t mode = va.va_mode & ~(S_ISUID | S_ISGID); + /* Clear all vattr fields except mode */ + vattr_null(&va); + va.va_mode = mode; + + /* + * Ignore fuse_internal_setattr's return value, + * because at this point the write operation has + * already succeeded and we don't want to return + * failing status for that. + */ + (void)fuse_internal_setattr(vp, &va, td, NULL); + } + } + } +} + SDT_PROBE_DEFINE5(fusefs, , io, io_dispatch, "struct vnode*", "struct uio*", "int", "struct ucred*", "struct fuse_filehandle*"); int @@ -194,6 +235,7 @@ fuse_io_dispatch(struct vnode *vp, struct uio *uio, in err = fuse_write_biobackend(vp, uio, cred, fufh, ioflag, pid); } + fuse_io_clear_suid_on_write(vp, cred, uio->uio_td); break; default: panic("uninterpreted mode passed to fuse_io_dispatch"); @@ -810,6 +852,9 @@ fuse_io_strategy(struct vnode *vp, struct buf *bp) bp->b_ioflags |= BIO_ERROR; bp->b_flags |= B_INVAL; bp->b_error = error; + } else { + fuse_io_clear_suid_on_write(vp, cred, + uio.uio_td); } bp->b_dirtyoff = bp->b_dirtyend = 0; } Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_vnops.c Mon May 6 16:17:38 2019 (r347188) +++ projects/fuse2/sys/fs/fuse/fuse_vnops.c Mon May 6 16:17:55 2019 (r347189) @@ -1516,44 +1516,33 @@ fuse_vnop_setattr(struct vop_setattr_args *ap) struct vattr *vap = ap->a_vap; struct ucred *cred = ap->a_cred; struct thread *td = curthread; - struct fuse_dispatcher fdi; - struct fuse_setattr_in *fsai; struct mount *mp; - pid_t pid = td->td_proc->p_pid; struct fuse_data *data; int dataflags; int err = 0; - enum vtype vtyp; - int sizechanged = 0; - uint64_t newsize = 0; accmode_t accmode = 0; + bool checkperm; mp = vnode_mount(vp); data = fuse_get_mpdata(mp); dataflags = data->dataflags; + checkperm = dataflags & FSESS_DEFAULT_PERMISSIONS; if (fuse_isdeadfs(vp)) { return ENXIO; } - fdisp_init(&fdi, sizeof(*fsai)); - fdisp_make_vp(&fdi, FUSE_SETATTR, vp, td, cred); - fsai = fdi.indata; - fsai->valid = 0; if (vap->va_uid != (uid_t)VNOVAL) { - if (dataflags & FSESS_DEFAULT_PERMISSIONS) { + if (checkperm) { /* Only root may change a file's owner */ err = priv_check_cred(cred, PRIV_VFS_CHOWN); if (err) - goto out; + return err;; } - fsai->uid = vap->va_uid; - fsai->valid |= FATTR_UID; accmode |= VADMIN; } if (vap->va_gid != (gid_t)VNOVAL) { - if (dataflags & FSESS_DEFAULT_PERMISSIONS && - !groupmember(vap->va_gid, cred)) + if (checkperm && !groupmember(vap->va_gid, cred)) { /* * Non-root users may only chgrp to one of their own @@ -1561,112 +1550,56 @@ fuse_vnop_setattr(struct vop_setattr_args *ap) */ err = priv_check_cred(cred, PRIV_VFS_CHOWN); if (err) - goto out; + return err; } - fsai->gid = vap->va_gid; - fsai->valid |= FATTR_GID; accmode |= VADMIN; } if (vap->va_size != VNOVAL) { - - struct fuse_filehandle *fufh = NULL; - - /*Truncate to a new value. */ - fsai->size = vap->va_size; - sizechanged = 1; - newsize = vap->va_size; - fsai->valid |= FATTR_SIZE; - accmode |= VWRITE; - - fuse_filehandle_getrw(vp, FWRITE, &fufh, cred, pid); - if (fufh) { - fsai->fh = fufh->fh_id; - fsai->valid |= FATTR_FH; + switch (vp->v_type) { + case VDIR: + return (EISDIR); + case VLNK: + case VREG: + if (vfs_isrdonly(mp)) + return (EROFS); + break; + default: + /* + * According to POSIX, the result is unspecified + * for file types other than regular files, + * directories and shared memory objects. We + * don't support shared memory objects in the file + * system, and have dubious support for truncating + * symlinks. Just ignore the request in other cases. + */ + return (0); } + accmode |= VWRITE; } - if (vap->va_atime.tv_sec != VNOVAL) { - fsai->atime = vap->va_atime.tv_sec; - fsai->atimensec = vap->va_atime.tv_nsec; - fsai->valid |= FATTR_ATIME; + /* + * TODO: for atime and mtime, only require VWRITE if UTIMENS_NULL is + * set. PR 237181 + */ + if (vap->va_atime.tv_sec != VNOVAL) accmode |= VADMIN; - /* - * TODO: only require VWRITE if UTIMENS_NULL is set. PR 237181 - */ - } - if (vap->va_mtime.tv_sec != VNOVAL) { - fsai->mtime = vap->va_mtime.tv_sec; - fsai->mtimensec = vap->va_mtime.tv_nsec; - fsai->valid |= FATTR_MTIME; + if (vap->va_mtime.tv_sec != VNOVAL) accmode |= VADMIN; - /* - * TODO: only require VWRITE if UTIMENS_NULL is set. PR 237181 - */ - } if (vap->va_mode != (mode_t)VNOVAL) { /* Only root may set the sticky bit on non-directories */ - if (dataflags & FSESS_DEFAULT_PERMISSIONS && - vp->v_type != VDIR && (vap->va_mode & S_ISTXT)) - { - if (priv_check_cred(cred, PRIV_VFS_STICKYFILE)) { - err = EFTYPE; - goto out; - } - } - fsai->mode = vap->va_mode & ALLPERMS; - fsai->valid |= FATTR_MODE; + if (checkperm && vp->v_type != VDIR && (vap->va_mode & S_ISTXT) + && priv_check_cred(cred, PRIV_VFS_STICKYFILE)) + return EFTYPE; accmode |= VADMIN; } - if (!fsai->valid) { - goto out; - } - vtyp = vnode_vtype(vp); - if (fsai->valid & FATTR_SIZE && vtyp == VDIR) { - err = EISDIR; - goto out; - } - if (vfs_isrdonly(vnode_mount(vp))) { - err = EROFS; - goto out; - } + if (vfs_isrdonly(mp)) + return EROFS; + err = fuse_internal_access(vp, accmode, td, cred); if (err) - goto out; - - if ((err = fdisp_wait_answ(&fdi))) - goto out; - vtyp = IFTOVT(((struct fuse_attr_out *)fdi.answ)->attr.mode); - - if (vnode_vtype(vp) != vtyp) { - if (vnode_vtype(vp) == VNON && vtyp != VNON) { - SDT_PROBE2(fusefs, , vnops, trace, 1, "FUSE: Dang! " - "vnode_vtype is VNON and vtype isn't."); - } else { - /* - * STALE vnode, ditch - * - * The vnode has changed its type "behind our back". - * There's nothing really we can do, so let us just - * force an internal revocation and tell the caller to - * try again, if interested. - */ - fuse_internal_vnode_disappear(vp); - err = EAGAIN; - } - } - if (err == 0) { - struct fuse_attr_out *fao = (struct fuse_attr_out*)fdi.answ; - fuse_internal_cache_attrs(vp, &fao->attr, fao->attr_valid, - fao->attr_valid_nsec, NULL); - } - -out: - fdisp_destroy(&fdi); - if (!err && sizechanged) { - fuse_vnode_setsize(vp, cred, newsize); - VTOFUD(vp)->flag &= ~FN_SIZECHANGE; - } - return err; + return err; + else + return fuse_internal_setattr(vp, vap, td, cred); } /* Modified: projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc Mon May 6 16:17:38 2019 (r347188) +++ projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc Mon May 6 16:17:55 2019 (r347189) @@ -68,6 +68,24 @@ virtual void SetUp() { } public: +void expect_chmod(uint64_t ino, mode_t mode) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_SETATTR && + in->header.nodeid == ino && + in->body.setattr.valid == FATTR_MODE && + in->body.setattr.mode == mode); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) { + SET_OUT_HEADER_LEN(out, attr); + out->body.attr.attr.ino = ino; // Must match nodeid + out->body.attr.attr.mode = S_IFREG | mode; + out->body.attr.attr_valid = UINT64_MAX; + }))); +} + void expect_getattr(uint64_t ino, mode_t mode, uint64_t attr_valid, int times, uid_t uid = 0, gid_t gid = 0) { @@ -104,6 +122,7 @@ class Lookup: public DefaultPermissions {}; class Open: public DefaultPermissions {}; class Setattr: public DefaultPermissions {}; class Unlink: public DefaultPermissions {}; +class Write: public DefaultPermissions {}; /* * Test permission handling during create, mkdir, mknod, link, symlink, and @@ -934,4 +953,56 @@ TEST_F(Unlink, sticky_directory) ASSERT_EQ(-1, unlink(FULLPATH)); ASSERT_EQ(EPERM, errno); +} + +/* A write by a non-owner should clear a file's SUID bit */ +TEST_F(Write, clear_suid) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + struct stat sb; + uint64_t ino = 42; + mode_t oldmode = 04777; + mode_t newmode = 0777; + char wbuf[1] = {'x'}; + int fd; + + expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1); + expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX); + expect_open(ino, 0, 1); + expect_write(ino, 0, sizeof(wbuf), sizeof(wbuf), 0, wbuf); + expect_chmod(ino, newmode); + + fd = open(FULLPATH, O_WRONLY); + ASSERT_LE(0, fd) << strerror(errno); + ASSERT_EQ(1, write(fd, wbuf, sizeof(wbuf))) << strerror(errno); + ASSERT_EQ(0, fstat(fd, &sb)) << strerror(errno); + EXPECT_EQ(S_IFREG | newmode, sb.st_mode); + /* Deliberately leak fd. close(2) will be tested in release.cc */ +} + +/* A write by a non-owner should clear a file's SGID bit */ +TEST_F(Write, clear_sgid) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + struct stat sb; + uint64_t ino = 42; + mode_t oldmode = 02777; + mode_t newmode = 0777; + char wbuf[1] = {'x'}; + int fd; + + expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1); + expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX); + expect_open(ino, 0, 1); + expect_write(ino, 0, sizeof(wbuf), sizeof(wbuf), 0, wbuf); + expect_chmod(ino, newmode); + + fd = open(FULLPATH, O_WRONLY); + ASSERT_LE(0, fd) << strerror(errno); + ASSERT_EQ(1, write(fd, wbuf, sizeof(wbuf))) << strerror(errno); + ASSERT_EQ(0, fstat(fd, &sb)) << strerror(errno); + EXPECT_EQ(S_IFREG | newmode, sb.st_mode); + /* Deliberately leak fd. close(2) will be tested in release.cc */ } Modified: projects/fuse2/tests/sys/fs/fusefs/default_permissions_privileged.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/default_permissions_privileged.cc Mon May 6 16:17:38 2019 (r347188) +++ projects/fuse2/tests/sys/fs/fusefs/default_permissions_privileged.cc Mon May 6 16:17:55 2019 (r347189) @@ -98,7 +98,7 @@ void expect_lookup(const char *relpath, uint64_t ino, class Setattr: public DefaultPermissionsPrivileged {}; -TEST_F(Setattr, sticky_regular_file_eftype) +TEST_F(Setattr, sticky_regular_file) { const char FULLPATH[] = "mountpoint/some_file.txt"; const char RELPATH[] = "some_file.txt";
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201905061617.x46GHu8P009148>