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

[Christian Brauner <brauner@xxxxxxxxxx>]

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));

so say a fuse filesystem is mounted in a userns with a mapping of
0:10000:100. Assume root in init_user_ns is trying to access that fuse
filesystem:

fuse_sb->s_user_ns = 0:10000:100
current_fsuid() = 0
current_fsgid() = 0
capable(CAP_SYS_ADMIN)

that would fail as

fsuidgid_has_mapping() {
	kuid_has_mapping(0:10000:1000, 0) -> INVALID_UID
	kgid_has_mapping(0:10000:1000, 0) -> INVALID_GID
}

so root would have to do:

setfsuid(100000)
setfsgid(100000)

// This transition will cost you at least
// CAP_CHOWN
// CAP_MKNOD
// CAP_DAC_OVERRIDE
// CAP_DAC_READ_SEARCH
// CAP_FOWNER
// CAP_FSETID
// but those are regained when transitioning back to fsuid/fsgid 0.

fuse_sb->s_user_ns = 0:10000:100
current_fsuid() = 100000
current_fsgid() = 100000
capable(CAP_SYS_ADMIN)

that would succeed as

fsuidgid_has_mapping() {
	kuid_has_mapping(0:10000:1000, 0) -> 0
	kgid_has_mapping(0:10000:1000, 0) -> 0
}