From patchwork Fri May 30 08:17:36 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tyler Hicks X-Patchwork-Id: 353952 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id DBF631400F5; Fri, 30 May 2014 18:18:10 +1000 (EST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1WqI0t-0000xj-JB; Fri, 30 May 2014 08:18:07 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1WqI0W-0000qz-SY for kernel-team@lists.ubuntu.com; Fri, 30 May 2014 08:17:44 +0000 Received: from [194.158.52.142] (helo=boyd.l.tihix.com) by youngberry.canonical.com with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1WqI0W-0001E6-Nz; Fri, 30 May 2014 08:17:44 +0000 From: Tyler Hicks To: kernel-team@lists.ubuntu.com Subject: [PATCH 4/4] exec/ptrace: fix get_dumpable() incorrect tests Date: Fri, 30 May 2014 10:17:36 +0200 Message-Id: <1401437856-26438-5-git-send-email-tyhicks@canonical.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1401437856-26438-1-git-send-email-tyhicks@canonical.com> References: <1401437856-26438-1-git-send-email-tyhicks@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com From: Kees Cook BugLink: http://bugs.launchpad.net/bugs/1266546 commit d049f74f2dbe71354d43d393ac3a188947811348 upstream. The get_dumpable() return value is not boolean. Most users of the function actually want to be testing for non-SUID_DUMP_USER(1) rather than SUID_DUMP_DISABLE(0). The SUID_DUMP_ROOT(2) is also considered a protected state. Almost all places did this correctly, excepting the two places fixed in this patch. Wrong logic: if (dumpable == SUID_DUMP_DISABLE) { /* be protective */ } or if (dumpable == 0) { /* be protective */ } or if (!dumpable) { /* be protective */ } Correct logic: if (dumpable != SUID_DUMP_USER) { /* be protective */ } or if (dumpable != 1) { /* be protective */ } Without this patch, if the system had set the sysctl fs/suid_dumpable=2, a user was able to ptrace attach to processes that had dropped privileges to that user. (This may have been partially mitigated if Yama was enabled.) The macros have been moved into the file that declares get/set_dumpable(), which means things like the ia64 code can see them too. CVE-2013-2929 Reported-by: Vasily Kulikov Signed-off-by: Kees Cook Cc: "Luck, Tony" Cc: Oleg Nesterov Cc: "Eric W. Biederman" Cc: Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds [tyhicks: Backported to 3.4] Signed-off-by: Tyler Hicks --- arch/ia64/include/asm/processor.h | 2 +- fs/exec.c | 6 ++++++ include/linux/binfmts.h | 3 --- include/linux/sched.h | 4 ++++ kernel/ptrace.c | 3 ++- 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/arch/ia64/include/asm/processor.h b/arch/ia64/include/asm/processor.h index 483f6c6..2d0cb8e 100644 --- a/arch/ia64/include/asm/processor.h +++ b/arch/ia64/include/asm/processor.h @@ -322,7 +322,7 @@ struct thread_struct { regs->loadrs = 0; \ regs->r8 = get_dumpable(current->mm); /* set "don't zap registers" flag */ \ regs->r12 = new_sp - 16; /* allocate 16 byte scratch area */ \ - if (unlikely(!get_dumpable(current->mm))) { \ + if (unlikely(get_dumpable(current->mm) != SUID_DUMP_USER)) { \ /* \ * Zap scratch regs to avoid leaking bits between processes with different \ * uid/privileges. \ diff --git a/fs/exec.c b/fs/exec.c index ee5bdaa..7efefc1 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -2020,6 +2020,12 @@ static int __get_dumpable(unsigned long mm_flags) return (ret >= 2) ? 2 : ret; } +/* + * This returns the actual value of the suid_dumpable flag. For things + * that are using this for checking for privilege transitions, it must + * test against SUID_DUMP_USER rather than treating it as a boolean + * value. + */ int get_dumpable(struct mm_struct *mm) { return __get_dumpable(mm->flags); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 366422b..a34e590 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -115,9 +115,6 @@ extern void setup_new_exec(struct linux_binprm * bprm); extern void would_dump(struct linux_binprm *, struct file *); extern int suid_dumpable; -#define SUID_DUMP_DISABLE 0 /* No setuid dumping */ -#define SUID_DUMP_USER 1 /* Dump as user of process */ -#define SUID_DUMP_ROOT 2 /* Dump as root */ /* Stack area protections */ #define EXSTACK_DEFAULT 0 /* Whatever the arch defaults to */ diff --git a/include/linux/sched.h b/include/linux/sched.h index 5fb3719..226daf4 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -404,6 +404,10 @@ static inline void arch_pick_mmap_layout(struct mm_struct *mm) {} extern void set_dumpable(struct mm_struct *mm, int value); extern int get_dumpable(struct mm_struct *mm); +#define SUID_DUMP_DISABLE 0 /* No setuid dumping */ +#define SUID_DUMP_USER 1 /* Dump as user of process */ +#define SUID_DUMP_ROOT 2 /* Dump as root */ + /* mm flags */ /* dumpable bits */ #define MMF_DUMPABLE 0 /* core dump is permitted */ diff --git a/kernel/ptrace.c b/kernel/ptrace.c index ee8d49b..94d6f85 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -215,7 +215,8 @@ ok: smp_rmb(); if (task->mm) dumpable = get_dumpable(task->mm); - if (!dumpable && !ptrace_has_cap(task_user_ns(task), mode)) + if (dumpable != SUID_DUMP_USER && + !ptrace_has_cap(task_user_ns(task), mode)) return -EPERM; return security_ptrace_access_check(task, mode);