Re: [PATCH] Fix kernel stack data disclosure in binfmt_script during execve

[Randy Dunlap <rdunlap@xenotime.net>]

On 09/20/2012 09:05 AM, halfdog wrote:

> halfdog wrote:
> 
> Now this is the updated and also tested patch (vs. linux-3.5.4 kernel) to fix
> https://bugzilla.kernel.org/show_bug.cgi?id=46841 . See also
> http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
> This patch adresses the stack data disclosure but does not deal with the
> excessive recursion (to be handled in separate patch if needed).
> 
> --- fs/binfmt_script.c	2012-09-14 22:28:08.000000000 +0000
> +++ fs/binfmt_script.c	2012-09-20 16:01:58.951942355 +0000


Incorrect diff/patch format for kernel patches.
It should be apply-able by using 'patch -p1'.

Oh, the patch is not signed off.
And Documentation/SubmittingPatches says signoffs are:

"using your real name (sorry, no pseudonyms or anonymous contributions.)"

There are also some kernel coding style issues that should be
fixed if this patch is to be merged.

> @@ -14,12 +14,24 @@
>  #include <linux/err.h>
>  #include <linux/fs.h>
> 
> +/** Check if this handler is suitable to load the "binary" identified


/** means kernel-doc notation in the kernel, but this comment
block is not kernel-doc, so don't start it with /**

> + *  by first BINPRM_BUF_SIZE bytes in bprm->buf.
> + *  @returns -ENOEXEC if this handler is not suitable for that type


We don't use "@returns", just returns: <text>.

> + *  of binary. In that case, the handler must not modify any of the
> + *  data associated with bprm.
> + *  Any error if the binary should have been handled by this loader
> + *  but handling failed. In that case. FIXME: be defensive? also
> + *  kill bprm->mm or bprm->file also to make it impossible, that
> + *  upper search_binary_handler can continue handling?
> + *  0 (OK) otherwise, the new executable is ready in bprm->mm.
> + */
>  static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
>  {
>  	const char *i_arg, *i_name;
>  	char *cp;
>  	struct file *file;
> -	char interp[BINPRM_BUF_SIZE];
> +	char bprm_buf_copy[BINPRM_BUF_SIZE];
> +	const char *bprm_old_interp_name;
>  	int retval;
> 
>  	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
> @@ -30,25 +42,29 @@ static int load_script(struct linux_binp
>  	 * Sorta complicated, but hopefully it will work.  -TYT
>  	 */
> 
> -	bprm->recursion_depth++;
> -	allow_write_access(bprm->file);
> -	fput(bprm->file);
> -	bprm->file = NULL;
> +	/* Keep bprm unchanged until we known, that this is a script
> +	 * to be handled by this loader. Copy bprm->buf for sure,
> +	 * otherwise returning -ENOEXEC will make other handlers see
> +	 * modified data. (hd)
> +	 */


Kernel multi-line comment style is
	/*
	 * line 1
	 * line 2
	 */

> +	memcpy(bprm_buf_copy, bprm->buf, BINPRM_BUF_SIZE);
> 
> -	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
> -	if ((cp = strchr(bprm->buf, '\n')) == NULL)
> -		cp = bprm->buf+BINPRM_BUF_SIZE-1;
> +	bprm_buf_copy[BINPRM_BUF_SIZE - 1]='\0';
> +	if ((cp = strchr(bprm_buf_copy, '\n')) == NULL)
> +		cp = bprm_buf_copy+BINPRM_BUF_SIZE-1;
>  	*cp = '\0';
> -	while (cp > bprm->buf) {
> +	while (cp > bprm_buf_copy) {
>  		cp--;
>  		if ((*cp == ' ') || (*cp == '\t'))
>  			*cp = '\0';
>  		else
>  			break;
>  	}
> -	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
> +	for (cp = bprm_buf_copy+2; (*cp == ' ') || (*cp == '\t'); cp++);
>  	if (*cp == '\0')
> -		return -ENOEXEC; /* No interpreter name found */
> +	/* No interpreter name found. No problem to let other handlers
> +	 * retry, we did not change anything. */


multi-line comment style.

> +		return -ENOEXEC;
>  	i_name = cp;
>  	i_arg = NULL;
>  	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
> @@ -57,45 +73,84 @@ static int load_script(struct linux_binp
>  		*cp++ = '\0';
>  	if (*cp)
>  		i_arg = cp;
> -	strcpy (interp, i_name);
> +
> +	/* So this is our point-of-no-return: modification of bprm
> +	 * will be irreversible, so if we fail to setup execution
> +	 * using the new interpreter name (i_name), we have to make
> +	 * sure, that no other handler tries again. (hd)
> +	 */


ditto.

> +
>  	/*
>  	 * OK, we've parsed out the interpreter name and
>  	 * (optional) argument.
>  	 * Splice in (1) the interpreter's name for argv[0]
> -	 *           (2) (optional) argument to interpreter
> -	 *           (3) filename of shell script (replace argv[0])
> +	 *	   (2) (optional) argument to interpreter
> +	 *	   (3) filename of shell script (replace argv[0])
>  	 *
>  	 * This is done in reverse order, because of how the
>  	 * user environment and arguments are stored.
>  	 */
> +
> +	/* Ugly: we store pointer to local stack frame in bprm,
> +	 * so make sure to clear this up before returning.
> +	 */


ditto.

> +	bprm_old_interp_name = bprm->interp;
> +	bprm->interp = i_name;
> +
>  	retval = remove_arg_zero(bprm);
> -	if (retval)
> -		return retval;
> -	retval = copy_strings_kernel(1, &bprm->interp, bprm);
> -	if (retval < 0) return retval;
> +	if (retval) goto out;


Really should be
	if (retval)
		goto out;

> +	/* copy_strings_kernel is ok here, even when racy: since no
> +	 * user can be attached to new mm, there is nobody to race
> +	 * with and call is safe for now. The return code of
> +	 * copy_strings_kernel cannot be -ENOEXEC in any case,
> +	 * so no special checks needed. (hd)
> +	 */


style.

> +	retval = copy_strings_kernel(1, &bprm_old_interp_name, bprm);
> +	if (retval < 0) goto out;
>  	bprm->argc++;
>  	if (i_arg) {
>  		retval = copy_strings_kernel(1, &i_arg, bprm);
> -		if (retval < 0) return retval;
> +		if (retval < 0) goto out;


style.

>  		bprm->argc++;
>  	}
> -	retval = copy_strings_kernel(1, &i_name, bprm);
> -	if (retval) return retval;
> +	retval = copy_strings_kernel(1, &bprm->interp, bprm);
> +	if (retval) goto out;


style.  (but Al can override these if he wants to)

>  	bprm->argc++;
> -	bprm->interp = interp;
> 
>  	/*
>  	 * OK, now restart the process with the interpreter's dentry.
> +         * Release old file first


indentation mucked up.

>  	 */
> -	file = open_exec(interp);
> -	if (IS_ERR(file))
> -		return PTR_ERR(file);
> -
> +	allow_write_access(bprm->file);
> +	fput(bprm->file);
> +	bprm->file = NULL;
> +	file = open_exec(bprm->interp);
> +	if (IS_ERR(file)) {
> +		retval=PTR_ERR(file);
> +		goto out;
> +        }
>  	bprm->file = file;
> +	/* Caveat: This also updates the credentials of the next exec. */
>  	retval = prepare_binprm(bprm);
>  	if (retval < 0)
> -		return retval;
> -	return search_binary_handler(bprm,regs);
> +		goto out;
> +	bprm->recursion_depth++;
> +	retval=search_binary_handler(bprm,regs);
> +
> +out:	/* Make sure, we do not return local stack frame data. If
> +	 * it would be needed after returning, we would have needed
> +	 * to allocate memory or use copy from new bprm->mm anyway. (hd)
> +         */


Comment block probably should come before the label.
and the indentation is mucked up.

> +	bprm->interp = bprm_old_interp_name;
> +	if(!retval) {
> +		/* The handlers for starting of interpreter failed.
> +		 * bprm is already modified, hence we are dead here.
> +		 * Make sure, that we do not return -ENOEXEC, that would
> +		 * allow searching for handlers to continue. (hd).
> +		 */

style

> +		if(retval==-ENOEXEC) retval=-EINVAL;


missing space before '('.
etc.

> +	}
> +	return(retval);
>  }
> 
>  static struct linux_binfmt script_format = {
> 



-- 
~Randy