[PATCH 0/3] coredump: setuid core dump cleanups

[PATCH 0/3] coredump: setuid core dump cleanups

[Eugene Teo]

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

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

[Eugene Teo]

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

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 Documentation/sysctl/fs.txt |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index aa986a3..2318f7a 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -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.
 
 ==============================================================
 

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

[Alan Cox]

On Tue, 31 Jul 2007 15:03:40 +0800
Eugene Teo <eugeneteo@kernel.sg> wrote:

> This patch removes documentation that is related to suidsafe core dump
> mode.
> 
> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>

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.

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

[Eugene Teo]

Hi Alan,

Alan Cox wrote:
> On Tue, 31 Jul 2007 15:03:40 +0800
> Eugene Teo <eugeneteo@kernel.sg> wrote:
> 
>> This patch removes documentation that is related to suidsafe core dump
>> mode.
>>
>> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
> 
> 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

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

[Alan Cox]

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

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

[Eugene Teo]

This patch removes suidsafe core dump mode related dead code.

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 fs/exec.c               |   16 +---------------
 include/linux/binfmts.h |    3 ---
 2 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 7bdea79..60b4080 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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;
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 91c8c07..ca75ee4 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -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 */

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

[Alan Cox]

On Tue, 31 Jul 2007 15:04:58 +0800
Eugene Teo <eugeneteo@kernel.sg> wrote:

> This patch removes suidsafe core dump mode related dead code.
> 
> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>

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

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

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

Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>
---
 fs/exec.c             |   42 +++++-------------------------------------
 include/linux/sched.h |   13 ++++++-------
 2 files changed, 11 insertions(+), 44 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 60b4080..0f30b94 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2e49027..8a0092d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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 \

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

[Alan Cox]

On Tue, 31 Jul 2007 15:05:38 +0800
Eugene Teo <eugeneteo@kernel.sg> 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.
> 
> Signed-off-by: Eugene Teo <eugeneteo@kernel.sg>

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