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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wh=G47oD2F1CgOrvGFbEPh2ddMKLV4_wV_bs6S=98aZ5A@mail.gmail.com>
Date:   Wed, 29 Apr 2020 10:58:11 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jann Horn <jannh@...gle.com>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Bernd Edlinger <bernd.edlinger@...mail.de>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Waiman Long <longman@...hat.com>,
        Ingo Molnar <mingo@...nel.org>, Will Deacon <will@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexey Gladkov <gladkov.alexey@...il.com>
Subject: Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

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