| Message ID | 20201009181512.65496-4-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Implement FUSE_HANDLE_KILLPRIV_V2 and enable SB_NOSEC | expand |
On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > If fc->handle_killpriv_v2 is enabled, we expect file server to clear > suid/sgid/security.capbility upon chown/truncate/write as appropriate. > > Upon truncate (ATTR_SIZE), suid/sgid is cleared only if caller does > not have CAP_FSETID. File server does not know whether caller has > CAP_FSETID or not. Hence set FATTR_KILL_PRIV upon truncate to let > file server know that caller does not have CAP_FSETID and it should > kill suid/sgid as appropriate. > > We don't have to send this information for chown (ATTR_UID/ATTR_GID) > as that always clears suid/sgid irrespective of capabilities of > calling process. I'm undecided on this. Would it hurt to set it on chown? That might make the logic in some servers simpler, no? What would be the drawback of setting FATTR_KILL_PRIV for chown as well? Thanks, Miklos
On Fri, Nov 06, 2020 at 03:39:29PM +0100, Miklos Szeredi wrote: > On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > If fc->handle_killpriv_v2 is enabled, we expect file server to clear > > suid/sgid/security.capbility upon chown/truncate/write as appropriate. > > > > Upon truncate (ATTR_SIZE), suid/sgid is cleared only if caller does > > not have CAP_FSETID. File server does not know whether caller has > > CAP_FSETID or not. Hence set FATTR_KILL_PRIV upon truncate to let > > file server know that caller does not have CAP_FSETID and it should > > kill suid/sgid as appropriate. > > > > We don't have to send this information for chown (ATTR_UID/ATTR_GID) > > as that always clears suid/sgid irrespective of capabilities of > > calling process. > > I'm undecided on this. Would it hurt to set it on chown? That > might make the logic in some servers simpler, no? > > What would be the drawback of setting FATTR_KILL_PRIV for chown as well? Hi Miklos, Thinking loud. So these are the rules we expect from VFS point of view. - 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. From server point of view, these rules become. - caps are always cleared on chown/write/truncate - suid is always cleared on chown, while for truncate/write it is cleared only if client set appropriate flag. - For truncate, this flag will either be FUSE_OPEN_KILL_PRIV or FATTR_KILL_PRIV. - For write, FUSE_WRITE_KILL_PRIV will be set. - sgid is always cleared on chown, while for truncate/write it is cleared only if caller has set a flag as well as file has group execute permission. - For truncate, this flag will either be FUSE_OPEN_KILL_PRIV or FATTR_KILL_PRIV. - For write, FUSE_WRITE_KILL_PRIV will be set. Above rules assumes that chown() will always clear caps/suid/sgid and server does not have to rely on any flags. I think it does not hurt to start passing FATTR_KILL_PRIV for chown() as well. In that case, server will always clear caps on chown but clear suid/sgid only if FATTR_KILL_PRIV is set. (Which will always be set). So anything is fine. We just need to document it well. I think I will write it very clearly in qemu patch depending on what goes in kernel. Thanks Vivek
On Fri, Nov 6, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote: > I think it does not hurt to start passing FATTR_KILL_PRIV for chown() > as well. In that case, server will always clear caps on chown but > clear suid/sgid only if FATTR_KILL_PRIV is set. (Which will always > be set). Okay. More thoughts for FUSE_HANDLE_KILLPRIV_V2: - clear "security.capability" on write, truncate and chown unconditionally - clear suid/sgid if o setattr has FATTR_SIZE and FATTR_KILL_PRIV o setattr has FATTR_UID or FATTR_GID o open has O_TRUNC and FUSE_OPEN_KILL_PRIV o write has FUSE_WRITE_KILL_PRIV Kernel has: ATTR_KILL_PRIV -> clear "security.capability" ATTR_KILL_SUID -> clear S_ISUID ATTR_KILL_SGID -> clear S_ISGID if executable Fuse has: FUSE_*KILL_PRIV -> clear S_ISUID and S_ISGID if executable So the fuse meaning of FUSE_*KILL_PRIV has a complementary meaning to that of ATTR_KILL_PRIV, which is somewhat confusing. Also "PRIV" implies all privileges, including "security.capability" but the fuse ones relate to suid/sgid only. How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias for FUSE_WRITE_KILL_PRIV)? Thanks, Miklos > > So anything is fine. We just need to document it well. I think I will > write it very clearly in qemu patch depending on what goes in kernel. > > Thanks > Vivek >
On Wed, Nov 11, 2020 at 2:54 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias > for FUSE_WRITE_KILL_PRIV)? Series pushed to #for-next with these changes. Please take a look and test. Thanks, Miklos
On Wed, Nov 11, 2020 at 02:54:43PM +0100, Miklos Szeredi wrote: > On Fri, Nov 6, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > I think it does not hurt to start passing FATTR_KILL_PRIV for chown() > > as well. In that case, server will always clear caps on chown but > > clear suid/sgid only if FATTR_KILL_PRIV is set. (Which will always > > be set). > > Okay. > > More thoughts for FUSE_HANDLE_KILLPRIV_V2: > > - clear "security.capability" on write, truncate and chown unconditionally > - clear suid/sgid if > o setattr has FATTR_SIZE and FATTR_KILL_PRIV > o setattr has FATTR_UID or FATTR_GID > o open has O_TRUNC and FUSE_OPEN_KILL_PRIV > o write has FUSE_WRITE_KILL_PRIV > > Kernel has: > ATTR_KILL_PRIV -> clear "security.capability" > ATTR_KILL_SUID -> clear S_ISUID > ATTR_KILL_SGID -> clear S_ISGID if executable > > Fuse has: > FUSE_*KILL_PRIV -> clear S_ISUID and S_ISGID if executable > > So the fuse meaning of FUSE_*KILL_PRIV has a complementary meaning to > that of ATTR_KILL_PRIV, which is somewhat confusing. Also "PRIV" > implies all privileges, including "security.capability" but the fuse > ones relate to suid/sgid only. > > How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias > for FUSE_WRITE_KILL_PRIV)? Hi Miklos, Renaming FUSE_*KILL_PRIV to FUSE_*KILL_SUIDSGID sounds good. For a breif moment I was also thinking that these FUSE_*KILL_PRIV and and ATTR_KILL_PRIV are not exactly mapping. Glad you caught it and made the situation better. Thanks Vivek
On Wed, Nov 11, 2020 at 05:24:22PM +0100, Miklos Szeredi wrote: > On Wed, Nov 11, 2020 at 2:54 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > > How about FUSE_*KILL_SUIDGID (FUSE_WRITE_KILL_SUIDGID being an alias > > for FUSE_WRITE_KILL_PRIV)? > > Series pushed to #for-next with these changes. Please take a look and test. Thanks Miklos. I looked at the patches quickly and they look good. I am now fixing qemu virtiofsd side and will test. Thanks Vivek
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index c4a01290aec6..ecdb7895c156 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -1575,6 +1575,8 @@ int fuse_do_setattr(struct dentry *dentry, struct iattr *attr, /* For mandatory locking in truncate */ inarg.valid |= FATTR_LOCKOWNER; inarg.lock_owner = fuse_lock_owner_id(fc, current->files); + if (fc->handle_killpriv_v2 && !capable(CAP_FSETID)) + inarg.valid |= FATTR_KILL_PRIV; } fuse_setattr_fill(fc, &args, inode, &inarg, &outarg); err = fuse_simple_request(fc, &args); diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 3ae3f222a0ed..7b8da0a2de0d 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -269,6 +269,7 @@ struct fuse_file_lock { #define FATTR_MTIME_NOW (1 << 8) #define FATTR_LOCKOWNER (1 << 9) #define FATTR_CTIME (1 << 10) +#define FATTR_KILL_PRIV (1 << 14) /* Matches ATTR_KILL_PRIV */ /** * Flags returned by the OPEN request
If fc->handle_killpriv_v2 is enabled, we expect file server to clear suid/sgid/security.capbility upon chown/truncate/write as appropriate. Upon truncate (ATTR_SIZE), suid/sgid is cleared only if caller does not have CAP_FSETID. File server does not know whether caller has CAP_FSETID or not. Hence set FATTR_KILL_PRIV upon truncate to let file server know that caller does not have CAP_FSETID and it should kill suid/sgid as appropriate. We don't have to send this information for chown (ATTR_UID/ATTR_GID) as that always clears suid/sgid irrespective of capabilities of calling process. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/fuse/dir.c | 2 ++ include/uapi/linux/fuse.h | 1 + 2 files changed, 3 insertions(+)