Re: [PATCH v2] fuse: Rearrange fuse_allow_current_process checks

[Seth Forshee <sforshee@kernel.org>]

On Sat, Oct 22, 2022 at 03:22:13PM +0200, Christian Brauner wrote:
> On Fri, Oct 21, 2022 at 09:05:26AM -0700, Andrii Nakryiko wrote:
> > On Thu, Oct 20, 2022 at 1:14 PM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> > >
> > > This is a followup to a previous commit of mine [0], which added the
> > > allow_sys_admin_access && capable(CAP_SYS_ADMIN) check. This patch
> > > rearranges the order of checks in fuse_allow_current_process without
> > > changing functionality.
> > >
> > > [0] added allow_sys_admin_access && capable(CAP_SYS_ADMIN) check to the
> > > beginning of the function, with the reasoning that
> > > allow_sys_admin_access should be an 'escape hatch' for users with
> > > CAP_SYS_ADMIN, allowing them to skip any subsequent checks.
> > >
> > > However, placing this new check first results in many capable() calls when
> > > allow_sys_admin_access is set, where another check would've also
> > > returned 1. This can be problematic when a BPF program is tracing
> > > capable() calls.
> > >
> > > At Meta we ran into such a scenario recently. On a host where
> > > allow_sys_admin_access is set but most of the FUSE access is from
> > > processes which would pass other checks - i.e. they don't need
> > > CAP_SYS_ADMIN 'escape hatch' - this results in an unnecessary capable()
> > > call for each fs op. We also have a daemon tracing capable() with BPF and
> > > doing some data collection, so tracing these extraneous capable() calls
> > > has the potential to regress performance for an application doing many
> > > FUSE ops.
> > >
> > > So rearrange the order of these checks such that CAP_SYS_ADMIN 'escape
> > > hatch' is checked last. Previously, if allow_other is set on the
> > > fuse_conn, uid/gid checking doesn't happen as current_in_userns result
> > > is returned. It's necessary to add a 'goto' here to skip past uid/gid
> > > check to maintain those semantics here.
> > >
> > >   [0]: commit 9ccf47b26b73 ("fuse: Add module param for CAP_SYS_ADMIN access bypassing allow_other")
> > >
> > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> > > Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > ---
> > 
> > LGTM!
> > 
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > 
> > 
> > But I would also be curious to hear from Miklos or others whether
> > skipping uid/gid check if fc->allow_other is true wa an intentional
> > logic, oversight, or just doesn't matter. Because doing:
> 
> Originally, setting fc->allow_other granted access to everyone skipping
> all further permission checks.
> 
> When Seth (Cced) made it possible to mount fuse in userns
> fc->allow_other needed to be restricted to callers who are in the
> superblock's userns or a descendant of it. The reason is that an
> unprivileged user can mount a fuse filesystem with fc->allow_other
> turned on. But then the user could mess with processes outside its
> userns when they access the filesystem.
> 
> Without fc->allow_other it doesn't matter what userns the filesystem is
> accessed from as long as the uidgid is permissible. This could e.g.,
> happen if you unshare a userns but dont set{g,u}id.
> 
> In general, I see two obvious permission models. In the first model we
> restrict access to the owner of the mount by uidgid but also require
> callers to be within the userns hierarchy. If allow_other is specified
> the requirement would be relaxed to only require the caller to be within
> the userns hierarchy. That would make the permission checking
> consistent.
> 
> The second permission model would only require permissible uidgid by
> default and for allow other either permissible uidgid or for the caller
> to be within the userns hierarchy (Which is what you're asking about
> afaiu.).
> 
> I don't see any obvious reasons why this wouldn't work from a security
> perspective; maybe Seth has additional insights. The problem however is
> that it would be a non-trivial change for containers to lift the
> restriction now. It is quite useful to be able to mount a fuse
> filesystem with allow_other and not have it accessible by anything
> outside your userns hierarchy. So I wouldn't like to see that changed.

I think both of these are defensible models from a security perspective.
But I don't have a good idea of what container runtimes have come to
expect. Either of these would represent a change which could potentially
break assumptions of something in userspace.

Seth

> If so we should probably make it yet another fuse module option or sm.
> But in any case that should be a separate patch with a proper
> justification why this semantic change is needed other than that it
> simplifies the logic.
>