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:

NetBSD Home
NetBSD PR Database Search

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