[RFC PATCH] fuse: Clear SGID bit when setting mode in setacl

Setting file permissions with POSIX ACLs (setxattr) isn't clearing the
setgid bit.  This seems to be CVE-2016-7097, detected by running fstest
generic/375 in virtiofs.  Unfortunately, when the fix for this CVE landed
in the kernel with commit 073931017b49 ("posix_acl: Clear SGID bit when
setting file permissions"), FUSE didn't had ACLs support yet.

Signed-off-by: Luis Henriques <redacted>
---
 fs/fuse/acl.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c
index f529075a2ce8..1b273277c1c9 100644
--- a/fs/fuse/acl.c
+++ b/fs/fuse/acl.c

@@ -54,7 +54,9 @@ int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	const char *name;
+	umode_t mode = inode->i_mode;
 	int ret;
+	bool update_mode = false;
 
 	if (fuse_is_bad(inode))
 		return -EIO;

@@ -62,11 +64,18 @@ int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	if (!fc->posix_acl || fc->no_setxattr)
 		return -EOPNOTSUPP;
 
-	if (type == ACL_TYPE_ACCESS)
+	if (type == ACL_TYPE_ACCESS) {
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
-	else if (type == ACL_TYPE_DEFAULT)
+		if (acl) {
+			ret = posix_acl_update_mode(inode, &mode, &acl);
+			if (ret)
+				return ret;
+			if (inode->i_mode != mode)
+				update_mode = true;
+		}
+	} else if (type == ACL_TYPE_DEFAULT) {
 		name = XATTR_NAME_POSIX_ACL_DEFAULT;
-	else
+	} else
 		return -EINVAL;
 
 	if (acl) {

@@ -98,6 +107,20 @@ int fuse_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	} else {
 		ret = fuse_removexattr(inode, name);
 	}
+	if (!ret && update_mode) {
+		struct dentry *entry;
+		struct iattr attr;
+
+		entry = d_find_alias(inode);
+		if (entry) {
+			memset(&attr, 0, sizeof(attr));
+			attr.ia_valid = ATTR_MODE | ATTR_CTIME;
+			attr.ia_mode = mode;
+			attr.ia_ctime = current_time(inode);
+			ret = fuse_do_setattr(entry, &attr, NULL);
+			dput(entry);
+		}
+	}
 	forget_all_cached_acls(inode);
 	fuse_invalidate_attr(inode);
 

On Fri, Feb 26, 2021 at 06:33:57PM +0000, Luis Henriques wrote:

Setting file permissions with POSIX ACLs (setxattr) isn't clearing the
setgid bit.  This seems to be CVE-2016-7097, detected by running fstest
generic/375 in virtiofs.  Unfortunately, when the fix for this CVE landed
in the kernel with commit 073931017b49 ("posix_acl: Clear SGID bit when
setting file permissions"), FUSE didn't had ACLs support yet.

Hi Luis,

Interesting. I did not know that "chmod" can lead to clearing of SGID
as well. Recently we implemented FUSE_HANDLE_KILLPRIV_V2 flag which
means that file server is responsible for clearing of SUID/SGID/caps
as per following rules.

    - caps are always cleared on chown/write/truncate
    - suid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID.
    - sgid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID as well as file has group execute
      permission.

And we don't have anything about "chmod" in this list. Well, I will test
this and come back to this little later.

I see following comment in fuse_set_acl().

                /*
                 * Fuse userspace is responsible for updating access
                 * permissions in the inode, if needed. fuse_setxattr
                 * invalidates the inode attributes, which will force
                 * them to be refreshed the next time they are used,
                 * and it also updates i_ctime.
                 */

So looks like that original code has been written with intent that
file server is responsible for updating inode permissions. I am
assuming this will include clearing of S_ISGID if needed.

But question is, does file server has enough information to be able
to handle proper clearing of S_ISGID info. IIUC, file server will need
two pieces of information atleast.

- gid of the caller.
- Whether caller has CAP_FSETID or not.

I think we have first piece of information but not the second one. May
be we need to send this in fuse_setxattr_in->flags. And file server
can drop CAP_FSETID while doing setxattr().

What about "gid" info. We don't change to caller's uid/gid while doing
setxattr(). So host might not clear S_ISGID or clear it when it should
not. I am wondering that can we switch to caller's uid/gid in setxattr(),
atleast while setting acls.

Thanks
Vivek

On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:

On Fri, Feb 26, 2021 at 06:33:57PM +0000, Luis Henriques wrote:

quoted

Setting file permissions with POSIX ACLs (setxattr) isn't clearing the
setgid bit.  This seems to be CVE-2016-7097, detected by running fstest
generic/375 in virtiofs.  Unfortunately, when the fix for this CVE landed
in the kernel with commit 073931017b49 ("posix_acl: Clear SGID bit when
setting file permissions"), FUSE didn't had ACLs support yet.

Hi Luis,

Interesting. I did not know that "chmod" can lead to clearing of SGID
as well. Recently we implemented FUSE_HANDLE_KILLPRIV_V2 flag which
means that file server is responsible for clearing of SUID/SGID/caps
as per following rules.

    - caps are always cleared on chown/write/truncate
    - suid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID.
    - sgid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID as well as file has group execute
      permission.

And we don't have anything about "chmod" in this list. Well, I will test
this and come back to this little later.

I see following comment in fuse_set_acl().

                /*
                 * Fuse userspace is responsible for updating access
                 * permissions in the inode, if needed. fuse_setxattr
                 * invalidates the inode attributes, which will force
                 * them to be refreshed the next time they are used,
                 * and it also updates i_ctime.
                 */

So looks like that original code has been written with intent that
file server is responsible for updating inode permissions. I am
assuming this will include clearing of S_ISGID if needed.

But question is, does file server has enough information to be able
to handle proper clearing of S_ISGID info. IIUC, file server will need
two pieces of information atleast.

- gid of the caller.
- Whether caller has CAP_FSETID or not.

I think we have first piece of information but not the second one. May
be we need to send this in fuse_setxattr_in->flags. And file server
can drop CAP_FSETID while doing setxattr().

What about "gid" info. We don't change to caller's uid/gid while doing
setxattr(). So host might not clear S_ISGID or clear it when it should
not. I am wondering that can we switch to caller's uid/gid in setxattr(),
atleast while setting acls.

Thank for looking into this.  To be honest, initially I thought that the
fix should be done in the server too, but when I looked into the code I
couldn't find an easy way to get that done (without modifying the data
being passed from the kernel in setxattr).

So, what I've done was to look at what other filesystems were doing in the
ACL code, and that's where I found out about this CVE.  The CVE fix for
the other filesystems looked easy enough to be included in FUSE too.

Cheers,
--
Luís

On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:

On Fri, Feb 26, 2021 at 06:33:57PM +0000, Luis Henriques wrote:

quoted

Setting file permissions with POSIX ACLs (setxattr) isn't clearing the
setgid bit.  This seems to be CVE-2016-7097, detected by running fstest
generic/375 in virtiofs.  Unfortunately, when the fix for this CVE landed
in the kernel with commit 073931017b49 ("posix_acl: Clear SGID bit when
setting file permissions"), FUSE didn't had ACLs support yet.

Hi Luis,

Interesting. I did not know that "chmod" can lead to clearing of SGID
as well. Recently we implemented FUSE_HANDLE_KILLPRIV_V2 flag which
means that file server is responsible for clearing of SUID/SGID/caps
as per following rules.

    - caps are always cleared on chown/write/truncate
    - suid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID.
    - sgid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID as well as file has group execute
      permission.

And we don't have anything about "chmod" in this list. Well, I will test
this and come back to this little later.

Looks like I did not notice the setattr_prepare() call in
fuse_do_setattr() which clears SGID in client itself and server does not
have to do anything extra. So it works.

IOW, FUSE_HANDLE_KILLPRIV_V2 will not handle this particular case and
fuse client will clear SGID on chmod, if need be.

Vivek

On Mon, Mar 01, 2021 at 06:20:30PM +0000, Luis Henriques wrote:

On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:

quoted

On Fri, Feb 26, 2021 at 06:33:57PM +0000, Luis Henriques wrote:

quoted

Setting file permissions with POSIX ACLs (setxattr) isn't clearing the
setgid bit.  This seems to be CVE-2016-7097, detected by running fstest
generic/375 in virtiofs.  Unfortunately, when the fix for this CVE landed
in the kernel with commit 073931017b49 ("posix_acl: Clear SGID bit when
setting file permissions"), FUSE didn't had ACLs support yet.

Hi Luis,

Interesting. I did not know that "chmod" can lead to clearing of SGID
as well. Recently we implemented FUSE_HANDLE_KILLPRIV_V2 flag which
means that file server is responsible for clearing of SUID/SGID/caps
as per following rules.

    - caps are always cleared on chown/write/truncate
    - suid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID.
    - sgid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID as well as file has group execute
      permission.

And we don't have anything about "chmod" in this list. Well, I will test
this and come back to this little later.

I see following comment in fuse_set_acl().

                /*
                 * Fuse userspace is responsible for updating access
                 * permissions in the inode, if needed. fuse_setxattr
                 * invalidates the inode attributes, which will force
                 * them to be refreshed the next time they are used,
                 * and it also updates i_ctime.
                 */

So looks like that original code has been written with intent that
file server is responsible for updating inode permissions. I am
assuming this will include clearing of S_ISGID if needed.

But question is, does file server has enough information to be able
to handle proper clearing of S_ISGID info. IIUC, file server will need
two pieces of information atleast.

- gid of the caller.
- Whether caller has CAP_FSETID or not.

I think we have first piece of information but not the second one. May
be we need to send this in fuse_setxattr_in->flags. And file server
can drop CAP_FSETID while doing setxattr().

What about "gid" info. We don't change to caller's uid/gid while doing
setxattr(). So host might not clear S_ISGID or clear it when it should
not. I am wondering that can we switch to caller's uid/gid in setxattr(),
atleast while setting acls.

Thank for looking into this.  To be honest, initially I thought that the
fix should be done in the server too, but when I looked into the code I
couldn't find an easy way to get that done (without modifying the data
being passed from the kernel in setxattr).

So, what I've done was to look at what other filesystems were doing in the
ACL code, and that's where I found out about this CVE.  The CVE fix for
the other filesystems looked easy enough to be included in FUSE too.

Hi Luis,

I still feel that it should probably be fixed in virtiofsd, given fuse client
is expecting file server to take care of any change of mode (file
permission bits).

I wrote a proof of concept patch and this should fix this. But it
drop CAP_FSETID always. So I will need to modify kernel to pass
this information to file server and that should properly fix
generic/375. 

Please have a look. This applies on top of fuse acl support V4 patches
I had posted. I have pushed all the patches on a temporary git branch
as well.

https://github.com/rhvgoyal/qemu/commits/acl-sgid

Vivek


Subject: virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

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).

This is a proof of concept patch which switches to caller's uid/gid
and alwasys drops CAP_FSETID in lo_setxattr(system.posix_acl_access).
This should fix the xfstest generic/375 test case.

This patch is not complete yet. Kernel should pass information when
to drop CAP_FSETID and when not to. I will look into modifying
kernel to pass this information to file server.

Reported-by: Luis Henriques <redacted>
Yet-to-be-signed-off-by: Vivek Goyal [off-list ref]
---
 tools/virtiofsd/passthrough_ll.c |   28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Index: rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c
===================================================================

--- rhvgoyal-qemu.orig/tools/virtiofsd/passthrough_ll.c	2021-03-02 08:06:20.539820330 -0500
+++ rhvgoyal-qemu/tools/virtiofsd/passthrough_ll.c	2021-03-02 10:46:40.901334665 -0500

@@ -172,7 +172,7 @@ struct lo_data {
     int user_killpriv_v2, killpriv_v2;
     /* If set, virtiofsd is responsible for setting umask during creation */
     bool change_umask;
-    int user_posix_acl;
+    int user_posix_acl, posix_acl;
 };
 
 static const struct fuse_opt lo_opts[] = {

@@ -677,6 +677,7 @@ static void lo_init(void *userdata, stru
         fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
         conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
         lo->change_umask = true;
+        lo->posix_acl = true;
     } else {
         /* User either did not specify anything or wants it disabled */
         fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");

@@ -2981,12 +2982,37 @@ static void lo_setxattr(fuse_req_t req,
 
     sprintf(procname, "%i", inode->fd);
     if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) {
+        bool switched_creds = false;
+        struct lo_cred old = {};
+
         fd = openat(lo->proc_self_fd, procname, O_RDONLY);
         if (fd < 0) {
             saverr = errno;
             goto out;
         }
+
+        if (lo->posix_acl && !strcmp(name, "system.posix_acl_access")) {
+            ret = lo_change_cred(req, &old, false);
+            if (ret) {
+                saverr = ret;
+                goto out;
+            }
+            ret = drop_effective_cap("FSETID", NULL);
+            if (ret != 0) {
+                lo_restore_cred(&old, false);
+                saverr = ret;
+                goto out;
+            }
+            switched_creds = true;
+        }
+
         ret = fsetxattr(fd, name, value, size, flags);
+
+        if (switched_creds) {
+            if (gain_effective_cap("FSETID"))
+                fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_FSETID\n");
+            lo_restore_cred(&old, false);
+        }
     } else {
         /* fchdir should not fail here */
         assert(fchdir(lo->proc_self_fd) == 0);

On Tue, Mar 02, 2021 at 11:00:33AM -0500, Vivek Goyal wrote:

On Mon, Mar 01, 2021 at 06:20:30PM +0000, Luis Henriques wrote:

quoted

On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:

quoted

On Fri, Feb 26, 2021 at 06:33:57PM +0000, Luis Henriques wrote:

quoted

Setting file permissions with POSIX ACLs (setxattr) isn't clearing the
setgid bit.  This seems to be CVE-2016-7097, detected by running fstest
generic/375 in virtiofs.  Unfortunately, when the fix for this CVE landed
in the kernel with commit 073931017b49 ("posix_acl: Clear SGID bit when
setting file permissions"), FUSE didn't had ACLs support yet.

Hi Luis,

Interesting. I did not know that "chmod" can lead to clearing of SGID
as well. Recently we implemented FUSE_HANDLE_KILLPRIV_V2 flag which
means that file server is responsible for clearing of SUID/SGID/caps
as per following rules.

    - caps are always cleared on chown/write/truncate
    - suid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID.
    - sgid is always cleared on chown, while for truncate/write it is cleared
      only if caller does not have CAP_FSETID as well as file has group execute
      permission.

And we don't have anything about "chmod" in this list. Well, I will test
this and come back to this little later.

I see following comment in fuse_set_acl().

                /*
                 * Fuse userspace is responsible for updating access
                 * permissions in the inode, if needed. fuse_setxattr
                 * invalidates the inode attributes, which will force
                 * them to be refreshed the next time they are used,
                 * and it also updates i_ctime.
                 */

So looks like that original code has been written with intent that
file server is responsible for updating inode permissions. I am
assuming this will include clearing of S_ISGID if needed.

But question is, does file server has enough information to be able
to handle proper clearing of S_ISGID info. IIUC, file server will need
two pieces of information atleast.

- gid of the caller.
- Whether caller has CAP_FSETID or not.

I think we have first piece of information but not the second one. May
be we need to send this in fuse_setxattr_in->flags. And file server
can drop CAP_FSETID while doing setxattr().

What about "gid" info. We don't change to caller's uid/gid while doing
setxattr(). So host might not clear S_ISGID or clear it when it should
not. I am wondering that can we switch to caller's uid/gid in setxattr(),
atleast while setting acls.

Thank for looking into this.  To be honest, initially I thought that the
fix should be done in the server too, but when I looked into the code I
couldn't find an easy way to get that done (without modifying the data
being passed from the kernel in setxattr).

So, what I've done was to look at what other filesystems were doing in the
ACL code, and that's where I found out about this CVE.  The CVE fix for
the other filesystems looked easy enough to be included in FUSE too.

Hi Luis,

I still feel that it should probably be fixed in virtiofsd, given fuse client
is expecting file server to take care of any change of mode (file
permission bits).

Havid said that, there is one disadvantage of relying on server to
do this. Now idmapped mount patches have been merged. If virtiofs
were to ever support idmapped mounts, this will become an issue.
Server does not know about idmapped mounts, and it does not have
information on how to shift inode gid to determine if SGID should
be cleared or not.

So if we were to keep possible future support of idmapped mounts in mind,
then solving it in client makes more sense.  (/me is afraid that there
might be other dependencies like this elsewhere).

Miklos, WDYT.

Thanks
Vivek

On Tue, Mar 2, 2021 at 5:26 PM Vivek Goyal [off-list ref] wrote:

quoted

I still feel that it should probably be fixed in virtiofsd, given fuse client
is expecting file server to take care of any change of mode (file
permission bits).

Havid said that, there is one disadvantage of relying on server to
do this. Now idmapped mount patches have been merged. If virtiofs
were to ever support idmapped mounts, this will become an issue.
Server does not know about idmapped mounts, and it does not have
information on how to shift inode gid to determine if SGID should
be cleared or not.

So if we were to keep possible future support of idmapped mounts in mind,
then solving it in client makes more sense.  (/me is afraid that there
might be other dependencies like this elsewhere).

Miklos, WDYT.

Hmm sounds like two different modes of operation.

1) shared, non-idmapped: need to take care of races, so do the sgid
clearing in the server
2) non-shared, idmapped: can only do it in client

The same applies to all the other FUSE_*_KILL* stuff, so I guess the
decision about the mode just needs to be tied to a flag in some way.
Not sure if an existing one could be used.

Thanks,
Miklos