Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Miklos Szeredi <miklos@szeredi.hu>]

On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
>
>
>
> On 6/7/22 1:47 AM, Christian Brauner wrote:
> > On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:

[...]

> >> +static bool __read_mostly allow_other_parent_userns;
> >> +module_param(allow_other_parent_userns, bool, 0644);
> >> +MODULE_PARM_DESC(allow_other_parent_userns,
> >> + "Allow users not in mounting or descendant userns "
> >> + "to access FUSE with allow_other set");
> >
> > The name of the parameter also suggests that access is granted to parent
> > userns tasks whereas the change seems to me to allows every task access
> > to that fuse filesystem independent of what userns they are in.
> >
> > So even a task in a sibling userns could - probably with rather
> > elaborate mount propagation trickery - access that fuse filesystem.
> >
> > AFaict, either the module parameter is misnamed or the patch doesn't
> > implement the behavior expressed in the name.
> >
> > The original patch restricted access to a CAP_SYS_ADMIN capable task.
> > Did we agree that it was a good idea to weaken it to all tasks?
> > Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
> > the initial userns?
>
> I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
> ignore the allow_other mount option in such case? The idea is that
> CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
> user not mounting with allow_other preventing root from reading contents
> defeats the purpose at least partially.

If we want to be compatible with "user_allow_other", then it should be
checking if the uid/gid of the current task is mapped in the
filesystems user_ns (fsuidgid_has_mapping()).  Right?

Thanks,
Miklos