| Message ID | 20201009181512.65496-6-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: > > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill > suid/sgid/security.capability on open(O_TRUNC), if server supports > FUSE_ATOMIC_O_TRUNC. > > But server needs to kill suid/sgid only if caller does not have > CAP_FSETID. Given server does not have this information, client > needs to send this info to server. > > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells > server to kill suid/sgid(only if group execute is set). This is needed for FUSE_CREATE as well (which may act as a normal open in case the file exists, and no O_EXCL was specified), right? I can edit the patch, if you agree. Thanks, Miklos
On Fri, Nov 06, 2020 at 02:55:11PM +0100, Miklos Szeredi wrote: > On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill > > suid/sgid/security.capability on open(O_TRUNC), if server supports > > FUSE_ATOMIC_O_TRUNC. > > > > But server needs to kill suid/sgid only if caller does not have > > CAP_FSETID. Given server does not have this information, client > > needs to send this info to server. > > > > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells > > server to kill suid/sgid(only if group execute is set). > > This is needed for FUSE_CREATE as well (which may act as a normal open > in case the file exists, and no O_EXCL was specified), right? Hi Miklos, IIUC, In current code we seem to use FUSE_CREATE only if file does not exist. If file exists, then we probably will take FUSE_OPEN path. Are you concerned about future proofing where somebody decides to use FUSE_CREATE for create + open on a file which exists. If yes, I agree that patching FUSE_CREATE makes sense. > > I can edit the patch, if you agree. Please do. Thanks Vivek
On Fri, Nov 6, 2020 at 5:00 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Fri, Nov 06, 2020 at 02:55:11PM +0100, Miklos Szeredi wrote: > > On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill > > > suid/sgid/security.capability on open(O_TRUNC), if server supports > > > FUSE_ATOMIC_O_TRUNC. > > > > > > But server needs to kill suid/sgid only if caller does not have > > > CAP_FSETID. Given server does not have this information, client > > > needs to send this info to server. > > > > > > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells > > > server to kill suid/sgid(only if group execute is set). > > > > This is needed for FUSE_CREATE as well (which may act as a normal open > > in case the file exists, and no O_EXCL was specified), right? > > Hi Miklos, > > IIUC, In current code we seem to use FUSE_CREATE only if file does not exist. > If file exists, then we probably will take FUSE_OPEN path. That's true if the cache is up to date, one important point for FUSE_CREATE is that it works atomically even if the cache is stale. So if cache is negative and we send a FUSE_CREATE it may still open an *existing* file, and we want to do suid/caps clearing in that case also, no? Thanks, Miklos
On Fri, Nov 06, 2020 at 05:33:00PM +0100, Miklos Szeredi wrote: > On Fri, Nov 6, 2020 at 5:00 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > On Fri, Nov 06, 2020 at 02:55:11PM +0100, Miklos Szeredi wrote: > > > On Fri, Oct 9, 2020 at 8:16 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill > > > > suid/sgid/security.capability on open(O_TRUNC), if server supports > > > > FUSE_ATOMIC_O_TRUNC. > > > > > > > > But server needs to kill suid/sgid only if caller does not have > > > > CAP_FSETID. Given server does not have this information, client > > > > needs to send this info to server. > > > > > > > > So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells > > > > server to kill suid/sgid(only if group execute is set). > > > > > > This is needed for FUSE_CREATE as well (which may act as a normal open > > > in case the file exists, and no O_EXCL was specified), right? > > > > Hi Miklos, > > > > IIUC, In current code we seem to use FUSE_CREATE only if file does not exist. > > If file exists, then we probably will take FUSE_OPEN path. > > That's true if the cache is up to date, one important point for > FUSE_CREATE is that it works atomically even if the cache is stale. > So if cache is negative and we send a FUSE_CREATE it may still open an > *existing* file, and we want to do suid/caps clearing in that case > also, no? Yes, makes sense. This can happen in a race condition also where fuse_lookup_name() gets a negative dentry and then another client creates file (with setuid/setgid/caps) set. Now fuse_create_open() is called without O_EXCL and in that case we should remove setuid/setgid/caps as needed. So yes, please make modifications accordingly for FUSE_CREATE. If you want me to do make those changes, please let me know. Thanks Vivek
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index ee1bb9bfdcd5..5400c6d77701 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -42,6 +42,11 @@ static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file, inarg.flags = file->f_flags & ~(O_CREAT | O_EXCL | O_NOCTTY); if (!fc->atomic_o_trunc) inarg.flags &= ~O_TRUNC; + + if (fc->handle_killpriv_v2 && (inarg.flags & O_TRUNC) && + !capable(CAP_FSETID)) + inarg.open_flags |= FUSE_OPEN_KILL_PRIV; + args.opcode = opcode; args.nodeid = nodeid; args.in_numargs = 1; diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index 7b8da0a2de0d..e20b3ee9d292 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -173,6 +173,7 @@ * - add FUSE_SETUPMAPPING and FUSE_REMOVEMAPPING * - add map_alignment to fuse_init_out, add FUSE_MAP_ALIGNMENT flag * - add FUSE_HANDLE_KILLPRIV_V2 + * - add FUSE_OPEN_KILL_PRIV */ #ifndef _LINUX_FUSE_H @@ -427,6 +428,13 @@ struct fuse_file_lock { */ #define FUSE_FSYNC_FDATASYNC (1 << 0) +/** + * Open flags + * FUSE_OPEN_KILL_PRIV: Kill suid/sgid/security.capability. sgid is cleared + * only if file has group execute permission. + */ +#define FUSE_OPEN_KILL_PRIV (1 << 0) + enum fuse_opcode { FUSE_LOOKUP = 1, FUSE_FORGET = 2, /* no reply */ @@ -588,7 +596,7 @@ struct fuse_setattr_in { struct fuse_open_in { uint32_t flags; - uint32_t unused; + uint32_t open_flags; }; struct fuse_create_in {
With FUSE_HANDLE_KILLPRIV_V2 support, server will need to kill suid/sgid/security.capability on open(O_TRUNC), if server supports FUSE_ATOMIC_O_TRUNC. But server needs to kill suid/sgid only if caller does not have CAP_FSETID. Given server does not have this information, client needs to send this info to server. So add a flag FUSE_OPEN_KILL_PRIV to fuse_open_in request which tells server to kill suid/sgid(only if group execute is set). Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/fuse/file.c | 5 +++++ include/uapi/linux/fuse.h | 10 +++++++++- 2 files changed, 14 insertions(+), 1 deletion(-)