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

[Linus Torvalds]

On Tue, Apr 28, 2020 at 4:36 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
>
> On Wed, Apr 29, 2020 at 12:14 AM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> >  - 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.

Actually, looking at it, this looks painful for multiple reasons.

The LSM_UNSAFE_xyz flags are used by security_bprm_set_creds(), which
when I traced it through, happened much earlier than I thought. Making
things worse, it's done by prepare_binprm(), which also potentially
gets called from random points by the low-level binfmt handlers too.

And we also have that odd "fs->in_exec" flag, which is used by thread
cloning and io_uring, and I'm not sure what the exact semantics are.

I'm _almost_ inclined to say that we should just abort the execve()
entirely if somebody tries to attach in the middle.

IOW, get rid of the locking, and replace it all just with a sequence
count. Make execve() abort if the sequence count has changed between
loading the original creds, and having installed the new creds.

You can ptrace _over_ an execve, and you can ptrace _after_ an
execve(), but trying to attach just as we execve() would just cause
the execve() to fail.

We could maybe make it conditional on the credentials actually having
changed at all (set another flag in bprm_fill_uid()). So it would only
fail for the suid exec case.

Because honestly, trying to ptrace in the middle of a suid execve()
sounds like an attack, not a useful thing.

That sequence count approach would be a much simpler change.

              Linus