Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751142AbeACRlR (ORCPT + 1 other); Wed, 3 Jan 2018 12:41:17 -0500 Received: from h2.hallyn.com ([78.46.35.8]:46642 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750865AbeACRlQ (ORCPT ); Wed, 3 Jan 2018 12:41:16 -0500 Date: Wed, 3 Jan 2018 11:41:15 -0600 From: "Serge E. Hallyn" To: Kees Cook Cc: "Serge E. Hallyn" , Linus Torvalds , Tom Horsley , Laura Abbott , David Howells , James Morris , LKML Subject: Re: [PATCH] exec: Weaken dumpability for secureexec Message-ID: <20180103174115.GA15331@mail.hallyn.com> References: <20180102232133.GA39880@beast> <20180103070622.GA6950@mail.hallyn.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Quoting Kees Cook (keescook@chromium.org): > On Tue, Jan 2, 2018 at 11:06 PM, Serge E. Hallyn wrote: > > On Tue, Jan 02, 2018 at 03:21:33PM -0800, Kees Cook wrote: > >> This is a logical revert of: > >> > >> commit e37fdb785a5f ("exec: Use secureexec for setting dumpability") > >> > >> This weakens dumpability back to checking only for uid/gid changes in > >> current (which is useless), but userspace depends on dumpability not > >> being tied to secureexec. > >> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1528633 > >> > >> Reported-by: Tom Horsley > >> Fixes: e37fdb785a5f ("exec: Use secureexec for setting dumpability") > >> Cc: stable@vger.kernel.org > >> Signed-off-by: Kees Cook > >> --- > >> fs/exec.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/exec.c b/fs/exec.c > >> index 5688b5e1b937..7eb8d21bcab9 100644 > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -1349,9 +1349,14 @@ void setup_new_exec(struct linux_binprm * bprm) > >> > >> current->sas_ss_sp = current->sas_ss_size = 0; > >> > >> - /* Figure out dumpability. */ > >> + /* > >> + * Figure out dumpability. Note that this checking only of current > >> + * is wrong, but userspace depends on it. This should be testing > >> + * bprm->secureexec instead. > >> + */ > >> if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || > >> - bprm->secureexec) > >> + !(uid_eq(current_euid(), current_uid()) && > >> + gid_eq(current_egid(), current_gid()))) > > > > So what about the pdeath_signal? Is that going to be another subtle > > time-bomb? > > pdeath_signal used another wrong method to set itself, but it was > better than dumpable. I'd rather we leave it as-is, since I'd like to > have everything controlled by secureexec. Yes, but if there is some weird userspace out there that depends on pdeath_signal handling in the same corner case, then we'll break that just like we did, except it'll be even harder to track down, because debugging a wrong pdeath_signal will be even more subtle, and it'll fail only when it's supposed to be exiting... > The more interesting thing here is that secureexec is set for a > process that ISN'T actually setuid. (ptrace of a setuid process). I yeah i'd like to find some time to track that down too. > think tha'ts the real bug, but not something I'm going to be able to > fix quickly. So, for now, I want to revert this, then try to fix the > weird case, and see if that breaks anyone, then fix this back to > secureexec. That sounds good, I'm only saying that the core bug is the wrong setting of secureexec, and you've switched both setting of pdeath and dumpability to using secureexec, so it stands to reason that setting of pdeath is still wrong in these cases. -serge