Re: [PATCH v4 3/3] prctl: Allow ptrace capable processes to change /proc/self/exe

[Christian Brauner <christian.brauner@ubuntu.com>]

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@hallyn.com>
> > > >
> > > > 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@openvz.org/ 
[5]: https://lore.kernel.org/lkml/CAGXu5jL3exT4j+8rjMv1O54uJWQ5UHL69Z-24b61rhXROqZamQ@mail.gmail.com/
[6]: https://lore.kernel.org/lkml/20140722203614.GF838@moon/