Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751179AbeACRVT (ORCPT + 1 other); Wed, 3 Jan 2018 12:21:19 -0500 Received: from mail-ua0-f194.google.com ([209.85.217.194]:37374 "EHLO mail-ua0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750819AbeACRVS (ORCPT ); Wed, 3 Jan 2018 12:21:18 -0500 X-Google-Smtp-Source: ACJfBotlGmX4ivheLUXcwnk4P6B9t6iP3/4UHtVd67zUtmhTe9ew+o0a7eQZnFT8E3/DpK0dKdvaYljV1WXmDqJ5BPA= MIME-Version: 1.0 In-Reply-To: <20180103070622.GA6950@mail.hallyn.com> References: <20180102232133.GA39880@beast> <20180103070622.GA6950@mail.hallyn.com> From: Kees Cook Date: Wed, 3 Jan 2018 09:21:16 -0800 X-Google-Sender-Auth: rH67t8IVEFnUl7GCtSQwn1q84TI Message-ID: Subject: Re: [PATCH] exec: Weaken dumpability for secureexec To: "Serge E. Hallyn" Cc: Linus Torvalds , Tom Horsley , Laura Abbott , David Howells , James Morris , LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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. The more interesting thing here is that secureexec is set for a process that ISN'T actually setuid. (ptrace of a setuid process). I 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. -Kees -- Kees Cook Pixel Security