On Tue, Jan 2, 2018 at 11:06 PM, Serge E. Hallyn <[email protected]> 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 <[email protected]> >> Fixes: e37fdb785a5f ("exec: Use secureexec for setting dumpability") >> Cc: [email protected] >> Signed-off-by: Kees Cook <[email protected]> >> --- >> 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
