Re: Search for patch for kernel stack data disclosure in binfmt_scriptduring execve

[halfdog]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

halfdog wrote:
> I'm searching for a patch for linux kernel stack disclosure in 
> binfmt_script with crafted interpreter names when CONFIG_MODULES
> is active (see [1]).

Please disregard my previous proposal [2], since it did not address
the problem directly (referencing local stack frame data from bprm
structure) but worked around it. I suspect, that this could increase
probability to reintroduce similar bugs.

Opinions on (untested sketch for) second solution: Could someone look
on the source code comments and changes in patch to judge, if this is
going in the right direction?


Explanation of patch: Since load_script will start to irreversibly
change bprm structures at some point (using stack local data was one
of those changes), try to delay this point. Run checks if load_script
could be the right handler, if not give other binfmt handlers the
chance to do so.

If binfmt_script is the right one, try to load the interpreter
(causing bprm modification), if failing make sure that no other binfmt
handler has the chance to continue on the now modified bprm data.

CAVEAT: This assumes, that if binfmt_script could handle the load,
that it would be the one and only binfmt with that ability, so no
other one, e.g. binfmt_misc should have the chance to do so. If this
assumption is wrong, leaving binfmt_script would have to rollback all
bprm changes (e.g. restore old credentials).

hd

[1]
http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
[2] http://lkml.org/lkml/2012/8/18/75

- -- 
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88  2BD8 C459 9386 feed a bee
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAlAwphsACgkQxFmThv7tq+6UAQCgh7IA8UcqNieV41YKHS5/YxGE
IbcAn1uP1nIakg/gD1KlV0KNnLIfitEp
=5Klt
-----END PGP SIGNATURE-----
--- fs/binfmt_script.c	2012-01-19 23:04:48.000000000 +0000
+++ fs/binfmt_script.c	2012-08-19 07:08:42.540611605 +0000
@@ -14,12 +14,24 @@
 #include <linux/err.h>
 #include <linux/fs.h>
 
+/** Check if this handler is suitable to load the "binary" identified
+ *  by first BINPRM_BUF_SIZE bytes in bprm->buf.
+ *  @returns -ENOEXEC if this handler is not suitable for that type
+ *  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];
+	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)
+	 */
+	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. */
+		return -ENOEXEC;
 	i_name = cp;
 	i_arg = NULL;
 	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
@@ -57,45 +73,83 @@ 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)
+	 */
+
 	/*
 	 * 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.
+	 */
+	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;
+	/* 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. And copy_strings_kernel
+	 * cannot return -ENOEXEC in any case. (hd)
+	 */
+	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;
 		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;
 	bprm->argc++;
-	bprm->interp = interp;
 
 	/*
 	 * OK, now restart the process with the interpreter's dentry.
+         * Release old file first
 	 */
-	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)
+         */
+	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).
+		 */
+		if(retval==-ENOEXEC) retval=-EINVAL;
+	}
+	return(retval);
 }
 
 static struct linux_binfmt script_format = {