Linux kernel: fs: fix get_dumpable() incorrect tests (CVE-2013-2929)

[Solar Designer <solar () openwall com>]

I'm afraid the issue below was never brought to oss-security (as it must
have been).  The fix was committed on November 13:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d049f74f2dbe71354d43d393ac3a188947811348

including detailed description and the CVE-2013-2929 reference.  So it
was clearly disclosed as a security issue, yet bringing it to
oss-security specifically seems to have falled through the cracks. :-(

Kees' posting to linux-distros:

----- Forwarded message from Kees Cook <kees () outflux net> -----

From: Kees Cook <kees () outflux net>
Subject: [PATCH] fs: fix get_dumpable() incorrect tests
Date: Tue, 22 Oct 2013 16:25:30 -0700

The get_dumpable() return value is not boolean. Most users of the
function actually want to be testing for SUID_DUMP_USER rather than
non-zero.

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.)

CVE-2013-2929

Reported-by: Vasily Kulikov <segoon () openwall com>
Signed-off-by: Kees Cook <keescook () chromium org>
Cc: stable () vger kernel org
---
I plan to send this to security () kernel org on Monday, unless anyone
would like to push that a bit. Also note, I do not have ia64 cross
compilers available, so I used the "1" literal instead of adding
binfmts.h to the ia64 code.
---
 arch/ia64/include/asm/processor.h |    2 +-
 fs/exec.c                         |    6 ++++++
 kernel/ptrace.c                   |    4 +++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/include/asm/processor.h b/arch/ia64/include/asm/processor.h
index e0a899a..d036a33 100644
--- a/arch/ia64/include/asm/processor.h
+++ b/arch/ia64/include/asm/processor.h
@@ -319,7 +319,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) != 1)) { \
                /*                                                                              \
                 * Zap scratch regs to avoid leaking bits between processes with different      \
                 * uid/privileges.                                                              \
diff --git a/fs/exec.c b/fs/exec.c
index 8875dd1..bb8afc1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1668,6 +1668,12 @@ int __get_dumpable(unsigned long mm_flags)
        return (ret > SUID_DUMP_USER) ? SUID_DUMP_ROOT : 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/kernel/ptrace.c b/kernel/ptrace.c
index dd562e9..a75ad41 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -26,6 +26,7 @@
 #include <linux/hw_breakpoint.h>
 #include <linux/cn_proc.h>
 #include <linux/compat.h>
+#include <linux/binfmts.h>
 
 
 static int ptrace_trapping_sleep_fn(void *flags)
@@ -257,7 +258,8 @@ ok:
        if (task->mm)
                dumpable = get_dumpable(task->mm);
        rcu_read_lock();
-       if (!dumpable && !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
+       if (dumpable != SUID_DUMP_USER &&
+           !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
                rcu_read_unlock();
                return -EPERM;
        }
-- 
1.7.9.5


-- 
Kees Cook                                            @outflux.net

----- End forwarded message -----