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

[Konstantin Belousov]

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.

> @@ -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..7fdb079 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -3094,21 +3094,101 @@ 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 < MAXPATHLEN && format[i] != '\0'; i++) {
Was it already suggested to use sizeof(corefilename) instead of MAXPATHLEN ?

> +		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 == MAXPATHLEN)
> +		return (ENAMETOOLONG);
> +
> +	return (0);
> +}
> +
> +static void
> +corefilename_init(void *arg)
> +{
> +	char *format;
> +	int error;
> +
> +	format = kern_getenv("kern.corefile");
> +	if (format == NULL)
> +		return;
> +
> +	error = corefilename_check(format);
> +
> +	switch (error) {
> +	case 0:
> +		strcpy(corefilename, format);
Use strlcpy(9).

> +		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 ?

> +	default:
> +		KASSERT(0, ("%s: unknown return value %d from "
> +		    "corefilename_check", __func__, error));
> +		break;
> +	}
> +
> +	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(MAXPATHLEN, M_TEMP, M_WAITOK);
> +
> +	sx_slock(&corefilename_lock);
> +	strcpy(format, corefilename);
Again, use strlcpy.  It works correctly, comparing with strncpy.

> +	sx_sunlock(&corefilename_lock);
> +
> +	error = sysctl_handle_string(oidp, format, MAXPATHLEN, 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);
> +	strcpy(corefilename, format);
And there.

>  	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 +3251,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