On Tue, May 17, 2022 at 12:50:32PM -0400, Dave Marchevsky wrote: > On 11/15/21 10:28 AM, Miklos Szeredi wrote: > > On Sat, 13 Nov 2021 at 00:29, Dave Marchevsky <davemarchevsky@xxxxxx> wrote: > > > >>> If your tracing daemon runs in init_user_ns with CAP_SYS_ADMIN why can't > >>> it simply use a helper process/thread to > >>> setns(userns_fd/pidfd, CLONE_NEWUSER) > >>> to the target userns? This way we don't need to special-case > >>> init_user_ns at all. > >> > >> helper process + setns could work for my usecase. But the fact that there's no > >> way to say "I know what I am about to do is potentially stupid and dangerous, > >> but I am root so let me do it", without spawning a helper process in this case, > >> feels like it'll result in special-case userspace workarounds for anyone doing > >> symbolication of backtraces. > > > > Note: any mechanism that grants filesystem access to users that have > > higher privileges than the daemon serving the filesystem will > > potentially open DoS attacks against the higher privilege task. This > > would be somewhat mitigated if the filesystem is only mounted in a > > private mount namespace, but AFAICS that's not guaranteed. > > > > The above obviously applies to your original patch but it also applies > > to any other mechanism where the high privilege user doesn't > > explicitly acknowledge and accept the consequences. IOW granting the > > exception has to be initiated by the high privleged user. > > > > Thanks, > > Miklos > > > > Sorry to ressurect this old thread. My proposed alternate approach of "special > ioctl to grant exception to descendant userns check" proved unnecessarily > complex: ioctls also go through fuse_allow_current_process check, so a special > carve-out would be necessary for in both ioctl and fuse_permission check in > order to make it possible for non-descendant-userns user to opt in to exception. > > How about a version of this patch with CAP_DAC_READ_SEARCH check? This way > there's more of a clear opt-in vs CAP_SYS_ADMIN. I still think this isn't needed given that especially for the use-cases listed here you have a workable userspace solution to this problem. If the CAP_SYS_ADMIN/CAP_DAC_READ_SEARCH check were really just about giving a privileged task access then it'd be fine imho. But given that this means the privileged task is open to a DoS attack it seems we're building a trap into the fuse code. The setns() model has the advantage that this forces the task to assume the correct privileges and also serves as an explicit opt-in. Just my 2 cents here. > > FWIW we've been running CAP_SYS_ADMIN version of this patch internally and > can confirm it fixes tracing tools' ability to symbolicate binaries in FUSE. > > > > >> > >> e.g. perf will have to add some logic: "did I fail > >> to grab this exe that some process had mapped? Is it in a FUSE mounted by some > >> descendant userns? let's fork a helper process..." Not the end of the world, > >> but unnecessary complexity nonetheless. > >> > >> That being said, I agree that this patch's special-casing of init_user_ns is > >> hacky. What do you think about a more explicit and general "let me do this > >> stupid and dangerous thing" mechanism - perhaps a new struct fuse_conn field > >> containing a set of exception userns', populated with ioctl or similar. > > > > > > > >> > >>> > >>>> > >>>> Note: I was unsure whether CAP_SYS_ADMIN or CAP_SYS_PTRACE was the best > >>>> choice of capability here. Went with the former as it's checked > >>>> elsewhere in fs/fuse while CAP_SYS_PTRACE isn't. > >>>> > >>>> fs/fuse/dir.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > >>>> index 0654bfedcbb0..2524eeb0f35d 100644 > >>>> --- a/fs/fuse/dir.c > >>>> +++ b/fs/fuse/dir.c > >>>> @@ -1134,7 +1134,7 @@ int fuse_allow_current_process(struct fuse_conn *fc) > >>>> const struct cred *cred; > >>>> > >>>> if (fc->allow_other) > >>>> - return current_in_userns(fc->user_ns); > >>>> + return current_in_userns(fc->user_ns) || capable(CAP_SYS_ADMIN); > >>>> > >>>> cred = current_cred(); > >>>> if (uid_eq(cred->euid, fc->user_id) && > >>>> -- > >>>> 2.30.2 > >>>> > >>>> > >>
