[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 15/19] virtiofsd: Switch creds, drop FSETID for system.posix_acl_a
From: |
Dr. David Alan Gilbert (git) |
Subject: |
[PULL 15/19] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr |
Date: |
Mon, 5 Jul 2021 11:02:31 +0100 |
From: Vivek Goyal <vgoyal@redhat.com>
When posix access acls are set on a file, it can lead to adjusting file
permissions (mode) as well. If caller does not have CAP_FSETID and it
also does not have membership of owner group, this will lead to clearing
SGID bit in mode.
Current fuse code is written in such a way that it expects file server
to take care of chaning file mode (permission), if there is a need.
Right now, host kernel does not clear SGID bit because virtiofsd is
running as root and has CAP_FSETID. For host kernel to clear SGID,
virtiofsd need to switch to gid of caller in guest and also drop
CAP_FSETID (if caller did not have it to begin with).
If SGID needs to be cleared, client will set the flag
FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
should kill sgid.
Currently just switch to uid/gid of the caller and drop CAP_FSETID
and that should do it.
This should fix the xfstest generic/375 test case.
We don't have to switch uid for this to work. That could be one optimization
that pass a parameter to lo_change_cred() to only switch gid and not uid.
Also this will not work whenever (if ever) we support idmapped mounts. In
that case it is possible that uid/gid in request are 0/0 but still we
need to clear SGID. So we will have to pick a non-root sgid and switch
to that instead. That's an TODO item for future when idmapped mount
support is introduced.
This patch only adds the capability to switch creds and drop FSETID
when acl xattr is set. This does not take affect yet. It can take
affect when next patch adds the capability to enable posix_acl.
Reported-by: Luis Henriques <lhenriques@suse.de>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Message-Id: <20210622150852.1507204-7-vgoyal@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++
1 file changed, 75 insertions(+)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 65b2c6fd74..6e30fd9113 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -175,6 +175,7 @@ struct lo_data {
int user_killpriv_v2, killpriv_v2;
/* If set, virtiofsd is responsible for setting umask during creation */
bool change_umask;
+ int posix_acl;
};
static const struct fuse_opt lo_opts[] = {
@@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool
restore_umask)
umask(old->umask);
}
+/*
+ * A helper to change cred and drop capability. Returns 0 on success and
+ * errno on error
+ */
+static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old,
+ bool change_umask, const char *cap_name,
+ bool *cap_dropped)
+{
+ int ret;
+ bool __cap_dropped;
+
+ assert(cap_name);
+
+ ret = drop_effective_cap(cap_name, &__cap_dropped);
+ if (ret) {
+ return ret;
+ }
+
+ ret = lo_change_cred(req, old, change_umask);
+ if (ret) {
+ if (__cap_dropped) {
+ if (gain_effective_cap(cap_name)) {
+ fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
+ }
+ }
+ }
+
+ if (cap_dropped) {
+ *cap_dropped = __cap_dropped;
+ }
+ return ret;
+}
+
+static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask,
+ const char *cap_name)
+{
+ assert(cap_name);
+
+ lo_restore_cred(old, restore_umask);
+
+ if (gain_effective_cap(cap_name)) {
+ fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name);
+ }
+}
+
static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
const char *name, mode_t mode, dev_t rdev,
const char *link)
@@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
ssize_t ret;
int saverr;
int fd = -1;
+ bool switched_creds = false;
+ bool cap_fsetid_dropped = false;
+ struct lo_cred old = {};
mapped_name = NULL;
name = in_name;
@@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
", name=%s value=%s size=%zd)\n", ino, name, value, size);
sprintf(procname, "%i", inode->fd);
+ /*
+ * If we are setting posix access acl and if SGID needs to be
+ * cleared, then switch to caller's gid and drop CAP_FSETID
+ * and that should make sure host kernel clears SGID.
+ *
+ * This probably will not work when we support idmapped mounts.
+ * In that case we will need to find a non-root gid and switch
+ * to it. (Instead of gid in request). Fix it when we support
+ * idmapped mounts.
+ */
+ if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")
+ && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) {
+ ret = lo_drop_cap_change_cred(req, &old, false, "FSETID",
+ &cap_fsetid_dropped);
+ if (ret) {
+ saverr = ret;
+ goto out;
+ }
+ switched_creds = true;
+ }
if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
fd = openat(lo->proc_self_fd, procname, O_RDONLY);
if (fd < 0) {
@@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino,
const char *in_name,
saverr = ret == -1 ? errno : 0;
FCHDIR_NOFAIL(lo->root.fd);
}
+ if (switched_creds) {
+ if (cap_fsetid_dropped)
+ lo_restore_cred_gain_cap(&old, false, "FSETID");
+ else
+ lo_restore_cred(&old, false);
+ }
out:
if (fd >= 0) {
--
2.31.1
- [PULL 06/19] migration: failover: continue to wait card unplug on error, (continued)
- [PULL 06/19] migration: failover: continue to wait card unplug on error, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 05/19] migration: move wait-unplug loop to its own function, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 07/19] virtiofsd: use GDateTime for formatting timestamp for debug messages, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 08/19] docs: describe the security considerations with virtiofsd xattr mapping, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 09/19] virtiofsd: Don't allow file creation with FUSE_OPEN, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 10/19] virtiofsd: Fix fuse setxattr() API change issue, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 11/19] virtiofsd: Fix xattr operations overwriting errno, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 12/19] virtiofsd: Add support for extended setxattr, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 13/19] virtiofsd: Add umask to seccom allow list, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 14/19] virtiofsd: Add capability to change/restore umask, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 15/19] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr,
Dr. David Alan Gilbert (git) <=
- [PULL 16/19] virtiofsd: Add an option to enable/disable posix acls, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 17/19] tests/migration: parse the thread-id key of CpuInfoFast, Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 18/19] tests/migration: fix "downtime_limit" type when "migrate-set-parameters", Dr. David Alan Gilbert (git), 2021/07/05
- [PULL 19/19] migration/rdma: Use error_report to suppress errno message, Dr. David Alan Gilbert (git), 2021/07/05
- Re: [PULL v2 00/19] migration queue, Peter Maydell, 2021/07/05