Skip to contentSkip to conversation

coredump: setuid core dump cleanups

PATCH NAK Eugene Teo 2 participants 9 messages Jul 31, 2007 – Aug 1, 2007

Subsystems (4)
lore
Share & tools
lore

Eugene Teo

author
#0 lore raw

cover coredump: setuid core dump cleanups

Hi,

A year ago, commit abf75a5033d4da7b8a7e92321d74021d1fcfb502 was included to fix a security vulnerability that is related to prctl privilege escalation, and suid_dumpable (CVE-2006-2451). But the commit was just a quick fix to prevent users from calling prctl(PR_SET_DUMPABLE, 2).

This patch series try to remove code that is related to the value 2 (suidsafe) core dump mode, and also re-implement Hidehiro-san's re-implementation of dumpable using a bit flag instead of a pair (see commit 6c5d523826dc639df709ed0f88c5d2ce25379652).

Thanks,
Eugene

author
#1 lore raw

PATCH 1/3 coredump: cleanup documentation for suid_dumpable

This patch removes documentation that is related to suidsafe core dump mode.

---
 Documentation/sysctl/fs.txt |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)
Documentation/sysctl/fs.txt +0 -6
index aa986a3..2318f7a 100644
@@ -142,12 +142,6 @@ or otherwise protected/tainted binaries. The modes are
 1 - (debug) - all processes dump core when possible. The core dump is
 	owned by the current user and no security is applied. This is
 	intended for system debugging situations only. Ptrace is unchecked.
-2 - (suidsafe) - any binary which normally would not be dumped is dumped
-	readable by root only. This allows the end user to remove
-	such a dump but not access it directly. For security reasons
-	core dumps in this mode will not overwrite one another or
-	other files. This mode is appropriate when administrators are
-	attempting to debug problems in a normal environment.
 
 ==============================================================
 
#2 lore raw

Re: [PATCH 1/3] coredump: cleanup documentation for suid_dumpable

Quoted 4 lines · On Tue, 31 Jul 2007 15:03:40 +0800 Eugene Teo <[email protected]> wrote:

This patch removes documentation that is related to suidsafe core dump mode.

NAK - this feature is actively used and can be set by the sysctl interface. The PRCTL fixup was just a bug being fixed. The sysctl interface is still relevant.

author
#3 lore raw

Re: [PATCH 1/3] coredump: cleanup documentation for suid_dumpable

Hi Alan,

Quoted 11 lines · Alan Cox
Quoted 4 lines · On Tue, 31 Jul 2007 15:03:40 +0800 Eugene Teo <[email protected]> wrote:

This patch removes documentation that is related to suidsafe core dump mode.

NAK - this feature is actively used and can be set by the sysctl interface. The PRCTL fixup was just a bug being fixed. The sysctl interface is still relevant.

Thank you for correcting me. Appreciated. I am not aware that it is still being actively used. May I know who or which program is using this feature?

Thanks,
Eugene

#4 lore raw

Re: [PATCH 1/3] coredump: cleanup documentation for suid_dumpable

Quoted 6 lines
Quoted 3 lines

NAK - this feature is actively used and can be set by the sysctl interface. The PRCTL fixup was just a bug being fixed. The sysctl interface is still relevant.

Thank you for correcting me. Appreciated. I am not aware that it is still being actively used. May I know who or which program is using this feature?

Not programs - people.

When you want to debug a large complex system with multiple setuid applications it can rapidly get quite unpleasant. If you
set /proc/sys/kernel/suid_dumpable to 2 and /proc/sys/kernel/core_pattern then it becomes possible to get hold of all the core dumps and debug the system as a whole. The system is not secure in this state but while you are doing that kind of debug on a devel system its usually acceptable.

Secondly we don't break userspace interfaces except in extreme cases - proc/sys/kernel/suid_dumpable is a user space interface

Alan

author
#5 lore raw

PATCH 2/3 coredump: remove suidsafe mode related dead code

In reply to Eugene Teo

This patch removes suidsafe core dump mode related dead code.

---
 fs/exec.c               |   16 +---------------
 include/linux/binfmts.h |    3 ---
 2 files changed, 1 insertions(+), 18 deletions(-)
fs/exec.c +1 -15
index 7bdea79..60b4080 100644
@@ -1723,8 +1723,6 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 	struct inode * inode;
 	struct file * file;
 	int retval = 0;
-	int fsuid = current->fsuid;
-	int flag = 0;
 	int ispipe = 0;
 
 	audit_core_dumps(signr);
@@ -1737,16 +1735,6 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 		up_write(&mm->mmap_sem);
 		goto fail;
 	}
-
-	/*
-	 *	We cannot trust fsuid as being the "true" uid of the
-	 *	process nor do we know its entire history. We only know it
-	 *	was tainted so we dump it as root in mode 2.
-	 */
-	if (get_dumpable(mm) == 2) {	/* Setuid core dump mode */
-		flag = O_EXCL;		/* Stop rewrite attacks */
-		current->fsuid = 0;	/* Dump root private */
-	}
 	set_dumpable(mm, 0);
 
 	retval = coredump_wait(exit_code);
@@ -1778,8 +1766,7 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
  		}
  	} else
  		file = filp_open(corename,
-				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE | flag,
-				 0600);
+				 O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
 	if (IS_ERR(file))
 		goto fail_unlock;
 	inode = file->f_path.dentry->d_inode;
@@ -1806,7 +1793,6 @@ int do_coredump(long signr, int exit_code, struct pt_regs * regs)
 close_fail:
 	filp_close(file, NULL);
 fail_unlock:
-	current->fsuid = fsuid;
 	complete_all(&mm->core_done);
 fail:
 	return retval;
include/linux/binfmts.h +0 -3
index 91c8c07..ca75ee4 100644
@@ -81,9 +81,6 @@ extern int search_binary_handler(struct linux_binprm *,struct pt_regs *);
 extern int flush_old_exec(struct linux_binprm * bprm);
 
 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 */
#6 lore raw

Re: [PATCH 2/3] coredump: remove suidsafe mode related dead code

Quoted 3 lines · On Tue, 31 Jul 2007 15:04:58 +0800 Eugene Teo <[email protected]> wrote:

This patch removes suidsafe core dump mode related dead code.

NAK: This feature is used by end users and people debugging complex system problems.

author
#7 lore raw

PATCH 3/3 coredump: re-implement suid_dumpable using a flag

In reply to Eugene Teo

Hidehiro-san re-implemented suid_dumpable using a pair of bit flags. But since we no longer permitting users to call prctl(PR_SET_DUMPABLE, 2), there is no need to waste a bit of mm_struct.flags for something that will never be used.

---
 fs/exec.c             |   42 +++++-------------------------------------
 include/linux/sched.h |   13 ++++++-------
 2 files changed, 11 insertions(+), 44 deletions(-)
fs/exec.c +5 -37
index 60b4080..0f30b94 100644
@@ -1666,53 +1666,21 @@ fail:
 }
 
 /*
- * set_dumpable converts traditional three-value dumpable to two flags and
- * stores them into mm->flags.  It modifies lower two bits of mm->flags, but
- * these bits are not changed atomically.  So get_dumpable can observe the
- * intermediate state.  To avoid doing unexpected behavior, get get_dumpable
- * return either old dumpable or new one by paying attention to the order of
- * modifying the bits.
- *
- * dumpable |   mm->flags (binary)
- * old  new | initial interim  final
- * ---------+-----------------------
- *  0    1  |   00      01      01
- *  0    2  |   00      10(*)   11
- *  1    0  |   01      00      00
- *  1    2  |   01      11      11
- *  2    0  |   11      10(*)   00
- *  2    1  |   11      11      01
- *
- * (*) get_dumpable regards interim value of 10 as 11.
+ * set_dumpable converts traditional two-value dumpable to one flag and
+ * stores it in mm->flags. It modifies the lower bit of mm->flags.
  */
 void set_dumpable(struct mm_struct *mm, int value)
 {
-	switch (value) {
-	case 0:
+	if (value == 0)
 		clear_bit(MMF_DUMPABLE, &mm->flags);
-		smp_wmb();
-		clear_bit(MMF_DUMP_SECURELY, &mm->flags);
-		break;
-	case 1:
-		set_bit(MMF_DUMPABLE, &mm->flags);
-		smp_wmb();
-		clear_bit(MMF_DUMP_SECURELY, &mm->flags);
-		break;
-	case 2:
-		set_bit(MMF_DUMP_SECURELY, &mm->flags);
-		smp_wmb();
+	else if (value == 1)
 		set_bit(MMF_DUMPABLE, &mm->flags);
-		break;
-	}
 }
 EXPORT_SYMBOL_GPL(set_dumpable);
 
 int get_dumpable(struct mm_struct *mm)
 {
-	int ret;
-
-	ret = mm->flags & 0x3;
-	return (ret >= 2) ? 2 : ret;
+	return (mm->flags & 0x1);
 }
 
 int do_coredump(long signr, int exit_code, struct pt_regs * regs)
include/linux/sched.h +6 -7
index 2e49027..8a0092d 100644
@@ -350,15 +350,14 @@ extern int get_dumpable(struct mm_struct *mm);
 
 /* mm flags */
 /* dumpable bits */
-#define MMF_DUMPABLE      0  /* core dump is permitted */
-#define MMF_DUMP_SECURELY 1  /* core file is readable only by root */
-#define MMF_DUMPABLE_BITS 2
+#define MMF_DUMPABLE		0  /* core dump is permitted */
+#define MMF_DUMPABLE_BITS	1
 
 /* coredump filter bits */
-#define MMF_DUMP_ANON_PRIVATE	2
-#define MMF_DUMP_ANON_SHARED	3
-#define MMF_DUMP_MAPPED_PRIVATE	4
-#define MMF_DUMP_MAPPED_SHARED	5
+#define MMF_DUMP_ANON_PRIVATE	1
+#define MMF_DUMP_ANON_SHARED	2
+#define MMF_DUMP_MAPPED_PRIVATE	3
+#define MMF_DUMP_MAPPED_SHARED	4
 #define MMF_DUMP_FILTER_SHIFT	MMF_DUMPABLE_BITS
 #define MMF_DUMP_FILTER_BITS	4
 #define MMF_DUMP_FILTER_MASK \
#8 lore raw

Re: [PATCH 3/3] coredump: re-implement suid_dumpable using a flag

Quoted 6 lines · On Tue, 31 Jul 2007 15:05:38 +0800 Eugene Teo <[email protected]> wrote:

Hidehiro-san re-implemented suid_dumpable using a pair of bit flags. But since we no longer permitting users to call prctl(PR_SET_DUMPABLE, 2), there is no need to waste a bit of mm_struct.flags for something that will never be used.

NAK. This is dependant upon removing a feature in use by end users today.