Project

General

Profile

Actions

Bug #950

closed

Coredumping design error

Added by ed about 16 years ago. Updated almost 16 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Hello,

Similar to CVE-2007-6206 [1] I also noticed this minor design error in
Dragon Fly BSD when using the default %N.core format.

uid=1001(user) gid=1001(user) groups=1001(user), 0(wheel)

./coredumper

Segmentation fault (core dumped)
syslog: Feb 16 09:40:22 kernel: pid 723 (coredumper), uid 1001:
exited on signal 11 (core dumped)

md5 coredumper.core

MD5 (coredumper.core) = 1a21427d1b52b9bbea22cbf2b207b6f7

ls -la coredumper.core

rw------ 1 user user 1003520 Feb 16 09:40 coredumper.core

su

Password:
syslog: Feb 16 09:40:56 su: user to root on /dev/ttyd0
  1. ./coredumper
    Segmentation fault (core dumped)
    syslog: Feb 16 09:41:14 kernel: pid 728 (coredumper), uid 0: exited
    on signal 11 (core dumped)
  2. md5 coredumper.core
    MD5 (coredumper.core) = 68e3e5fee874e688c795537721a6b511
  3. ls la coredumper.core
    -rw------
    1 user user 1003520 Feb 16 09:41 coredumper.core #

I was not able to test the below patch. Trivial enough to fix if broken.

--- kern_sig.c 2008-02-14 13:41:12.000000000 0800
++ kern_sig-20080216.c 2008-02-16 01:15:01.000000000 +0800
@ -2066,6 +2066,12 @ coredump(struct lwp *lp, int sig)
goto out1;
}

+ /* Don't dump to files current user does not own */
+ if (vattr.va_uid != p->p_ucred->cr_uid) {
+ error = EFAULT;
+ goto out1;
+ }
+
VATTR_NULL(&vattr);
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
vattr.va_size = 0;

Regards,
Ed

[1] <http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-6206>

Actions #1

Updated by corecode about 16 years ago

Shouldn't we rather remove the file and recreate a new file (which then
will be owned by root)?

cheers
simon

Actions #2

Updated by ed about 16 years ago

Hello Simon,

In my opinion checking for ownership is better. We are avoiding other
possible(?) bugs e.g. allowing to read files you don't own but resides
on a directory you own. I also noticed that non-root users trying to
coredump on other non-root users pre-created dumps fail silently.

By the way as seen in my patch, we wouldn't want to hard code != 0
because DragonFly may implement a type enforcement system or
authorization framework.

Up to you guys. I might be missing something.

Cheers,
Ed

On Feb 16, 2008 4:03 AM, Simon 'corecode' Schubert
<> wrote:

Eduardo Tongson wrote:

su

Password:
syslog: Feb 16 09:40:56 su: user to root on /dev/ttyd0
  1. ./coredumper
    Segmentation fault (core dumped)
    syslog: Feb 16 09:41:14 kernel: pid 728 (coredumper), uid 0: exited
    on signal 11 (core dumped)
  2. md5 coredumper.core
    MD5 (coredumper.core) = 68e3e5fee874e688c795537721a6b511
  3. ls la coredumper.core
    -rw------
    1 user user 1003520 Feb 16 09:41 coredumper.core #

I was not able to test the below patch. Trivial enough to fix if broken.

--- kern_sig.c 2008-02-14 13:41:12.000000000 0800
++ kern_sig-20080216.c 2008-02-16 01:15:01.000000000 +0800
@ -2066,6 +2066,12 @ coredump(struct lwp *lp, int sig)
goto out1;
}

+ /* Don't dump to files current user does not own */

Shouldn't we rather remove the file and recreate a new file (which then
will be owned by root)?

cheers
simon

Actions #3

Updated by dillon about 16 years ago

:Hello Simon,
:
:In my opinion checking for ownership is better. We are avoiding other
:possible(?) bugs e.g. allowing to read files you don't own but resides
:on a directory you own. I also noticed that non-root users trying to
:coredump on other non-root users pre-created dumps fail silently.
:
:By the way as seen in my patch, we wouldn't want to hard code != 0
:because DragonFly may implement a type enforcement system or
:authorization framework.
:
:Up to you guys. I might be missing something.
:
:Cheers,
: Ed

Yes, I agree, we definitely do not want to remove the file.  The reason
is that it is possible (and likely) that a service process that forks
may coredump multiple times, possibly even in parallel. Possibly
hundreds of times in parallel. This is one reason why the coredump
code gets an advisory lock on the file.
My only question re: this patch is whether it will work properly
with sysctl kern.sugid_coredump.
-Matt
Actions #4

Updated by ed about 16 years ago

Hello Matt,

p->p_ucred->cr_uid is equal to the user/group ID of the process right?
I think it will work.

Ed

Actions #5

Updated by dillon about 16 years ago

Ok, I've committed the basic ownership check to the coredump code for
this release. I did verify that sgid dumps still work (when enabled
via the sysctl).

I have not committed the group/other permissions check, at least not
yet. It seems a little excessive considering the access needed to
exploit such a hole is of far greater consequence then any desire to
use it to exploit the core dump path would be.
-Matt
Actions

Also available in: Atom PDF