Commit d049f74f2dbe71354d43d393ac3a188947811348

Authored by Kees Cook
Committed by Linus Torvalds
1 parent 1c3fc3e5cc

exec/ptrace: fix get_dumpable() incorrect tests

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 <segoon@openwall.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: "Luck, Tony" <tony.luck@intel.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Showing 5 changed files with 13 additions and 5 deletions Side-by-side Diff

arch/ia64/include/asm/processor.h
... ... @@ -319,7 +319,7 @@
319 319 regs->loadrs = 0; \
320 320 regs->r8 = get_dumpable(current->mm); /* set "don't zap registers" flag */ \
321 321 regs->r12 = new_sp - 16; /* allocate 16 byte scratch area */ \
322   - if (unlikely(!get_dumpable(current->mm))) { \
  322 + if (unlikely(get_dumpable(current->mm) != SUID_DUMP_USER)) { \
323 323 /* \
324 324 * Zap scratch regs to avoid leaking bits between processes with different \
325 325 * uid/privileges. \
... ... @@ -1669,6 +1669,12 @@
1669 1669 return (ret > SUID_DUMP_USER) ? SUID_DUMP_ROOT : ret;
1670 1670 }
1671 1671  
  1672 +/*
  1673 + * This returns the actual value of the suid_dumpable flag. For things
  1674 + * that are using this for checking for privilege transitions, it must
  1675 + * test against SUID_DUMP_USER rather than treating it as a boolean
  1676 + * value.
  1677 + */
1672 1678 int get_dumpable(struct mm_struct *mm)
1673 1679 {
1674 1680 return __get_dumpable(mm->flags);
include/linux/binfmts.h
... ... @@ -99,9 +99,6 @@
99 99 extern void would_dump(struct linux_binprm *, struct file *);
100 100  
101 101 extern int suid_dumpable;
102   -#define SUID_DUMP_DISABLE 0 /* No setuid dumping */
103   -#define SUID_DUMP_USER 1 /* Dump as user of process */
104   -#define SUID_DUMP_ROOT 2 /* Dump as root */
105 102  
106 103 /* Stack area protections */
107 104 #define EXSTACK_DEFAULT 0 /* Whatever the arch defaults to */
include/linux/sched.h
... ... @@ -323,6 +323,10 @@
323 323 extern void set_dumpable(struct mm_struct *mm, int value);
324 324 extern int get_dumpable(struct mm_struct *mm);
325 325  
  326 +#define SUID_DUMP_DISABLE 0 /* No setuid dumping */
  327 +#define SUID_DUMP_USER 1 /* Dump as user of process */
  328 +#define SUID_DUMP_ROOT 2 /* Dump as root */
  329 +
326 330 /* mm flags */
327 331 /* dumpable bits */
328 332 #define MMF_DUMPABLE 0 /* core dump is permitted */
... ... @@ -257,7 +257,8 @@
257 257 if (task->mm)
258 258 dumpable = get_dumpable(task->mm);
259 259 rcu_read_lock();
260   - if (!dumpable && !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
  260 + if (dumpable != SUID_DUMP_USER &&
  261 + !ptrace_has_cap(__task_cred(task)->user_ns, mode)) {
261 262 rcu_read_unlock();
262 263 return -EPERM;
263 264 }