Re: [RFC PATCH] fuse: Clear SGID bit when setting mode in setacl
On Tue, Mar 2, 2021 at 5:26 PM Vivek Goyal wrote: > > 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
Re: [RFC PATCH] fuse: Clear SGID bit when setting mode in setacl
On Tue, Mar 02, 2021 at 11:00:33AM -0500, Vivek Goyal wrote:
> On Mon, Mar 01, 2021 at 06:20:30PM +, Luis Henriques wrote:
> > On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:
> > > On Fri, Feb 26, 2021 at 06:33:57PM +, 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.
> >
> > 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
>
> 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.
>
>
Re: [RFC PATCH] fuse: Clear SGID bit when setting mode in setacl
On Mon, Mar 01, 2021 at 06:20:30PM +, Luis Henriques wrote:
> On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:
> > On Fri, Feb 26, 2021 at 06:33:57PM +, 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.
>
> 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
Yet-to-be-signed-off-by: Vivek Goyal
---
Re: [RFC PATCH] fuse: Clear SGID bit when setting mode in setacl
On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:
> On Fri, Feb 26, 2021 at 06:33:57PM +, 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.
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
Re: [RFC PATCH] fuse: Clear SGID bit when setting mode in setacl
On Mon, Mar 01, 2021 at 11:33:24AM -0500, Vivek Goyal wrote:
> On Fri, Feb 26, 2021 at 06:33:57PM +, 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.
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
Re: [RFC PATCH] fuse: Clear SGID bit when setting mode in setacl
On Fri, Feb 26, 2021 at 06:33:57PM +, 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
>
> Signed-off-by: Luis Henriques
> ---
> 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, , );
> + 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(, 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, , NULL);
> + dput(entry);
> + }
> + }
> forget_all_cached_acls(inode);
> fuse_invalidate_attr(inode);
>
>
