Re: [PATCH] coredump - as root not only if euid switched

Re: [PATCH] coredump - as root not only if euid switched

[Peter Waechtler]

 
On Thursday, April 22, 2004, at 10:55AM, Andrew Morton <akpm@osdl.org> wrote:

>Peter Waechtler <pwaechtler@mac.com> wrote:
>>
>>  But I agree that sys_unlink should be the fast call and dumping core
>>  is the exception :)
>> 
>>  would fastcall do_unlink() help? I guess the arg is then passed in a
>>  register
>
>I've never been able to measure any size or space benefit for fastcall, and
>we do it via compiler options kernel-wide nowadays.
>
>The above will work fine.  You can probably just open-code it at the place
>where you're unlinking the file.
>
>(why are you trying to unlink the old file anyway?)
>

For security measure :O
I tried on solaris: touch the core file as user, open it and wait, dump core
as root -> nope, couldn't read the damn core - it was unlinked and created!
So do I want.
I will sent the new patch from home.

Re: [PATCH] coredump - as root not only if euid switched

[Andrew Morton]

Peter Waechtler <pwaechtler@mac.com> wrote:
>
>  >(why are you trying to unlink the old file anyway?)
>  >
> 
>  For security measure :O
>  I tried on solaris: touch the core file as user, open it and wait, dump core
>  as root -> nope, couldn't read the damn core - it was unlinked and created!

hm, OK.  There's a window in which someone can come in and recreate the
file, but the open is using O_EXCL|O_CREATE so that seems safe enough.

Re: [PATCH] coredump - as root not only if euid switched

[Peter Wächtler]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

Am Do, 2004-04-22 um 11.56 schrieb Andrew Morton:
> Peter Waechtler <pwaechtler@mac.com> wrote:
> >
> >  >(why are you trying to unlink the old file anyway?)
> >  >
> > 
> >  For security measure :O
> >  I tried on solaris: touch the core file as user, open it and wait, dump core
> >  as root -> nope, couldn't read the damn core - it was unlinked and created!
> 
> hm, OK.  There's a window in which someone can come in and recreate the
> file, but the open is using O_EXCL|O_CREATE so that seems safe enough.

So here is the updated patch with an open coded call to sys_unlink


[-- Attachment #2: Type: text/x-patch, Size: 3674 bytes --]

diff -Nur -X dontdiff linux-2.6.5/fs/exec.c linux-2.6/fs/exec.c
--- linux-2.6.5/fs/exec.c	2004-04-08 22:06:32.000000000 +0200
+++ linux-2.6/fs/exec.c	2004-04-22 20:44:59.000000000 +0200
@@ -1378,6 +1378,10 @@
 		goto fail;
 	}
 	mm->dumpable = 0;
+	if (mm->dump_as_root){
+		current->fsuid = 0;
+		current->fsgid = 0;
+	}
 	init_completion(&mm->core_done);
 	current->signal->group_exit = 1;
 	current->signal->group_exit_code = exit_code;
@@ -1387,9 +1391,23 @@
 		goto fail_unlock;
 
  	format_corename(corename, core_pattern, signr);
-	file = filp_open(corename, O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
-	if (IS_ERR(file))
-		goto fail_unlock;
+	file = filp_open(corename, O_CREAT | O_EXCL | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
+	if (IS_ERR(file)){
+        long error;
+        mm_segment_t old_fs = get_fs();
+
+        set_fs(KERNEL_DS);
+        error = sys_unlink(corename);
+        set_fs(old_fs);
+        if (error)
+			goto fail_unlock;
+		file = filp_open(corename, O_CREAT | O_EXCL | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
+		if (IS_ERR(file)){
+			printk(KERN_WARNING "pid %d doesn't dump core to %s because of creation race\n",
+				current->pid, corename);
+			goto fail_unlock;
+		}
+	}
 	inode = file->f_dentry->d_inode;
 	if (inode->i_nlink > 1)
 		goto close_fail;	/* multiple links - don't dump */
diff -Nur -X dontdiff linux-2.6.5/include/linux/sched.h linux-2.6/include/linux/sched.h
--- linux-2.6.5/include/linux/sched.h	2004-04-08 22:07:23.000000000 +0200
+++ linux-2.6/include/linux/sched.h	2004-04-08 19:14:17.000000000 +0200
@@ -211,6 +211,7 @@
 	unsigned long saved_auxv[40]; /* for /proc/PID/auxv */
 
 	unsigned dumpable:1;
+	unsigned dump_as_root:1;
 #ifdef CONFIG_HUGETLB_PAGE
 	int used_hugetlb;
 #endif
diff -Nur -X dontdiff linux-2.6.5/kernel/sys.c linux-2.6/kernel/sys.c
--- linux-2.6.5/kernel/sys.c	2004-04-08 22:06:37.000000000 +0200
+++ linux-2.6/kernel/sys.c	2004-04-08 19:22:10.000000000 +0200
@@ -573,7 +573,7 @@
 	}
 	if (new_egid != old_egid)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	if (rgid != (gid_t) -1 ||
@@ -603,7 +603,7 @@
 	{
 		if(old_egid != gid)
 		{
-			current->mm->dumpable=0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->gid = current->egid = current->sgid = current->fsgid = gid;
@@ -612,7 +612,7 @@
 	{
 		if(old_egid != gid)
 		{
-			current->mm->dumpable=0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->egid = current->fsgid = gid;
@@ -641,7 +641,7 @@
 
 	if(dumpclear)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->uid = new_ruid;
@@ -698,7 +698,7 @@
 
 	if (new_euid != old_euid)
 	{
-		current->mm->dumpable=0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->fsuid = current->euid = new_euid;
@@ -746,7 +746,7 @@
 
 	if (old_euid != uid)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->fsuid = current->euid = uid;
@@ -789,7 +789,7 @@
 	if (euid != (uid_t) -1) {
 		if (euid != current->euid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->euid = euid;
@@ -837,7 +837,7 @@
 	if (egid != (gid_t) -1) {
 		if (egid != current->egid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->egid = egid;
@@ -882,7 +882,7 @@
 	{
 		if (uid != old_fsuid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->fsuid = uid;
@@ -910,7 +910,7 @@
 	{
 		if (gid != old_fsgid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->fsgid = gid;

Re: [PATCH] coredump - as root not only if euid switched

[Chris Wright]

* Peter Wächtler (pwaechtler@mac.com) wrote:
> Am Do, 2004-04-22 um 11.56 schrieb Andrew Morton:
> > Peter Waechtler <pwaechtler@mac.com> wrote:
> > >
> > >  >(why are you trying to unlink the old file anyway?)
> > >  >
> > > 
> > >  For security measure :O
> > >  I tried on solaris: touch the core file as user, open it and wait, dump core
> > >  as root -> nope, couldn't read the damn core - it was unlinked and created!
> > 
> > hm, OK.  There's a window in which someone can come in and recreate the
> > file, but the open is using O_EXCL|O_CREATE so that seems safe enough.
> 
> So here is the updated patch with an open coded call to sys_unlink

This patch breaks various ptrace() checks.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] coredump - as root not only if euid switched

[Linus Torvalds]

On Thu, 22 Apr 2004, Peter Wächtler wrote:
>
> > hm, OK.  There's a window in which someone can come in and recreate the
> > file, but the open is using O_EXCL|O_CREATE so that seems safe enough.
> 
> So here is the updated patch with an open coded call to sys_unlink

Aughr. 

Wouldn't it be much nicer to just refuse to overwrite files owned by 
anybody else?

In other words, I'd much rather see a patch that is a much simpler one, 
which just says: if we opened an existing file, we won't touch it if we 
weren't the owners of it.

That should be safe for root _and_ it should be safe for people who 
already had a file descriptor open previously (hey, if the previous 
root-owned core-file was world readable, then what else is new?)

Tell me why this isn't simpler?

		Linus

---
--- 1.111/fs/exec.c	Wed Apr 21 02:11:57 2004
+++ edited/fs/exec.c	Thu Apr 22 13:03:27 2004
@@ -1378,6 +1378,8 @@
 	inode = file->f_dentry->d_inode;
 	if (inode->i_nlink > 1)
 		goto close_fail;	/* multiple links - don't dump */
+	if (inode->i_uid != current->euid || inode->i_gid != current->egid)
+		goto close_fail;
 	if (d_unhashed(file->f_dentry))
 		goto close_fail;
 

Re: [PATCH] coredump - as root not only if euid switched

[Albert Cahalan]

> While it's more secure to not dump core at all if the
> program has switched euid, it's also very unpractical.
> Since only programs started from root, being setuid
> root or have CAP_SETUID it's far more practical to
> dump as root.root mode 600. This is the bahavior 
> of Solaris.

Solaris can keep their security holes.

Consider a setuid core dump on removable media which
is user-controlled.

Also consider filesystems that don't store full security
data, like vfat and smb/cifs.

Core dumps to remote filesystems are a problem in
general, because the server might not implement the
type of security you expect it to implement.

Here's a better idea: add a sysctl for insecure core
dumps. When set, dump all cores as root.root mode 444.
Ignore directory permissions when doing so, so that
forcing dumps into a MacOS-style /cores directory does
not require that users be able to access it normally.
This lets appropriately authorized users debug setuid
apps and get support for them without adding security
holes like Solaris has.

Re: [PATCH] coredump - as root not only if euid switched

[Peter Wächtler]

Am Fr, 2004-04-23 um 17.35 schrieb Albert Cahalan:
> > While it's more secure to not dump core at all if the
> > program has switched euid, it's also very unpractical.
> > Since only programs started from root, being setuid
> > root or have CAP_SETUID it's far more practical to
> > dump as root.root mode 600. This is the bahavior 
> > of Solaris.
> 
> Solaris can keep their security holes.
> 

I checked (older) versions on

HP-UX, True64, AiX, MacOsX

HP-UX didn't dump core on a seteuid 0->n prog
Aix,MacOsX and True64 dumped core with ownership of user
I could check Irix

> Consider a setuid core dump on removable media which
> is user-controlled.
> 

boot into rescue system...

> Also consider filesystems that don't store full security
> data, like vfat and smb/cifs.
> 
> Core dumps to remote filesystems are a problem in
> general, because the server might not implement the
> type of security you expect it to implement.
> 

mkdir /var/cores
chmod a+rwx,o+t /var/cores
echo /var/cores/%e.core.%p > /proc/sys/kernel/core_pattern

> 
> Here's a better idea: add a sysctl for insecure core
> dumps. When set, dump all cores as root.root mode 444.
> Ignore directory permissions when doing so, so that
> forcing dumps into a MacOS-style /cores directory does
> not require that users be able to access it normally.
> This lets appropriately authorized users debug setuid
> apps and get support for them without adding security
> holes like Solaris has.

It's tunable via coreadm

troll, troll

Re: [PATCH] coredump - as root not only if euid switched

[Albert Cahalan]

On Fri, 2004-04-23 at 15:14, Peter Wächtler wrote:
> Am Fr, 2004-04-23 um 17.35 schrieb Albert Cahalan:
> > > While it's more secure to not dump core at all if the
> > > program has switched euid, it's also very unpractical.
> > > Since only programs started from root, being setuid
> > > root or have CAP_SETUID it's far more practical to
> > > dump as root.root mode 600. This is the bahavior 
> > > of Solaris.
> > 
> > Solaris can keep their security holes.
> > 
> 
> I checked (older) versions on
> 
> HP-UX, True64, AiX, MacOsX
> 
> HP-UX didn't dump core on a seteuid 0->n prog
> Aix,MacOsX and True64 dumped core with ownership of user
> I could check Irix

It would be nice of you to do so, then issue some
sort of security alert.

> > Consider a setuid core dump on removable media which
> > is user-controlled.
> 
> boot into rescue system...

If you're suggesting that user-controlled media implies
the ability to boot via that media, you're very wrong.

a. LinuxBIOS
b. OpenFirmware (Mac or Sun) with boot password
c. no floppy, and a Zip drive on BIOS-disabled SCSI
d. parallel port Zip drive
e. FireWire w/o BIOS support

(the "rip computer open" option is either physically
blocked or there is a human nearby that would notice)

> > Also consider filesystems that don't store full security
> > data, like vfat and smb/cifs.
> > 
> > Core dumps to remote filesystems are a problem in
> > general, because the server might not implement the
> > type of security you expect it to implement.
> > 
> 
> mkdir /var/cores
> chmod a+rwx,o+t /var/cores
> echo /var/cores/%e.core.%p > /proc/sys/kernel/core_pattern

Sure, but you shouldn't assume that all admins know
to do this. The default must be secure.

> > Here's a better idea: add a sysctl for insecure core
> > dumps. When set, dump all cores as root.root mode 444.
> > Ignore directory permissions when doing so, so that
> > forcing dumps into a MacOS-style /cores directory does
> > not require that users be able to access it normally.
> > This lets appropriately authorized users debug setuid
> > apps and get support for them without adding security
> > holes like Solaris has.
> 
> It's tunable via coreadm
> 
> troll, troll

You said "This is the bahavior of Solaris." and I took
that for the truth. Now you say it's tunable. Well, OK
then, so Solaris _doesn't_ have an insecure config as
it comes from Sun? I have no problem with supporting an
insecure config, but you were suggesting that it would
be the new default (and only) behavior.

Re: [PATCH] coredump - as root not only if euid switched

[Peter Waechtler]

 
On Thursday, April 22, 2004, at 09:53PM, Chris Wright <chrisw@osdl.org> wrote:

>* Peter W�chtler (pwaechtler@mac.com) wrote:
>> Am Do, 2004-04-22 um 11.56 schrieb Andrew Morton:
>> > Peter Waechtler <pwaechtler@mac.com> wrote:
>> > >
>> > >  >(why are you trying to unlink the old file anyway?)
>> > >  >
>> > > 
>> > >  For security measure :O
>> > >  I tried on solaris: touch the core file as user, open it and wait, dump core
>> > >  as root -> nope, couldn't read the damn core - it was unlinked and created!
>> > 
>> > hm, OK.  There's a window in which someone can come in and recreate the
>> > file, but the open is using O_EXCL|O_CREATE so that seems safe enough.
>> 
>> So here is the updated patch with an open coded call to sys_unlink
>
>This patch breaks various ptrace() checks.
>

I guess the mm->dumpable flag was misused then as something like mm->switchUid.
I can add this flag and make the ptrace paths use that.

Re: [PATCH] coredump - as root not only if euid switched

[Peter Waechtler]

 
On Thursday, April 22, 2004, at 09:53PM, Chris Wright <chrisw@osdl.org> wrote:

>* Peter W�chtler (pwaechtler@mac.com) wrote:
>> Am Do, 2004-04-22 um 11.56 schrieb Andrew Morton:
>> > Peter Waechtler <pwaechtler@mac.com> wrote:
>> > >
>> > >  >(why are you trying to unlink the old file anyway?)
>> > >  >
>> > > 
>> > >  For security measure :O
>> > >  I tried on solaris: touch the core file as user, open it and wait, dump core
>> > >  as root -> nope, couldn't read the damn core - it was unlinked and created!
>> > 
>> > hm, OK.  There's a window in which someone can come in and recreate the
>> > file, but the open is using O_EXCL|O_CREATE so that seems safe enough.
>> 
>> So here is the updated patch with an open coded call to sys_unlink
>
>This patch breaks various ptrace() checks.
>
>thanks,

please
Care to share your wisdom? No? Then please don't reply

Re: [PATCH] coredump - as root not only if euid switched

[Chris Wright]

* Peter Waechtler (pwaechtler@mac.com) wrote:
> On Thursday, April 22, 2004, at 09:53PM, Chris Wright <chrisw@osdl.org> wrote:
> >This patch breaks various ptrace() checks.
> 
> Care to share your wisdom? No? Then please don't reply

-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;

Changes like that break ptrace authentication checks.  Look more closely
at what mm->dumpable is used for, you'll see checks like:

	if (!task->mm->dumpable && !capable(CAP_SYS_PTRACE))
		goto out;

BTW, putting the patch inline makes it easier to comment directly on the
patch.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] coredump - as root not only if euid switched

[Peter Wächtler]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

Am Fr, 2004-04-23 um 19.10 schrieb Chris Wright:
> * Peter Waechtler (pwaechtler@mac.com) wrote:
> > On Thursday, April 22, 2004, at 09:53PM, Chris Wright <chrisw@osdl.org> wrote:
> > >This patch breaks various ptrace() checks.
> > 
> > Care to share your wisdom? No? Then please don't reply
> 
> -		current->mm->dumpable = 0;
> +		current->mm->dump_as_root = 1;
> 
> Changes like that break ptrace authentication checks.  Look more closely
> at what mm->dumpable is used for, you'll see checks like:
> 
> 	if (!task->mm->dumpable && !capable(CAP_SYS_PTRACE))
> 		goto out;


added checks like

	if ((task->mm->dump_as_root || !task->mm->dumpable)
			&& !capable(CAP_SYS_PTRACE))

in kernel/ptrace.c and fs/proc/base.c

I guess the checks will get more and more complicated, perhaps
we should invent a special flag dont_ptrace or so



[-- Attachment #2: coredump-patch --]
[-- Type: text/x-patch, Size: 4938 bytes --]

diff -Nur -X dontdiff linux-2.6.5/fs/exec.c linux-2.6/fs/exec.c
--- linux-2.6.5/fs/exec.c	2004-04-08 22:06:32.000000000 +0200
+++ linux-2.6/fs/exec.c	2004-04-22 21:17:42.000000000 +0200
@@ -1378,6 +1378,10 @@
 		goto fail;
 	}
 	mm->dumpable = 0;
+	if (mm->dump_as_root){
+		current->fsuid = 0;
+		current->fsgid = 0;
+	}
 	init_completion(&mm->core_done);
 	current->signal->group_exit = 1;
 	current->signal->group_exit_code = exit_code;
@@ -1387,9 +1391,23 @@
 		goto fail_unlock;
 
  	format_corename(corename, core_pattern, signr);
-	file = filp_open(corename, O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
-	if (IS_ERR(file))
-		goto fail_unlock;
+	file = filp_open(corename, O_CREAT | O_EXCL | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
+	if (IS_ERR(file)){
+        long error;
+        mm_segment_t old_fs = get_fs();
+
+        set_fs(KERNEL_DS);
+        error = sys_unlink(corename);
+        set_fs(old_fs);
+        if (error)
+			goto fail_unlock;
+		file = filp_open(corename, O_CREAT | O_EXCL | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
+		if (IS_ERR(file)){
+			printk(KERN_WARNING "pid %d doesn't dump core to %s because of creation race\n",
+				current->pid, corename);
+			goto fail_unlock;
+		}
+	}
 	inode = file->f_dentry->d_inode;
 	if (inode->i_nlink > 1)
 		goto close_fail;	/* multiple links - don't dump */
diff -Nur -X dontdiff linux-2.6.5/fs/proc/base.c linux-2.6/fs/proc/base.c
--- linux-2.6.5/fs/proc/base.c	2004-04-08 22:06:33.000000000 +0200
+++ linux-2.6/fs/proc/base.c	2004-04-23 19:41:57.000000000 +0200
@@ -299,7 +299,8 @@
 	     (current->gid != task->gid)) && !capable(CAP_SYS_PTRACE))
 		goto out;
 	rmb();
-	if (!task->mm->dumpable && !capable(CAP_SYS_PTRACE))
+	if ((task->mm->dump_as_root || !task->mm->dumpable)
+			&& !capable(CAP_SYS_PTRACE))
 		goto out;
 	if (security_ptrace(current, task))
 		goto out;
@@ -924,7 +925,7 @@
 	task_lock(task);
 	mm = task->mm;
 	if (mm)
-		dumpable = mm->dumpable;
+		dumpable = !mm->dump_as_root || mm->dumpable;
 	task_unlock(task);
 	return dumpable;
 }
diff -Nur -X dontdiff linux-2.6.5/include/linux/sched.h linux-2.6/include/linux/sched.h
--- linux-2.6.5/include/linux/sched.h	2004-04-08 22:07:23.000000000 +0200
+++ linux-2.6/include/linux/sched.h	2004-04-08 19:14:17.000000000 +0200
@@ -211,6 +211,7 @@
 	unsigned long saved_auxv[40]; /* for /proc/PID/auxv */
 
 	unsigned dumpable:1;
+	unsigned dump_as_root:1;
 #ifdef CONFIG_HUGETLB_PAGE
 	int used_hugetlb;
 #endif
diff -Nur -X dontdiff linux-2.6.5/kernel/ptrace.c linux-2.6/kernel/ptrace.c
--- linux-2.6.5/kernel/ptrace.c	2004-04-08 22:07:24.000000000 +0200
+++ linux-2.6/kernel/ptrace.c	2004-04-23 19:38:45.000000000 +0200
@@ -97,7 +97,8 @@
  	    (current->gid != task->gid)) && !capable(CAP_SYS_PTRACE))
 		goto bad;
 	rmb();
-	if (!task->mm->dumpable && !capable(CAP_SYS_PTRACE))
+	if ((task->mm->dump_as_root || !task->mm->dumpable) 
+			&& !capable(CAP_SYS_PTRACE))
 		goto bad;
 	/* the same process cannot be attached many times */
 	if (task->ptrace & PT_PTRACED)
diff -Nur -X dontdiff linux-2.6.5/kernel/sys.c linux-2.6/kernel/sys.c
--- linux-2.6.5/kernel/sys.c	2004-04-08 22:06:37.000000000 +0200
+++ linux-2.6/kernel/sys.c	2004-04-08 19:22:10.000000000 +0200
@@ -573,7 +573,7 @@
 	}
 	if (new_egid != old_egid)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	if (rgid != (gid_t) -1 ||
@@ -603,7 +603,7 @@
 	{
 		if(old_egid != gid)
 		{
-			current->mm->dumpable=0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->gid = current->egid = current->sgid = current->fsgid = gid;
@@ -612,7 +612,7 @@
 	{
 		if(old_egid != gid)
 		{
-			current->mm->dumpable=0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->egid = current->fsgid = gid;
@@ -641,7 +641,7 @@
 
 	if(dumpclear)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->uid = new_ruid;
@@ -698,7 +698,7 @@
 
 	if (new_euid != old_euid)
 	{
-		current->mm->dumpable=0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->fsuid = current->euid = new_euid;
@@ -746,7 +746,7 @@
 
 	if (old_euid != uid)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->fsuid = current->euid = uid;
@@ -789,7 +789,7 @@
 	if (euid != (uid_t) -1) {
 		if (euid != current->euid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->euid = euid;
@@ -837,7 +837,7 @@
 	if (egid != (gid_t) -1) {
 		if (egid != current->egid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->egid = egid;
@@ -882,7 +882,7 @@
 	{
 		if (uid != old_fsuid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->fsuid = uid;
@@ -910,7 +910,7 @@
 	{
 		if (gid != old_fsgid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->fsgid = gid;

Re: [PATCH] coredump - as root not only if euid switched

[Peter Waechtler]

 
On Thursday, April 22, 2004, at 10:05PM, Linus Torvalds <torvalds@osdl.org> wrote:

>
>
>On Thu, 22 Apr 2004, Peter W�chtler wrote:
>>
>> > hm, OK.  There's a window in which someone can come in and recreate the
>> > file, but the open is using O_EXCL|O_CREATE so that seems safe enough.
>> 
>> So here is the updated patch with an open coded call to sys_unlink
>
>Aughr. 
>
>Wouldn't it be much nicer to just refuse to overwrite files owned by 
>anybody else?
>
>In other words, I'd much rather see a patch that is a much simpler one, 
>which just says: if we opened an existing file, we won't touch it if we 
>weren't the owners of it.
>
>That should be safe for root _and_ it should be safe for people who 
>already had a file descriptor open previously (hey, if the previous 
>root-owned core-file was world readable, then what else is new?)
>

the previous core was owned by user.donttellyourwisdom
The root process happily dumps it's core into it, doesn't change 
ownership nor permissions.

>Tell me why this isn't simpler?
>
I can't it's simpler.
If you are interested in the core (you don't because you don't make 
errors, you don't even use debuggers but change the VM ;> )
you get it, otherwise you have an old one.

>
>---
>--- 1.111/fs/exec.c	Wed Apr 21 02:11:57 2004
>+++ edited/fs/exec.c	Thu Apr 22 13:03:27 2004
>@@ -1378,6 +1378,8 @@
> 	inode = file->f_dentry->d_inode;
> 	if (inode->i_nlink > 1)
> 		goto close_fail;	/* multiple links - don't dump */
>+	if (inode->i_uid != current->euid || inode->i_gid != current->egid)
>+		goto close_fail;
> 	if (d_unhashed(file->f_dentry))
> 		goto close_fail;
> 
>
>

Re: [PATCH] coredump - as root not only if euid switched

[Peter Waechtler]

 
On Thursday, April 22, 2004, at 03:18AM, Andrew Morton <akpm@osdl.org> wrote:

>Peter W�chtler <pwaechtler@mac.com> wrote:
>>
>> While it's more secure to not dump core at all if the program has
>>  switched euid, it's also very unpractical. Since only programs 
>>  started from root, being setuid root or have CAP_SETUID it's far 
>>  more practical to dump as root.root mode 600. This is the bahavior 
>>  of Solaris.
>> 
>>  The current implementation does not ensure that an existing core
>>  file is only readable as root, i.e. after dumping the ownership 
>>  and mode is unchanged.
>> 
>>  Besides mm->dumpable to avoid recursive core dumps, on setuid files 
>>  the dumpable flag still prevents a core dump while seteuid & co will
>>  result in a core only readable as root.
>
>It's a bit sad to add another function call level to sys_unlink() simply
>because the core dumping code needs it.
>
>Is it not possible to call sys_unlink() directly from there?  Something like
>
>long kernel_unlink(const char *name)
>{
>	mm_segment_t old_fs = get_fs();
>	long ret;
>
>	set_fs(KERNEL_DS);
>	ret = sys_unlink(name);
>	set_fs(old_fs);
>	return ret;
>}	

And you're asking me? ;)
While getname() has a check for user/kernelspace - do you really
care about "the overhead" for a function call level with 1 argument?
Uhm, probably if you even care to write this..

What is the cost for switching the segments?
But I agree that sys_unlink should be the fast call and dumping core
is the exception :)

would fastcall do_unlink() help? I guess the arg is then passed in a
register

Re: [PATCH] coredump - as root not only if euid switched

[Andrew Morton]

Peter Waechtler <pwaechtler@mac.com> wrote:
>
>  >Is it not possible to call sys_unlink() directly from there?  Something like
>  >
>  >long kernel_unlink(const char *name)
>  >{
>  >	mm_segment_t old_fs = get_fs();
>  >	long ret;
>  >
>  >	set_fs(KERNEL_DS);
>  >	ret = sys_unlink(name);
>  >	set_fs(old_fs);
>  >	return ret;
>  >}	
> 
>  And you're asking me? ;)

Well someone has to know ;)

>  While getname() has a check for user/kernelspace - do you really
>  care about "the overhead" for a function call level with 1 argument?
>  Uhm, probably if you even care to write this..

It's very small, but heck, we unlink a lot more often than we dump core. 
Most of us, that is.

>  What is the cost for switching the segments?

Teeny.

>  But I agree that sys_unlink should be the fast call and dumping core
>  is the exception :)
> 
>  would fastcall do_unlink() help? I guess the arg is then passed in a
>  register

I've never been able to measure any size or space benefit for fastcall, and
we do it via compiler options kernel-wide nowadays.

The above will work fine.  You can probably just open-code it at the place
where you're unlinking the file.

(why are you trying to unlink the old file anyway?)

[PATCH] coredump - as root not only if euid switched

[Peter Wächtler]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

While it's more secure to not dump core at all if the program has
switched euid, it's also very unpractical. Since only programs 
started from root, being setuid root or have CAP_SETUID it's far 
more practical to dump as root.root mode 600. This is the bahavior 
of Solaris.

The current implementation does not ensure that an existing core
file is only readable as root, i.e. after dumping the ownership 
and mode is unchanged.

Besides mm->dumpable to avoid recursive core dumps, on setuid files 
the dumpable flag still prevents a core dump while seteuid & co will
result in a core only readable as root.

[-- Attachment #2: Type: text/x-patch, Size: 5226 bytes --]

diff -Nur -X dontdiff linux-2.6.5/fs/exec.c linux-2.6/fs/exec.c
--- linux-2.6.5/fs/exec.c	2004-04-08 22:06:32.000000000 +0200
+++ linux-2.6/fs/exec.c	2004-04-08 23:13:49.000000000 +0200
@@ -1378,6 +1378,10 @@
 		goto fail;
 	}
 	mm->dumpable = 0;
+	if (mm->dump_as_root){
+		current->fsuid = 0;
+		current->fsgid = 0;
+	}
 	init_completion(&mm->core_done);
 	current->signal->group_exit = 1;
 	current->signal->group_exit_code = exit_code;
@@ -1387,9 +1391,14 @@
 		goto fail_unlock;
 
  	format_corename(corename, core_pattern, signr);
-	file = filp_open(corename, O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
-	if (IS_ERR(file))
-		goto fail_unlock;
+	file = filp_open(corename, O_CREAT | O_EXCL | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
+	if (IS_ERR(file)){
+		if (do_unlink(corename))
+			goto fail_unlock;
+		file = filp_open(corename, O_CREAT | O_EXCL | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
+		if (IS_ERR(file))
+			goto fail_unlock;
+	}
 	inode = file->f_dentry->d_inode;
 	if (inode->i_nlink > 1)
 		goto close_fail;	/* multiple links - don't dump */
diff -Nur -X dontdiff linux-2.6.5/fs/namei.c linux-2.6/fs/namei.c
--- linux-2.6.5/fs/namei.c	2004-04-08 22:07:21.000000000 +0200
+++ linux-2.6/fs/namei.c	2004-04-08 23:21:25.000000000 +0200
@@ -1700,18 +1700,13 @@
  * writeout happening, and we don't want to prevent access to the directory
  * while waiting on the I/O.
  */
-asmlinkage long sys_unlink(const char __user * pathname)
+int do_unlink(const char *name)
 {
 	int error = 0;
-	char * name;
 	struct dentry *dentry;
 	struct nameidata nd;
 	struct inode *inode = NULL;
 
-	name = getname(pathname);
-	if(IS_ERR(name))
-		return PTR_ERR(name);
-
 	error = path_lookup(name, LOOKUP_PARENT, &nd);
 	if (error)
 		goto exit;
@@ -1736,8 +1731,6 @@
 exit1:
 	path_release(&nd);
 exit:
-	putname(name);
-
 	if (inode)
 		iput(inode);	/* truncate the inode here */
 	return error;
@@ -1746,6 +1739,22 @@
 	error = !dentry->d_inode ? -ENOENT :
 		S_ISDIR(dentry->d_inode->i_mode) ? -EISDIR : -ENOTDIR;
 	goto exit2;
+
+}
+
+asmlinkage long sys_unlink(const char __user * pathname)
+{
+	int error = 0;
+	char * name;
+
+	name = getname(pathname);
+	if(IS_ERR(name))
+		return PTR_ERR(name);
+
+	error = do_unlink(name);
+	putname(name);
+
+	return error;
 }
 
 int vfs_symlink(struct inode *dir, struct dentry *dentry, const char *oldname)
diff -Nur -X dontdiff linux-2.6.5/include/linux/fs.h linux-2.6/include/linux/fs.h
--- linux-2.6.5/include/linux/fs.h	2004-04-08 22:07:23.000000000 +0200
+++ linux-2.6/include/linux/fs.h	2004-04-08 21:57:16.000000000 +0200
@@ -1253,6 +1253,7 @@
 
 extern int open_namei(const char *, int, int, struct nameidata *);
 extern int may_open(struct nameidata *, int, int);
+extern int do_unlink(const char *);
 
 extern int kernel_read(struct file *, unsigned long, char *, unsigned long);
 extern struct file * open_exec(const char *);
diff -Nur -X dontdiff linux-2.6.5/include/linux/sched.h linux-2.6/include/linux/sched.h
--- linux-2.6.5/include/linux/sched.h	2004-04-08 22:07:23.000000000 +0200
+++ linux-2.6/include/linux/sched.h	2004-04-08 19:14:17.000000000 +0200
@@ -211,6 +211,7 @@
 	unsigned long saved_auxv[40]; /* for /proc/PID/auxv */
 
 	unsigned dumpable:1;
+	unsigned dump_as_root:1;
 #ifdef CONFIG_HUGETLB_PAGE
 	int used_hugetlb;
 #endif
diff -Nur -X dontdiff linux-2.6.5/kernel/sys.c linux-2.6/kernel/sys.c
--- linux-2.6.5/kernel/sys.c	2004-04-08 22:06:37.000000000 +0200
+++ linux-2.6/kernel/sys.c	2004-04-08 19:22:10.000000000 +0200
@@ -573,7 +573,7 @@
 	}
 	if (new_egid != old_egid)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	if (rgid != (gid_t) -1 ||
@@ -603,7 +603,7 @@
 	{
 		if(old_egid != gid)
 		{
-			current->mm->dumpable=0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->gid = current->egid = current->sgid = current->fsgid = gid;
@@ -612,7 +612,7 @@
 	{
 		if(old_egid != gid)
 		{
-			current->mm->dumpable=0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->egid = current->fsgid = gid;
@@ -641,7 +641,7 @@
 
 	if(dumpclear)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->uid = new_ruid;
@@ -698,7 +698,7 @@
 
 	if (new_euid != old_euid)
 	{
-		current->mm->dumpable=0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->fsuid = current->euid = new_euid;
@@ -746,7 +746,7 @@
 
 	if (old_euid != uid)
 	{
-		current->mm->dumpable = 0;
+		current->mm->dump_as_root = 1;
 		wmb();
 	}
 	current->fsuid = current->euid = uid;
@@ -789,7 +789,7 @@
 	if (euid != (uid_t) -1) {
 		if (euid != current->euid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->euid = euid;
@@ -837,7 +837,7 @@
 	if (egid != (gid_t) -1) {
 		if (egid != current->egid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->egid = egid;
@@ -882,7 +882,7 @@
 	{
 		if (uid != old_fsuid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->fsuid = uid;
@@ -910,7 +910,7 @@
 	{
 		if (gid != old_fsgid)
 		{
-			current->mm->dumpable = 0;
+			current->mm->dump_as_root = 1;
 			wmb();
 		}
 		current->fsgid = gid;

Re: [PATCH] coredump - as root not only if euid switched

[Andrew Morton]

Peter Wächtler <pwaechtler@mac.com> wrote:
>
> While it's more secure to not dump core at all if the program has
>  switched euid, it's also very unpractical. Since only programs 
>  started from root, being setuid root or have CAP_SETUID it's far 
>  more practical to dump as root.root mode 600. This is the bahavior 
>  of Solaris.
> 
>  The current implementation does not ensure that an existing core
>  file is only readable as root, i.e. after dumping the ownership 
>  and mode is unchanged.
> 
>  Besides mm->dumpable to avoid recursive core dumps, on setuid files 
>  the dumpable flag still prevents a core dump while seteuid & co will
>  result in a core only readable as root.

It's a bit sad to add another function call level to sys_unlink() simply
because the core dumping code needs it.

Is it not possible to call sys_unlink() directly from there?  Something like

long kernel_unlink(const char *name)
{
	mm_segment_t old_fs = get_fs();
	long ret;

	set_fs(KERNEL_DS);
	ret = sys_unlink(name);
	set_fs(old_fs);
	return ret;
}