Messages in this thread | | | From | Jann Horn <> | Date | Wed, 29 Apr 2020 21:26:49 +0200 | Subject | Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1 |
| |
On Wed, Apr 29, 2020 at 9:23 PM Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 4/29/20 7:58 PM, Linus Torvalds wrote: > > On Tue, Apr 28, 2020 at 4:36 PM Jann Horn <jannh@google.com> wrote: > >> > >> On Wed, Apr 29, 2020 at 12:14 AM Linus Torvalds > >> <torvalds@linux-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. > > > > 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@AM6PR03MB5170.eurprd03.prod.outlook.com/>?
| |