| Message ID | 20210226183357.28467-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] fuse: Clear SGID bit when setting mode in setacl | expand |
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 > > Signed-off-by: Luis Henriques <lhenriques@suse.de> > --- > 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 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: > > 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: > > 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: > > 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. > > 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 <lhenriques@suse.de> Yet-to-be-signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- 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: > > 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: > > > > 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. > > 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 <lhenriques@suse.de> > Yet-to-be-signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > 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 2, 2021 at 5:26 PM Vivek Goyal <vgoyal@redhat.com> 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
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);
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 <lhenriques@suse.de> --- fs/fuse/acl.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)