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@xxxxxxxxxx> wrote:
> >
> > On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxx> 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.
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.
Christian
