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

[Jann Horn]

On Wed, Apr 29, 2020 at 10:20 PM Bernd Edlinger
<bernd.edlinger@xxxxxxxxxx> wrote:
> On 4/29/20 9:26 PM, Jann Horn wrote:
> > On Wed, Apr 29, 2020 at 9:23 PM Bernd Edlinger
> > <bernd.edlinger@xxxxxxxxxx> wrote:
> >> On 4/29/20 7:58 PM, Linus Torvalds wrote:
> >>> 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.
> >>>
> >>
> >> I think the use case where a program attaches and detaches many
> >> processes at a high rate, is either an attack or a very aggressive
> >> virus checker, fixing a bug that prevents an attack is not a good
> >> idea, but fixing a bug that would otherwise break a virus checker
> >> would be a good thing.
> >>
> >> By the way, all other attempts to fix it look much more dangerous
> >> than my initially proposed patch, you know the one you hated, but
> >> it does work and does not look overly complicated either.
> >>
> >> What was the reason why that cannot be done this way?
> >
> > I'm not sure which patch you're talking about - I assume you don't
> > mean <https://lore.kernel.org/lkml/AM6PR03MB5170B06F3A2B75EFB98D071AE4E60@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/>?
> >
>
> No, I meant:
>
> [PATCH v7 15/16] exec: Fix dead-lock in de_thread with ptrace_attach
> https://marc.info/?l=linux-kernel&m=158559277631548&w=2

(on lore: https://lore.kernel.org/lkml/AM6PR03MB51700577CF9EF4972FDE568AE4CB0@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/)

I mean - I guess that kinda works? It'll mean that attaching to a task
with "strace" as root won't work reliably if the task is in the middle
of execve though, there'll be a weird race where if strace first
attaches to a sibling and then tries to attach to the thread going
through execve(), it'll fail to attach to that thread, and then lose
the process entirely once de_thread() has killed off the other
threads. My perfectionist side is somewhat irked by that.

Still, it's probably far better than Linus' "simpler change" where the
target just gets killed off if someone tries to trace it at the wrong
time.