d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fusefs: various cleanups

Browse Source 
* Eliminate fuse_access_param.  Whatever it was supposed to do, it seems
  like it was never complete.  The only real function it ever seems to have
  had was a minor performance optimization, which I've already eliminated.
* Make extended attribute operations obey the allow_other mount option.
* Allow unprivileged access to the SYSTEM extattr namespace when
  -o default_permissions is not in use.
* Disallow setextattr and deleteextattr on read-only mounts.
* Add tests for a few more error cases.

Sponsored by:	The FreeBSD Foundation
This commit is contained in:
Alan Somers 2019-04-10 21:10:21 +00:00
parent ff4fbdf548
commit 666f8543bb
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/projects/fuse2/; revision=346101
7 changed files with 180 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
sys/fs/fuse/fuse_internal.c
View File

@ -110,7 +110,6 @@ static int isbzero(void *buf, size_t len);
int
fuse_internal_access(struct vnode *vp,
accmode_t mode,
struct fuse_access_param *facp,
struct thread *td,
struct ucred *cred)
{
@ -143,13 +142,9 @@ fuse_internal_access(struct vnode *vp,
}
/* Unless explicitly permitted, deny everyone except the fs owner. */
if (!(facp->facc_flags)) {
if (!(dataflags & FSESS_DAEMON_CAN_SPY)) {
int denied = fuse_match_cred(data->daemoncred, cred);
if (denied)
return EPERM;
}
if (!(dataflags & FSESS_DAEMON_CAN_SPY)) {
if (fuse_match_cred(data->daemoncred, cred))
return EPERM;
}
if (dataflags & FSESS_DEFAULT_PERMISSIONS) {

23
sys/fs/fuse/fuse_internal.h
View File

@ -197,27 +197,6 @@ fuse_validity_2_timespec(const struct fuse_entry_out *feo,
/* access */
#define FVP_ACCESS_NOOP 0x01
#define FACCESS_VA_VALID 0x01
/*
* Caller must be the directory's owner, or the superuser, or the sticky bit
* must not be set
*/
#define FACCESS_STICKY 0x04
/* Caller requires access to change file's owner */
#define FACCESS_CHOWN 0x08
#define FACCESS_SETGID 0x12
#define FACCESS_XQUERIES (FACCESS_STICKY | FACCESS_CHOWN | FACCESS_SETGID)
struct fuse_access_param {
uid_t xuid;
gid_t xgid;
uint32_t facc_flags;
};
static inline int
fuse_match_cred(struct ucred *basecred, struct ucred *usercred)
{
@ -233,7 +212,7 @@ fuse_match_cred(struct ucred *basecred, struct ucred *usercred)
}
int fuse_internal_access(struct vnode *vp, accmode_t mode,
struct fuse_access_param *facp, struct thread *td, struct ucred *cred);
struct thread *td, struct ucred *cred);
/* attributes */
void fuse_internal_cache_attrs(struct vnode *vp, struct fuse_attr *attr,

91
sys/fs/fuse/fuse_vnops.c
View File

@ -211,6 +211,37 @@ uma_zone_t fuse_pbuf_zone;
#define fuse_vm_page_lock_queues() ((void)0)
#define fuse_vm_page_unlock_queues() ((void)0)
/* Check permission for extattr operations, much like extattr_check_cred */
static int
fuse_extattr_check_cred(struct vnode *vp, int ns, struct ucred *cred,
struct thread *td, accmode_t accmode)
{
struct mount *mp = vnode_mount(vp);
struct fuse_data *data = fuse_get_mpdata(mp);
/*
* Kernel-invoked always succeeds.
*/
if (cred == NOCRED)
return (0);
/*
* Do not allow privileged processes in jail to directly manipulate
* system attributes.
*/
switch (ns) {
case EXTATTR_NAMESPACE_SYSTEM:
if (data->dataflags & FSESS_DEFAULT_PERMISSIONS) {
return (priv_check_cred(cred, PRIV_VFS_EXTATTR_SYSTEM));
}
/* FALLTHROUGH */
case EXTATTR_NAMESPACE_USER:
return (fuse_internal_access(vp, accmode, td, cred));
default:
return (EPERM);
}
}
/* Get a filehandle for a directory */
static int
fuse_filehandle_get_dir(struct vnode *vp, struct fuse_filehandle **fufhp,
@ -272,7 +303,6 @@ fuse_vnop_access(struct vop_access_args *ap)
int accmode = ap->a_accmode;
struct ucred *cred = ap->a_cred;
struct fuse_access_param facp;
struct fuse_data *data = fuse_get_mpdata(vnode_mount(vp));
int err;
@ -295,9 +325,8 @@ fuse_vnop_access(struct vop_access_args *ap)
if (vnode_islnk(vp)) {
return 0;
}
bzero(&facp, sizeof(facp));
err = fuse_internal_access(vp, accmode, &facp, ap->a_td, ap->a_cred);
err = fuse_internal_access(vp, accmode, ap->a_td, ap->a_cred);
return err;
}
@ -711,7 +740,6 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
struct fuse_attr *fattr = NULL;
uint64_t nid;
struct fuse_access_param facp;
if (fuse_isdeadfs(dvp)) {
*vpp = NULL;
@ -730,9 +758,9 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
* See further comments at further access checks.
*/
bzero(&facp, sizeof(facp));
/* TODO: consider eliminating this. Is there any good reason for it? */
if (vnode_isvroot(dvp)) { /* early permission check hack */
if ((err = fuse_internal_access(dvp, VEXEC, &facp, td, cred))) {
if ((err = fuse_internal_access(dvp, VEXEC, td, cred))) {
return err;
}
}
@ -831,8 +859,7 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
if (lookup_err) {
/* Entry not found */
if ((nameiop == CREATE || nameiop == RENAME) && islastcn) {
err = fuse_internal_access(dvp, VWRITE, &facp, td,
cred);
err = fuse_internal_access(dvp, VWRITE, td, cred);
if (err)
goto out;
@ -861,11 +888,8 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
fattr = &(feo->attr);
}
/* TODO: check for ENOTDIR */
if ((nameiop == DELETE || nameiop == RENAME) && islastcn) {
err = fuse_internal_access(dvp, VWRITE, &facp,
td, cred);
err = fuse_internal_access(dvp, VWRITE, td, cred);
if (err != 0)
goto out;
/*
@ -878,10 +902,8 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
struct vattr dvattr;
fuse_internal_getattr(dvp, &dvattr, cred, td);
if ((dvattr.va_mode & S_ISTXT) &&
fuse_internal_access(dvp, VADMIN, &facp,
cnp->cn_thread, cnp->cn_cred) &&
fuse_internal_access(*vpp, VADMIN, &facp,
cnp->cn_thread, cnp->cn_cred)) {
fuse_internal_access(dvp, VADMIN, td, cred) &&
fuse_internal_access(*vpp, VADMIN, td, cred)) {
err = EPERM;
goto out;
}
@ -933,10 +955,6 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
if (err) {
goto out;
}
/*err = fuse_lookup_check(dvp, vp, &facp, td, cred,*/
/*nameiop, islastcn);*/
/*if (err)*/
/*goto out;*/
*vpp = vp;
/*
* Save the name for use in VOP_RENAME later.
@ -1088,15 +1106,12 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
*/
int tmpvtype = vnode_vtype(*vpp);
bzero(&facp, sizeof(facp));
/*the early perm check hack */
facp.facc_flags |= FACCESS_VA_VALID;
if ((tmpvtype != VDIR) && (tmpvtype != VLNK)) {
err = ENOTDIR;
}
if (!err && !vnode_mountedhere(*vpp)) {
err = fuse_internal_access(*vpp, VEXEC, &facp, td, cred);
err = fuse_internal_access(*vpp, VEXEC,
td, cred);
}
if (err) {
if (tmpvtype == VLNK)
@ -1528,7 +1543,6 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
struct thread *td = curthread;
struct fuse_dispatcher fdi;
struct fuse_setattr_in *fsai;
struct fuse_access_param facp;
pid_t pid = td->td_proc->p_pid;
int err = 0;
@ -1545,19 +1559,12 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
fsai = fdi.indata;
fsai->valid = 0;
bzero(&facp, sizeof(facp));
facp.xuid = vap->va_uid;
facp.xgid = vap->va_gid;
if (vap->va_uid != (uid_t)VNOVAL) {
facp.facc_flags |= FACCESS_CHOWN;
fsai->uid = vap->va_uid;
fsai->valid |= FATTR_UID;
accmode |= VADMIN;
}
if (vap->va_gid != (gid_t)VNOVAL) {
facp.facc_flags |= FACCESS_CHOWN;
fsai->gid = vap->va_gid;
fsai->valid |= FATTR_GID;
accmode |= VADMIN;
@ -1615,7 +1622,7 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
err = EROFS;
goto out;
}
err = fuse_internal_access(vp, accmode, &facp, td, cred);
err = fuse_internal_access(vp, accmode, td, cred);
if (err)
goto out;
@ -2028,7 +2035,7 @@ fuse_vnop_getextattr(struct vop_getextattr_args *ap)
if (fuse_isdeadfs(vp))
return (ENXIO);
err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD);
err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD);
if (err)
return err;
@ -2109,11 +2116,15 @@ fuse_vnop_setextattr(struct vop_setextattr_args *ap)
if (fuse_isdeadfs(vp))
return (ENXIO);
if (vfs_isrdonly(mp))
return EROFS;
/* Deleting xattrs must use VOP_DELETEEXTATTR instead */
if (ap->a_uio == NULL)
return (EINVAL);
err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VWRITE);
err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td,
VWRITE);
if (err)
return err;
@ -2244,7 +2255,7 @@ fuse_vnop_listextattr(struct vop_listextattr_args *ap)
if (fuse_isdeadfs(vp))
return (ENXIO);
err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD);
err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VREAD);
if (err)
return err;
@ -2352,7 +2363,11 @@ fuse_vnop_deleteextattr(struct vop_deleteextattr_args *ap)
if (fuse_isdeadfs(vp))
return (ENXIO);
err = extattr_check_cred(vp, ap->a_attrnamespace, cred, td, VWRITE);
if (vfs_isrdonly(mp))
return EROFS;
err = fuse_extattr_check_cred(vp, ap->a_attrnamespace, cred, td,
VWRITE);
if (err)
return err;

47
tests/sys/fs/fusefs/allow_other.cc
View File

@ -35,6 +35,7 @@
extern "C" {
#include <sys/types.h>
#include <sys/extattr.h>
#include <fcntl.h>
#include <unistd.h>
}
@ -222,3 +223,49 @@ TEST_F(NoAllowOther, disallowed_beneath_root)
}
);
}
/*
* Provide coverage for the extattr methods, which have a slightly different
* code path
*/
TEST_F(NoAllowOther, setextattr)
{
int ino = 42;
fork(true, [&] {
EXPECT_LOOKUP(1, RELPATH)
.WillOnce(Invoke(
ReturnImmediate([=](auto in __unused, auto out) {
SET_OUT_HEADER_LEN(out, entry);
out->body.entry.attr_valid = UINT64_MAX;
out->body.entry.entry_valid = UINT64_MAX;
out->body.entry.attr.mode = S_IFREG | 0644;
out->body.entry.nodeid = ino;
})));
/*
* lookup the file to get it into the cache.
* Otherwise, the unprivileged lookup will fail with
* EACCES
*/
ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno);
}, [&]() {
const char value[] = "whatever";
ssize_t value_len = strlen(value) + 1;
int ns = EXTATTR_NAMESPACE_USER;
ssize_t r;
r = extattr_set_file(FULLPATH, ns, "foo",
(void*)value, value_len);
if (r >= 0) {
fprintf(stderr, "should've failed\n");
return(1);
} else if (errno != EPERM) {
fprintf(stderr, "Unexpected error: %s\n",
strerror(errno));
return(1);
}
return 0;
}
);
}

19
tests/sys/fs/fusefs/lookup.cc
View File

@ -144,7 +144,22 @@ TEST_F(Lookup, enoent)
EXPECT_EQ(ENOENT, errno);
}
//TODO: test ENOTDIR
TEST_F(Lookup, enotdir)
{
const char FULLPATH[] = "mountpoint/not_a_dir/some_file.txt";
const char RELPATH[] = "not_a_dir";
EXPECT_LOOKUP(1, RELPATH)
.WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) {
SET_OUT_HEADER_LEN(out, entry);
out->body.entry.entry_valid = UINT64_MAX;
out->body.entry.attr.mode = S_IFREG | 0644;
out->body.entry.nodeid = 42;
})));
ASSERT_EQ(-1, access(FULLPATH, F_OK));
ASSERT_EQ(ENOTDIR, errno);
}
/*
* If lookup returns a non-zero entry timeout, then subsequent VOP_LOOKUPs
@ -291,5 +306,3 @@ TEST_F(Lookup, subdir)
*/
ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno);
}

29
tests/sys/fs/fusefs/setattr.cc
View File

@ -41,6 +41,14 @@ using namespace testing;
class Setattr : public FuseTest {};
class RofsSetattr: public Setattr {
public:
virtual void SetUp() {
m_ro = true;
Setattr::SetUp();
}
};
/*
* If setattr returns a non-zero cache timeout, then subsequent VOP_GETATTRs
@ -103,7 +111,6 @@ TEST_F(Setattr, chmod)
SET_OUT_HEADER_LEN(out, entry);
out->body.entry.attr.mode = S_IFREG | oldmode;
out->body.entry.nodeid = ino;
out->body.entry.attr.mode = S_IFREG | oldmode;
})));
EXPECT_CALL(*m_mock, process(
@ -554,4 +561,22 @@ TEST_F(Setattr, utimensat_mtime_only) {
<< strerror(errno);
}
// TODO: test for erofs
/* On a read-only mount, no attributes may be changed */
TEST_F(RofsSetattr, erofs)
{
const char FULLPATH[] = "mountpoint/some_file.txt";
const char RELPATH[] = "some_file.txt";
const uint64_t ino = 42;
const mode_t oldmode = 0755;
const mode_t newmode = 0644;
EXPECT_LOOKUP(1, RELPATH)
.WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) {
SET_OUT_HEADER_LEN(out, entry);
out->body.entry.attr.mode = S_IFREG | oldmode;
out->body.entry.nodeid = ino;
})));
ASSERT_EQ(-1, chmod(FULLPATH, newmode));
ASSERT_EQ(EROFS, errno);
}

33
tests/sys/fs/fusefs/xattr.cc
View File

@ -108,6 +108,13 @@ class Getxattr: public Xattr {};
class Listxattr: public Xattr {};
class Removexattr: public Xattr {};
class Setxattr: public Xattr {};
class RofsXattr: public Xattr {
public:
virtual void SetUp() {
m_ro = true;
Xattr::SetUp();
}
};
/*
* If the extended attribute does not exist on this file, the daemon should
@ -604,4 +611,30 @@ TEST_F(Setxattr, system)
ASSERT_EQ(value_len, r) << strerror(errno);
}
TEST_F(RofsXattr, deleteextattr_erofs)
{
uint64_t ino = 42;
int ns = EXTATTR_NAMESPACE_USER;
expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1);
ASSERT_EQ(-1, extattr_delete_file(FULLPATH, ns, "foo"));
ASSERT_EQ(EROFS, errno);
}
TEST_F(RofsXattr, setextattr_erofs)
{
uint64_t ino = 42;
const char value[] = "whatever";
ssize_t value_len = strlen(value) + 1;
int ns = EXTATTR_NAMESPACE_USER;
ssize_t r;
expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1);
r = extattr_set_file(FULLPATH, ns, "foo", (void*)value, value_len);
ASSERT_EQ(-1, r);
EXPECT_EQ(EROFS, errno);
}
// TODO: EROFS tests