[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr |
Date: |
Mon, 28 Jun 2021 18:37:27 +0100 |
User-agent: |
Mutt/2.0.7 (2021-05-04) |
* Vivek Goyal (vgoyal@redhat.com) wrote:
> 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>
Reviewed-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 0c9084ea15..113c725def 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.25.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
- Re: [PATCH v7 3/7] virtiofsd: Add support for extended setxattr, (continued)
[PATCH v7 4/7] virtiofsd: Add umask to seccom allow list, Vivek Goyal, 2021/06/22
[PATCH v7 1/7] virtiofsd: Fix fuse setxattr() API change issue, Vivek Goyal, 2021/06/22
[PATCH v7 7/7] virtiofsd: Add an option to enable/disable posix acls, Vivek Goyal, 2021/06/22
[PATCH v7 6/7] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr, Vivek Goyal, 2021/06/22
[PATCH v7 2/7] virtiofsd: Fix xattr operations overwriting errno, Vivek Goyal, 2021/06/22
[PATCH v7 5/7] virtiofsd: Add capability to change/restore umask, Vivek Goyal, 2021/06/22