On Mon, Jul 06, 2020 at 07:44:38PM +0200, Christian Brauner wrote: > On Mon, Jul 06, 2020 at 05:13:35PM +0000, Nicolas Viennot wrote: > > > > This is scary. But I believe it is safe. > > > > > > > > Reviewed-by: Serge Hallyn <serge@xxxxxxxxxx> > > > > > > > > I am a bit curious about the implications of the selinux patch. > > > > IIUC you are using the permission of the tracing process to execute > > > > the file without transition, so this is a way to work around the > > > > policy which might prevent the tracee from doing so. > > > > Given that SELinux wants to be MAC, I'm not *quite* sure that's > > > > considered kosher. You also are skipping the PROCESS__PTRACE to > > > > SECCLASS_PROCESS check which selinux_bprm_set_creds does later on. > > > > Again I'm just not quite sure what's considered normal there these > > > > days. > > > > > > > > Paul, do you have input there? > > > > > > I agree, the SELinux hook looks wrong. Building on what Christian said, this looks more like a ptrace operation than an exec operation. > > > > Serge, Paul, Christian, > > > > I made a PoC to demonstrate the change of /proc/self/exe without CAP_SYS_ADMIN using only ptrace and execve. > > You may find it here: https://github.com/nviennot/run_as_exe > > > > What do you recommend to relax the security checks in the kernel when it comes to changing the exe link? > > Looks fun! Yeah, so that this is possible is known afaict. But you're > not really circumventing the kernel check but are mucking with the EFL > by changing the auxv, right? > > Originally, you needed to be userns root, i.e. only uid 0 could > change the /proc/self/exe link (cf. [1]). This was changed to > ns_capable(CAP_SYS_ADMIN) in [2]. > > The original reasoning in [1] is interesting as it basically already > points to your poc: > > "Still note that updating exe-file link now doesn't require sys-resource > capability anymore, after all there is no much profit in preventing > setup own file link (there are a number of ways to execute own code -- > ptrace, ld-preload, so that the only reliable way to find which exactly > code is executed is to inspect running program memory). Still we > require the caller to be at least user-namespace root user." > > There were arguments being made that /proc/<pid>/exe needs to be sm that > userspace can have a decent amount of trust in but I believe that that's > not a great argument. > > But let me dig a little into the original discussion and see what the > thread-model was. > At this point I'm starting to believe that it was people being cautios > but better be sure. Ok, so the original patch proposal was presented in [4] in 2014. The final version of that patch added the PR_SET_MM_MAP we know today. The initial version presented in [4] did not require _any_ privilege. So the reasoning for only placing the /proc/<pid>/exe link under ns_capable(CAP_SYS_ADMIN) is very thin. to quote from [5]: "Controlling exe_fd without privileges may turn out to be dangerous. At least things like tomoyo examine it for making policy decisions (see tomoyo_manager())." So yes, tomoyo_get_exe() is what this was retained for apparently: const char *tomoyo_get_exe(void) { struct file *exe_file; const char *cp; struct mm_struct *mm = current->mm; if (!mm) return NULL; exe_file = get_mm_exe_file(mm); if (!exe_file) return NULL; cp = tomoyo_realpath_from_path(&exe_file->f_path); fput(exe_file); return cp; } The exe path is literally used in tomoyo_manager() to verify that you are allowed to change policy. That seems like a bad idea to me but then again, I don't know enough about Tomoyo. In any case, I think that means we can't remove CAP_SYS_ADMIN because that would make things worse than they are right now for Tomoyo but I also don't see why placing this under ns_capable(CAP_SYS_ADMIN) || ns_capable(CAP_CHECKPOINT_RESTORE) would make this any worse. And Cyrill (and later in that thread Andrei) already mentioned it in [6]: "@exe_fd is just a hint and as I mentioned if we have ptrace/preload rights there damn a lot of ways to inject own code into any program so that a user won't even notice ;)" Another place where the exe file is relevant is for the coredump with the -E option. But it only uses the path when generating the coredump pattern and if that's a security issue than your poc shows that this can already be achieved today. Christian > > [1]: f606b77f1a9e ("prctl: PR_SET_MM -- introduce PR_SET_MM_MAP operation") > [2]: 4d28df6152aa ("prctl: Allow local CAP_SYS_ADMIN changing exe_file") > [3]: https://lore.kernel.org/patchwork/patch/697304/ [4]: https://lore.kernel.org/lkml/20140703151102.842945837@xxxxxxxxxx/ [5]: https://lore.kernel.org/lkml/CAGXu5jL3exT4j+8rjMv1O54uJWQ5UHL69Z-24b61rhXROqZamQ@xxxxxxxxxxxxxx/ [6]: https://lore.kernel.org/lkml/20140722203614.GF838@moon/
