Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

[Jann Horn]

On Wed, Apr 29, 2020 at 12:14 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Apr 28, 2020 at 2:53 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
> >
> > You don't need LSM_UNSAFE_PTRACE if the tracer has already passed a
> > ptrace_may_access() check against the post-execve creds of the target
> > - that's no different from having done PTRACE_ATTACH after execve is
> > over.
>
> Hmm. That sounds believable, I guess.
>
> But along these ways, I'm starting to think that we might perhaps skip
> the lock entirely.

Just as a note: One effect of that will be that if a process goes into
execve on a setuid binary, but bails out before the point of no
return, a tracer may fail to attach to it during that time window. But
that should be completely fine.

> What if we made the rule instead be:
>
>  - we move check_unsafe_exec() down. As far as I can tell, there's no
> reason it's that early - the flags it sets aren't actually used until
> when we actually do that final set_creds..

Right, we should be able to do that stuff quite a bit later than it happens now.

At the moment the final security_bprm_set_creds() seems to happen
before we're calling would_dump(), which computes some of the data
we'll need for access checks... I guess we'll have to split up
security_bprm_set_creds into one hook for "here's another executable
file that's part of the execve chain" and a second hook
("security_bprm_cred_sync()"?) for "now the ->unsafe flags are ready
and we're about to drop the lock again, time to modify the creds if
you want to do that". And then LSMs can make decisions that are
influenced by ->unsafe (fiddling with the creds or rejecting the
execution - I think e.g. selinux_bprm_set_creds() can do both) inside
the "cred sync point".

>  - we add a "next cred" pointer to the signal struct (or task struct)
>
>  - make the rule be that check_unsafe_exec() checks p->ptrace under
> the tasklist_lock (or sighand lock - whatever ends up being most
> convenient)
>
>  - set "next cred" to be the known next cred there too under the lock.
> We call this small locked region the "cred sync point".
>
>  - ptrace will check if we have the "in_exec" flag set and have one of
> those "next cred" pointers, in which case it checks both the old and
> the next credentials.
>
> No cred_guard_mutex at all, instead the rule is that as execve() goes
> through that "cred sync point", we have two cases
>
>  (a) either ptrace has attached (because task->ptrace is set), and it
> does the LSM_UNSAFE_PTRACE dance.
>
> or
>
>  (b) it knows that ptrace will check the next creds if it races with execve.
>
> And then after execve has installed the final new creds, it just
> clears the "next cred" pointer again, because at that point it knows
> that now any subsequent PTRACE_ATTACH will be checking the new creds.
>
> So instead of taking and dropping the cred_guard_mutex, we'd basically
> get rid of it entirely.
>
> Yeah, I didn't look at the seccomp case, but I guess the issues will be similar.

Yeah, seccomp should be able to reject any thread sync if the "next
cred" pointer is set on any of the threads. It should work well as
long as the lock around the "next cred" pointer is at least on the
level of the signal_struct (or broader), so that TSYNC can decide
whether everything's good before starting to iterate over the threads.