NetBSD Problem Report #45393
From woods@once.weird.com Thu Sep 22 22:07:40 2011
Return-Path: <woods@once.weird.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
by www.NetBSD.org (Postfix) with ESMTP id 30C5E63B884
for <gnats-bugs@gnats.NetBSD.org>; Thu, 22 Sep 2011 22:07:40 +0000 (UTC)
Message-Id: <m1R6rQc-001EBeC@once.weird.com>
Date: Thu, 22 Sep 2011 15:07:34 -0700 (PDT)
From: "Greg A. Woods" <woods@planix.com>
Sender: "Greg A. Woods" <woods@once.weird.com>
Reply-To: "Greg A. Woods" <woods@planix.com>
To: gnats-bugs@gnats.NetBSD.org
Subject: core dumps are unilaterally prevented by unmounted cwd or MNT_NOCOREDUMP even if corename will be valid
X-Send-Pr-Version: 3.95
>Number: 45393
>Category: kern
>Synopsis: core dumps are unilaterally prevented by unmounted cwd or MNT_NOCOREDUMP even if corename will be valid
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: christos
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Thu Sep 22 22:10:01 +0000 2011
>Closed-Date: Sat Dec 19 02:28:29 +0000 2015
>Last-Modified: Sat Dec 19 02:28:29 +0000 2015
>Originator: Greg A. Woods
>Release: netbsd-5
>Organization:
Planix, Inc.; Kelowna, BC; Canada
>Environment:
System: NetBSD 5.2_STABLE
Architecture:
Machine:
>Description:
A question emailed to me about one of the validations done in
coredump() prompted me to note a potential problem which would
prevent core dumps from happening in a number of circumstances
even if those checks were not valid for the ultimate destination
of the core file. The conundrum was already mentioned in part
in a comment in the code, but nothing had been done about it
then.
>How-To-Repeat:
by reading the code, and comments
>Fix:
The following diff is from the netbsd-5 branch and also includes
some un-related changes to the coredump() API.
Instead of trying to edit the diff, or trying to re-apply just
the necessary changes to -current where I cannot test them at
the moment, I'll just show the whole diff here.
The changes relevant to this PR are in the 5'th and 6'th hunks.
The second hunk also updates the opening comment to better
reflect the current state of the code, something that should
have been done quite some time ago when the code was first
changed to make it incompatible with the comment.
--- kern_core.c 24 Apr 2008 11:39:23 -0700 1.12
+++ kern_core.c 22 Sep 2011 11:16:56 -0700
@@ -55,7 +55,7 @@
#if !defined(COREDUMP)
int
-coredump(struct lwp *l, const char *pattern)
+coredump(struct lwp *l, const char *pattern, char *corename)
{
return (ENOSYS);
@@ -73,11 +73,14 @@
static int coredump_buildname(struct proc *, char *, const char *, size_t);
/*
- * Dump core, into a file named "progname.core" or "core" (depending on the
- * value of shortcorename), unless the process was setuid/setgid.
+ * Dump core, into a file name created from 'pattern' (unless the process was
+ * setuid/setgid, in which case if security_setidcore_dump is also true then
+ * security_setidcore_path is used as the file name pattern).
+ *
+ * XXX maybe logging should go here instead of all callers? (need better API)
*/
int
-coredump(struct lwp *l, const char *pattern)
+coredump(struct lwp *l, const char *pattern, char *corename)
{
struct vnode *vp;
struct proc *p;
@@ -91,6 +94,10 @@
char *name;
name = PNBUF_GET();
+ if (name == NULL) {
+ error = ENOMEM;
+ goto done;
+ }
p = l->l_proc;
vm = p->p_vmspace;
@@ -105,7 +112,7 @@
*/
if (USPACE + ctob(vm->vm_dsize + vm->vm_ssize) >=
p->p_rlimit[RLIMIT_CORE].rlim_cur) {
- error = EFBIG; /* better error code? */
+ error = EFBIG;
mutex_exit(p->p_lock);
mutex_exit(proc_lock);
goto done;
@@ -119,35 +126,17 @@
cred = p->p_cred;
/*
- * The core dump will go in the current working directory. Make
- * sure that the directory is still there and that the mount flags
- * allow us to write core dumps there.
- *
- * XXX: this is partially bogus, it should be checking the directory
- * into which the file is actually written - which probably needs
- * a flag on namei()
- */
- vp = p->p_cwdi->cwdi_cdir;
- if (vp->v_mount == NULL ||
- (vp->v_mount->mnt_flag & MNT_NOCOREDUMP) != 0) {
- error = EPERM;
- mutex_exit(p->p_lock);
- mutex_exit(proc_lock);
- goto done;
- }
-
- /*
* Make sure the process has not set-id, to prevent data leaks,
* unless it was specifically requested to allow set-id coredumps.
*/
if (p->p_flag & PK_SUGID) {
if (!security_setidcore_dump) {
- error = EPERM;
+ error = EAUTH;
mutex_exit(p->p_lock);
mutex_exit(proc_lock);
goto done;
}
- pattern = security_setidcore_path;
+ pattern = security_setidcore_path; /* is permitted to be NULL */
}
/* It is (just) possible for p_limit and pl_corename to change */
@@ -155,12 +144,44 @@
mutex_enter(&lim->pl_lock);
if (pattern == NULL)
pattern = lim->pl_corename;
+ KASSERT(pattern); /* otherwise name is "", and what will vn_open() do? */
error = coredump_buildname(p, name, pattern, MAXPATHLEN);
mutex_exit(&lim->pl_lock);
+
+ /*
+ * IFF the final name for the core file is relative to the current
+ * working directory, then make sure that the directory is still there
+ * and that the mount flags allow us to write core dumps there.
+ *
+ * Note that there is no check to see if the partition where a fully
+ * qualified core name will be created allows core dumps or not
+ * (i.e. there is no check to make sure that it does not have the
+ * MNT_NOCOREDUMP flag set).
+ *
+ * Presumably the administrator who set the pathname in
+ * lim->pl_corename, or who specified the path in the ptrace() call,
+ * wants to ignore the MNT_NOCOREDUMP flag in this case, and is
+ * authorized to do so.
+ *
+ * This allows core dumps to generally be prevented on a system with
+ * only a single filesystem, unless they are explicitly directed to a
+ * specified location.
+ */
+ if (name[0] != '/') {
+ vp = p->p_cwdi->cwdi_cdir;
+ if (vp->v_mount == NULL ||
+ (vp->v_mount->mnt_flag & MNT_NOCOREDUMP) != 0) {
+ error = ENXIO;
+ mutex_exit(p->p_lock);
+ mutex_exit(proc_lock);
+ goto done;
+ }
+ }
mutex_exit(p->p_lock);
mutex_exit(proc_lock);
if (error)
goto done;
+
NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_SYSSPACE, name);
if ((error = vn_open(&nd, O_CREAT | O_NOFOLLOW | FWRITE,
S_IRUSR | S_IWUSR)) != 0)
@@ -198,8 +219,11 @@
if (error == 0)
error = error1;
done:
- if (name != NULL)
+ if (name != NULL) {
+ if (corename)
+ strcpy(corename, name);
PNBUF_PUT(name);
+ }
return error;
}
>Release-Note:
>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/45393 CVS commit: src/sys/kern
Date: Thu, 22 Sep 2011 20:03:29 -0400
Module Name: src
Committed By: christos
Date: Fri Sep 23 00:03:29 UTC 2011
Modified Files:
src/sys/kern: kern_core.c
Log Message:
PR/45393: Greg A. Woods: The mount point validation code (that looks for
nocoredump filesystems to avoid dumping on them) only worked for core
filenames that dump in the current working directory. Update the code to
validate the mount point of the parent directory of the core file if needed.
To generate a diff of this commit:
cvs rdiff -u -r1.18 -r1.19 src/sys/kern/kern_core.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Responsible-Changed-From-To: kern-bug-people->christos
Responsible-Changed-By: wiz@NetBSD.org
Responsible-Changed-When: Fri, 23 Sep 2011 09:01:05 +0000
Responsible-Changed-Why:
christos committed something.
State-Changed-From-To: open->feedback
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Fri, 23 Sep 2011 09:01:05 +0000
State-Changed-Why:
Ok to close?
From: "Greg A. Woods" <woods@planix.com>
To: NetBSD GNATS <gnats-bugs@NetBSD.org>
Cc: <christos@NetBSD.org>,
NetBSD Kernel Bug People <kern-bug-people@netbsd.org>,
<wiz@NetBSD.org>
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted cwd or MNT_NOCOREDUMP even if corename will be valid)
Date: Fri, 23 Sep 2011 17:18:11 -0700
--pgp-sign-Multipart_Fri_Sep_23_17:18:10_2011-1
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
At Fri, 23 Sep 2011 09:01:06 +0000 (UTC), wiz@NetBSD.org wrote:
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted=
cwd or MNT_NOCOREDUMP even if corename will be valid)
>=20
> Ok to close?
What Christos committed isn't exactly what I had in mind... :-)
It is now strictly obeying MNT_NOCOREDUMP, but given the prevalence of
single-filesystem systems, I think what I suggested makes more sense,
i.e. only honour MNT_NOCOREDUMP if the pathname is relative, thus
allowing the administrator to set an absolute pathname for one or more
processes (or even sometimes all of them via kern.defcorename, though
that choice obviates the need for setting MNT_NOCOREDUMP in the first
place, except as a second line of defence from accidental core
pollution) to allow them to dump core in a given location, and also
allowing a user to get a core image from their own processes upon
specific request, but otherwise preventing processes from normally
dumping in any old CWD.
However, on second look I realize I have not yet fully explored all the
potential problems with ptrace(). I think it's safe the way I wrote it,
but I cannot yet prove it.
If so then I guess the question is whether MNT_NOCOREDUMP should reign
supreme, or whether there should be a way around it for special cases.
Personally I think that if I as the superuser set proc.blah.corename (or
kern.defcorename) to an absolute pathname then I want core dumps to
happen even if I'm silly enough to also have MNT_NOCOREDUMP set on the
filesystem for that location.
I think users should also be able to use ptrace() or proc.*.corename to
force a core dump to an absolute pathname (where they have sufficient
privilege) regardless of whether the admin has set things up to
generally prevent cores from dropping in random CWDs anywhere and
everywhere.
Indeed I think MNT_NOCOREDUMP is basically a throwback to before there
was a way to set an absolute pathname for core files and that it could
be at least reduced in importance in the way I suggest, if not
deprecated entirely.
Perhaps this should be discussed with a wider audience?
--=20
Greg A. Woods
+1 250 762-7675 RoboHack <woods@robohack.ca>
Planix, Inc. <woods@planix.com> Secrets of the Weird <woods@weird.com>
--pgp-sign-Multipart_Fri_Sep_23_17:18:10_2011-1
Content-Type: application/pgp-signature
Content-Transfer-Encoding: 7bit
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (NetBSD)
iD8DBQBOfSHCZn1xt3i/9H8RAsi3AJ4iCuPv7qJ6aroTHez+yyZCB+PYdQCdHXDe
Zxg7j8MMckwWqjXu5L/AYls=
=ng3z
-----END PGP SIGNATURE-----
--pgp-sign-Multipart_Fri_Sep_23_17:18:10_2011-1--
From: christos@zoulas.com (Christos Zoulas)
To: "Greg A. Woods" <woods@planix.com>,
NetBSD GNATS <gnats-bugs@NetBSD.org>
Cc: NetBSD Kernel Bug People <kern-bug-people@netbsd.org>, <wiz@NetBSD.org>
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted cwd or MNT_NOCOREDUMP even if corename will be valid)
Date: Fri, 23 Sep 2011 20:26:08 -0400
On Sep 23, 5:18pm, woods@planix.com ("Greg A. Woods") wrote:
-- Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounte
| What Christos committed isn't exactly what I had in mind... :-)
:-)
| It is now strictly obeying MNT_NOCOREDUMP, but given the prevalence of
| single-filesystem systems, I think what I suggested makes more sense,
| i.e. only honour MNT_NOCOREDUMP if the pathname is relative, thus
| allowing the administrator to set an absolute pathname for one or more
| processes (or even sometimes all of them via kern.defcorename, though
| that choice obviates the need for setting MNT_NOCOREDUMP in the first
| place, except as a second line of defence from accidental core
| pollution) to allow them to dump core in a given location, and also
| allowing a user to get a core image from their own processes upon
| specific request, but otherwise preventing processes from normally
| dumping in any old CWD.
Well, I can ../../ my way out to anywhere then, so it is not a lot of help.
If I am allowed to set my core-name that is. I tought about the single
filesystem issue, and I guess you can work around the issue with either
a vnd mount or a loopback one. I think that it is better than an additional
flag or weaken MNT_NOCOREDUMP.
| However, on second look I realize I have not yet fully explored all the
| potential problems with ptrace(). I think it's safe the way I wrote it,
| but I cannot yet prove it.
Yes, ptrace() is problematic :-)
| If so then I guess the question is whether MNT_NOCOREDUMP should reign
| supreme, or whether there should be a way around it for special cases.
|
| Personally I think that if I as the superuser set proc.blah.corename (or
| kern.defcorename) to an absolute pathname then I want core dumps to
| happen even if I'm silly enough to also have MNT_NOCOREDUMP set on the
| filesystem for that location.
|
| I think users should also be able to use ptrace() or proc.*.corename to
| force a core dump to an absolute pathname (where they have sufficient
| privilege) regardless of whether the admin has set things up to
| generally prevent cores from dropping in random CWDs anywhere and
| everywhere.
What if the admin does not want cores anywhere?
| Indeed I think MNT_NOCOREDUMP is basically a throwback to before there
| was a way to set an absolute pathname for core files and that it could
| be at least reduced in importance in the way I suggest, if not
| deprecated entirely.
It is still a good blunt tool that was broken for a while.
| Perhaps this should be discussed with a wider audience?
Sure, let's do it. Perhaps others have better ideas.
christos
From: "Greg A. Woods" <woods@planix.com>
To: NetBSD GNATS <gnats-bugs@NetBSD.org>
Cc:
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted cwd or MNT_NOCOREDUMP even if corename will be valid)
Date: Sat, 24 Sep 2011 15:14:44 -0700
--pgp-sign-Multipart_Sat_Sep_24_15:14:44_2011-1
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
At Sat, 24 Sep 2011 00:30:04 +0000 (UTC), christos@zoulas.com (Christos Zou=
las) wrote:
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted=
cwd or MNT_NOCOREDUMP even if corename will be valid)
>=20
> Well, I can ../../ my way out to anywhere then, so it is not a lot of he=
lp.
> If I am allowed to set my core-name that is. I tought about the single
> filesystem issue, and I guess you can work around the issue with either
> a vnd mount or a loopback one. I think that it is better than an additio=
nal
> flag or weaken MNT_NOCOREDUMP.
After doing a little more research on the origins of MNT_NOCOREDUMP
(first by cgd, in NetBSD, in 1996, so far as I can tell) I'm now a lot
less inclined to worry about the single filesystem issue I initially
raised.
Indeed, as you said, if the admin doesn't want any core dumps then
MNT_NOCOREDUMP is the best way to ensure that (or at least it will be
after your fixes are in a release :-)).
When I originally encountered the "nocoredump" option I looked to it
more as a way to prevent pollution of core files in random locations,
not as the security mechanism as it is described in mount(8).
However my personal goal is now met by both the logging of core dumps
(at least with my patch to log the directory where the core is created),
and the ability to contain them all to one sub-directory by giving
kern.defcorename a fully qualified pathname template.
So, with that said I'd say yes, please close this PR (though perhaps
your final fix deserves a pull-up to netbsd-5?)
As a side note I find it interesting that not even OpenBSD has
implemented MNT_NOCOREDUMP. In fact I don't find it anywhere other than
in NetBSD.
Oh, and one more partly related thing my research revealed: OpenBSD
added a check in 2007 to prevent a core from overwriting a file owned by
a different user (their kern_sig.c rev. 1.96). I think NetBSD should
gain this check as well, but perhaps it deserves a separate PR?
--=20
Greg A. Woods
+1 250 762-7675 RoboHack <woods@robohack.ca>
Planix, Inc. <woods@planix.com> Secrets of the Weird <woods@weird.com>
--pgp-sign-Multipart_Sat_Sep_24_15:14:44_2011-1
Content-Type: application/pgp-signature
Content-Transfer-Encoding: 7bit
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (NetBSD)
iD8DBQBOflZUZn1xt3i/9H8RApveAJ9q1THJ16tQQWXkgs6eBtK2bqlJ6gCfaYr1
/jcBnbdBdcMsEqvSr1CDK6Q=
=uHuJ
-----END PGP SIGNATURE-----
--pgp-sign-Multipart_Sat_Sep_24_15:14:44_2011-1--
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
"Greg A. Woods" <woods@planix.com>
Cc:
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted cwd or MNT_NOCOREDUMP even if corename will be valid)
Date: Sat, 24 Sep 2011 18:55:18 -0400
On Sep 24, 10:15pm, woods@planix.com ("Greg A. Woods") wrote:
-- Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounte
| After doing a little more research on the origins of MNT_NOCOREDUMP
| (first by cgd, in NetBSD, in 1996, so far as I can tell) I'm now a lot
| less inclined to worry about the single filesystem issue I initially
| raised.
|
| Indeed, as you said, if the admin doesn't want any core dumps then
| MNT_NOCOREDUMP is the best way to ensure that (or at least it will be
| after your fixes are in a release :-)).
|
| When I originally encountered the "nocoredump" option I looked to it
| more as a way to prevent pollution of core files in random locations,
| not as the security mechanism as it is described in mount(8).
|
| However my personal goal is now met by both the logging of core dumps
| (at least with my patch to log the directory where the core is created),
| and the ability to contain them all to one sub-directory by giving
| kern.defcorename a fully qualified pathname template.
Great.
| So, with that said I'd say yes, please close this PR (though perhaps
| your final fix deserves a pull-up to netbsd-5?)
Well, that is going to be a bit difficult because 5 is missing the simple
namei() function that I am using :-)
| As a side note I find it interesting that not even OpenBSD has
| implemented MNT_NOCOREDUMP. In fact I don't find it anywhere other than
| in NetBSD.
|
I have not seen it either.
| Oh, and one more partly related thing my research revealed: OpenBSD
| added a check in 2007 to prevent a core from overwriting a file owned by
| a different user (their kern_sig.c rev. 1.96). I think NetBSD should
| gain this check as well, but perhaps it deserves a separate PR?
I've added it too, thanks for mentioning it.
christos
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: christos@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
"Greg A. Woods" <woods@planix.com>
Subject: Re: kern/45393 (core dumps are unilaterally prevented by unmounted
cwd or MNT_NOCOREDUMP even if corename will be valid)
Date: Wed, 28 Sep 2011 22:44:00 +0000
On Sat, Sep 24, 2011 at 11:00:09PM +0000, Christos Zoulas wrote:
> | So, with that said I'd say yes, please close this PR (though perhaps
> | your final fix deserves a pull-up to netbsd-5?)
>
> Well, that is going to be a bit difficult because 5 is missing the simple
> namei() function that I am using :-)
That's not that big a deal; you can just use the expansion of
namei_simple* from vfs_lookup.c and get rid of the pathbuf logic. The
main point of those functions is to isolate struct nameidata and
NDINIT from random callers that don't need to fiddle with them
directly.
admittedly it's not 100% trivial, but I think maybe it's worthwhile,
so I'll leave the PR open for now until I or someone gets around to it.
--
David A. Holland
dholland@netbsd.org
State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 30 Apr 2012 22:54:05 +0000
State-Changed-Why:
Got feedback in September (I apparently missed it, or it's part of
my mail backlog I haven't got to yet) and the conclusion is: yes,
it's fixed, but one of the fixes should be pulled up to -5. That
hasn't been done yet.
State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 19 Dec 2015 02:28:29 +0000
State-Changed-Why:
-5 is EOL; fixed in -6 and up
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.