[PATCH] Finish the task 'Validate coredump format string'

[Tiwei Bie]

On Wed, Mar 25, 2015 at 10:41:30AM +0200, Konstantin Belousov wrote:
> On Wed, Mar 25, 2015 at 02:00:12PM +0800, Tiwei Bie wrote:
> >  share/man/man5/core.5 |  7 ++++
> >  sys/kern/kern_sig.c   | 93 +++++++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 93 insertions(+), 7 deletions(-)
> > 
> > diff --git a/share/man/man5/core.5 b/share/man/man5/core.5
> > index 3f54f89..3047e0a 100644
> > --- a/share/man/man5/core.5
> > +++ b/share/man/man5/core.5
> Please bump date for the man page.
> 

Yeah, I will!

> > @@ -78,6 +78,7 @@ An index starting at zero until the sysctl
> > +
[..]
> > +static int
> > +corefilename_check(const char *format)
> > +{
> > +	int i, countI;
> > +
> > +	countI = 0;
> > +	for (i = 0; i < MAXPATHLEN && format[i] != '\0'; i++) {
> Was it already suggested to use sizeof(corefilename) instead of MAXPATHLEN ?
> 

Sorry, I didn't notice this.

> > +		if (format[i] == '%') {
> > +
[..]
> > +	format = kern_getenv("kern.corefile");
> > +	if (format == NULL)
> > +		return;
> > +
> > +	error = corefilename_check(format);
> > +
> > +	switch (error) {
> > +	case 0:
> > +		strcpy(corefilename, format);
> Use strlcpy(9).
> 

Got it, thanks!

> > +		break;
> > +	case EINVAL:
> > +		printf("Invalid format specified for corename `%s'\n", format);
> > +		break;
> > +	case ENAMETOOLONG:
> > +		printf("The format specified for corename is too long\n");
> > +		break;
> Sigh.  Why kernel should decode the error codes ?  Why should it print
> anything on the console, with non-zero chance of sysctl caller not seeing
> the text ?
> 

I will remove these prints.

My new patch:

---
 share/man/man5/core.5 |  9 +++++-
 sys/kern/kern_sig.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 78 insertions(+), 8 deletions(-)

diff --git a/share/man/man5/core.5 b/share/man/man5/core.5
index 3f54f89..cfbcb1c 100644
--- a/share/man/man5/core.5
+++ b/share/man/man5/core.5
@@ -28,7 +28,7 @@
 .\"     @(#)core.5	8.3 (Berkeley) 12/11/93
 .\" $FreeBSD$
 .\"
-.Dd March 8, 2015
+.Dd March 25, 2015
 .Dt CORE 5
 .Os
 .Sh NAME
@@ -78,6 +78,7 @@ An index starting at zero until the sysctl
 is reached.
 This can be useful for limiting the number of corefiles
 generated by a particular process.
+This specifier can only be specified at most once.
 .It Em \&%N
 process name.
 .It Em \&%P
@@ -91,6 +92,12 @@ The name defaults to
 yielding the traditional
 .Fx
 behaviour.
+When changing the name via the
+.Va kern.corefile
+sysctl, it will fail if the new name contains
+unknown format specifiers, or
+.Em \&%I
+is specified more than once, or its length is too long.
 .Pp
 By default, a process that changes user or group credentials whether
 real or effective will not create a corefile.
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 154c250..611d138 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -3094,21 +3094,85 @@ static int compress_user_cores = 0;
  */
 #define	corefilename_lock	allproc_lock
 
-static char corefilename[MAXPATHLEN] = {"%N.core"};
+static char corefilename[MAXPATHLEN] = "%N.core";
+
+static int
+corefilename_check(const char *format)
+{
+	int i, countI;
+
+	countI = 0;
+	for (i = 0; i < sizeof(corefilename) && format[i] != '\0'; i++) {
+		if (format[i] == '%') {
+			i++;
+			switch (format[i]) {
+			case 'I':
+				countI++;
+				if (countI > 1)
+					return (EINVAL);
+			case '%':
+			case 'H':
+			case 'N':
+			case 'P':
+			case 'U':
+				break;
+			default:
+				return (EINVAL);
+			}
+		}
+	}
+
+	if (i == sizeof(corefilename))
+		return (ENAMETOOLONG);
+
+	return (0);
+}
+
+static void
+corefilename_init(void *arg)
+{
+	char *format;
+
+	format = kern_getenv("kern.corefile");
+	if (format == NULL)
+		return;
+
+	if (corefilename_check(format) == 0)
+		strlcpy(corefilename, format, sizeof(corefilename));
+
+	freeenv(format);
+}
+SYSINIT(corefilename, SI_SUB_KMEM, SI_ORDER_FIRST, corefilename_init, 0);
 
 static int
 sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
 {
+	char *format;
 	int error;
 
+	format = malloc(sizeof(corefilename), M_TEMP, M_WAITOK);
+
+	sx_slock(&corefilename_lock);
+	strlcpy(format, corefilename, sizeof(corefilename));
+	sx_sunlock(&corefilename_lock);
+
+	error = sysctl_handle_string(oidp, format, sizeof(corefilename), req);
+	if (error != 0 || req->newptr == NULL)
+		goto out;
+
+	error = corefilename_check(format);
+	if (error != 0)
+		goto out;
+
 	sx_xlock(&corefilename_lock);
-	error = sysctl_handle_string(oidp, corefilename, sizeof(corefilename),
-	    req);
+	strlcpy(corefilename, format, sizeof(corefilename));
 	sx_xunlock(&corefilename_lock);
 
+out:
+	free(format, M_TEMP);
 	return (error);
 }
-SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN |
+SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RW |
     CTLFLAG_MPSAFE, 0, 0, sysctl_kern_corefile, "A",
     "Process corefile name format string");
 
@@ -3171,9 +3235,8 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td,
 				sbuf_printf(&sb, "%u", uid);
 				break;
 			default:
-				log(LOG_ERR,
-				    "Unknown format character %c in "
-				    "corename `%s'\n", format[i], format);
+				KASSERT(0, ("Unknown format character %c in "
+				    "corename `%s'", format[i], format));
 				break;
 			}
 			break;
-- 
2.1.2

Best regards,
Tiwei Bie