On Mon, 13 Jun 2022 at 12:46, Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Mon, Jun 13, 2022 at 12:34:05PM +0200, Miklos Szeredi 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... > > I aimed to show that. You can setfs*id() and retain capabilities and > still access the filesystem. > > > > > Maybe the original patch is fine: this check isn't meant to protect > > the filesystem from access, it's meant to protect the accessor. > > I don't have specific worries here. I'm just a bit hesitant to just let > anyone access the fs. But if we go for allow other semantics then that's > probably fine. Though I wonder why then we don't just do: > > if (fc->allow_other) > return current_in_userns(fc->user_ns) || > (userns_allow_other && > ns_capable(fc->user_ns, CAP_SYS_ADMIN)); > > ? That'll let any ancestor userns access the fs not just descendants of > fc->user_ns. Looks good to me. Thanks, Miklos
