Re: [PATCH] fuse: allow CAP_SYS_ADMIN in root userns to access allow_other mount

[Christian Brauner <brauner@kernel.org>]

On Tue, May 17, 2022 at 12:50:32PM -0400, Dave Marchevsky wrote:
> On 11/15/21 10:28 AM, Miklos Szeredi wrote:   
> > On Sat, 13 Nov 2021 at 00:29, Dave Marchevsky <davemarchevsky@fb.com> wrote:
> > 
> >>> If your tracing daemon runs in init_user_ns with CAP_SYS_ADMIN why can't
> >>> it simply use a helper process/thread to
> >>> setns(userns_fd/pidfd, CLONE_NEWUSER)
> >>> to the target userns? This way we don't need to special-case
> >>> init_user_ns at all.
> >>
> >> helper process + setns could work for my usecase. But the fact that there's no
> >> way to say "I know what I am about to do is potentially stupid and dangerous,
> >> but I am root so let me do it", without spawning a helper process in this case,
> >> feels like it'll result in special-case userspace workarounds for anyone doing
> >> symbolication of backtraces.
> > 
> > Note: any mechanism that grants filesystem access to users that have
> > higher privileges than the daemon serving the filesystem will
> > potentially open DoS attacks against the higher privilege task.  This
> > would be somewhat mitigated if the filesystem is only mounted in a
> > private mount namespace, but AFAICS that's not guaranteed.
> > 
> > The above obviously applies to your original patch but it also applies
> > to any other mechanism where the high privilege user doesn't
> > explicitly acknowledge and accept the consequences.   IOW granting the
> > exception has to be initiated by the high privleged user.
> > 
> > Thanks,
> > Miklos
> > 
> 
> Sorry to ressurect this old thread. My proposed alternate approach of "special
> ioctl to grant exception to descendant userns check" proved unnecessarily
> complex: ioctls also go through fuse_allow_current_process check, so a special
> carve-out would be necessary for in both ioctl and fuse_permission check in
> order to make it possible for non-descendant-userns user to opt in to exception.
> 
> How about a version of this patch with CAP_DAC_READ_SEARCH check? This way 
> there's more of a clear opt-in vs CAP_SYS_ADMIN.

I still think this isn't needed given that especially for the use-cases
listed here you have a workable userspace solution to this problem.

If the CAP_SYS_ADMIN/CAP_DAC_READ_SEARCH check were really just about
giving a privileged task access then it'd be fine imho. But given that
this means the privileged task is open to a DoS attack it seems we're
building a trap into the fuse code.

The setns() model has the advantage that this forces the task to assume
the correct privileges and also serves as an explicit opt-in. Just my 2
cents here.

> 
> FWIW we've been running CAP_SYS_ADMIN version of this patch internally and
> can confirm it fixes tracing tools' ability to symbolicate binaries in FUSE.
> 
> > 
> >>
> >> e.g. perf will have to add some logic: "did I fail
> >> to grab this exe that some process had mapped? Is it in a FUSE mounted by some
> >> descendant userns? let's fork a helper process..." Not the end of the world,
> >> but unnecessary complexity nonetheless.
> >>
> >> That being said, I agree that this patch's special-casing of init_user_ns is
> >> hacky. What do you think about a more explicit and general "let me do this
> >> stupid and dangerous thing" mechanism - perhaps a new struct fuse_conn field
> >> containing a set of exception userns', populated with ioctl or similar.
> > 
> > 
> > 
> >>
> >>>
> >>>>
> >>>> Note: I was unsure whether CAP_SYS_ADMIN or CAP_SYS_PTRACE was the best
> >>>> choice of capability here. Went with the former as it's checked
> >>>> elsewhere in fs/fuse while CAP_SYS_PTRACE isn't.
> >>>>
> >>>>  fs/fuse/dir.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> >>>> index 0654bfedcbb0..2524eeb0f35d 100644
> >>>> --- a/fs/fuse/dir.c
> >>>> +++ b/fs/fuse/dir.c
> >>>> @@ -1134,7 +1134,7 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> >>>>      const struct cred *cred;
> >>>>
> >>>>      if (fc->allow_other)
> >>>> -            return current_in_userns(fc->user_ns);
> >>>> +            return current_in_userns(fc->user_ns) || capable(CAP_SYS_ADMIN);
> >>>>
> >>>>      cred = current_cred();
> >>>>      if (uid_eq(cred->euid, fc->user_id) &&
> >>>> --
> >>>> 2.30.2
> >>>>
> >>>>
> >>