Bug #950

Coredumping design error

Added by ed almost 7 years ago. Updated over 6 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

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
# ./coredumper
Segmentation fault (core dumped)
syslog: Feb 16 09:41:14 kernel: pid 728 (coredumper), uid 0: exited
on signal 11 (core dumped)
# md5 coredumper.core
MD5 (coredumper.core) = 68e3e5fee874e688c795537721a6b511
# 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>

History

#1 Updated by corecode almost 7 years ago

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

cheers
simon

#2 Updated by ed almost 7 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
> > # ./coredumper
> > Segmentation fault (core dumped)
> > syslog: Feb 16 09:41:14 kernel: pid 728 (coredumper), uid 0: exited
> > on signal 11 (core dumped)
> > # md5 coredumper.core
> > MD5 (coredumper.core) = 68e3e5fee874e688c795537721a6b511
> > # 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
>
>

#3 Updated by dillon almost 7 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

#4 Updated by ed almost 7 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

#5 Updated by dillon almost 7 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

Also available in: Atom PDF