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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Jun 14, 2022 at 7:33 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Jun 13, 2022 at 11:21:24AM -0700, Andrii Nakryiko wrote:
> > On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
> > > > > 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?
> > > >
> > > > I think that's doable. So assuming we're still talking about requiring
> > > > cap_sys_admin then we'd roughly have sm like:
> > > >
> > > >         if (fc->allow_other)
> > > >                 return current_in_userns(fc->user_ns) ||
> > > >                         (capable(CAP_SYS_ADMIN) &&
> > > >                         fsuidgid_has_mapping(..., &init_user_ns));
> > >
> > > No, I meant this:
> > >
> > >         if (fc->allow_other)
> > >                 return current_in_userns(fc->user_ns) ||
> > >                         (userns_allow_other &&
> > >                         fsuidgid_has_mapping(..., &init_user_ns));
> > >
> > > But I think the OP wanted to allow real root to access the fs, which
> > > this doesn't allow (since 0 will have no mapping in the user ns), so
> > > I'm not sure what's the right solution...
> >
> > Right, so I was basically asking why not do something like this:
> >
> > $ git diff
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 74303d6e987b..8c04955eb26e 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> >  {
> >         const struct cred *cred;
> >
> > +       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
> > +               return 1;
> > +
> >         if (fc->allow_other)
> >                 return current_in_userns(fc->user_ns);
> >
> >
> > where fuse_allow_sys_admin_access is module param which has to be
> > opted into through sysfs?
>
> You can either do this or do what I suggested in:
> https://lore.kernel.org/linux-fsdevel/20220613104604.t5ptuhrl2d4l7kbl@wittgenstein
> which is a bit more lax.

My logic was that given we require opt-in and we are root, we
shouldn't be prevented from reading contents just because someone
didn't know about allow_other mount option. So I'd go with a simple
check before we even check fc-allow_other.

>
> If you make it module load parameter only it has the advantage that it
> can't be changed after fuse has been loaded which in this case might be
> an advantage. It's likely that users might not be too happy if module
> semantics can be changed that drastically at runtime. But I have no
> strong opinions here.
>

I'm not too familiar with this, whatever Dave was doing with
MODULE_PARM_DESC seems to be working fine? Did you have some other
preference for a specific param mechanism?

> Christian