When binfmt_script's load_script ran, it would manipulate bprm->buf and
leave bprm->interp pointing to the local stack. If a series of scripts
are executed, the final one will have leaked kernel stack contents into
the cmdline. For a proof of concept, see DoTest.sh from:
http://www.halfdog.net/Security/2012/LinuxKernelBinfmtScriptStackDataDisclosure/
Largely based on a patch by halfdog. Cleaned up various style issues,
including those reported by Randy Dunlap and scripts/checkpatch.pl.
Cc: halfdog <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
For more background, see the earlier thread:
https://lkml.org/lkml/2012/8/18/75
---
fs/binfmt_script.c | 126 +++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 101 insertions(+), 25 deletions(-)
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index d3b8c1f..15fe9e8 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -14,12 +14,22 @@
#include <linux/err.h>
#include <linux/fs.h>
+/*
+ * Check if this handler is suitable to load the interpreter identified
+ * by first BINPRM_BUF_SIZE bytes in bprm->buf following "#!".
+ *
+ * Returns:
+ * 0: success; the new executable is ready in bprm->mm.
+ * -ve: interpreter not found, or other binfmts failed to find a
+ * suitable binary.
+ */
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,34 +40,57 @@ static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
* 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.
+ */
+ 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;
+ /* Locate and truncate end of string. */
+ bprm_buf_copy[BINPRM_BUF_SIZE - 1] = '\0';
+ cp = strchr(bprm_buf_copy, '\n');
+ if (cp == NULL)
+ cp = bprm_buf_copy + BINPRM_BUF_SIZE - 1;
*cp = '\0';
- while (cp > bprm->buf) {
+ /* Truncate trailing white-space. */
+ while (cp > bprm_buf_copy) {
cp--;
if ((*cp == ' ') || (*cp == '\t'))
*cp = '\0';
else
break;
}
- for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
+ /* Skip leading white-space. */
+ for (cp = bprm_buf_copy + 2; (*cp == ' ') || (*cp == '\t'); cp++)
+ /* nothing */ ;
+
+ /*
+ * No interpreter name found? No problem to let other handlers
+ * retry, we did not change anything.
+ */
if (*cp == '\0')
- return -ENOEXEC; /* No interpreter name found */
+ return -ENOEXEC;
+
i_name = cp;
i_arg = NULL;
+ /* Find start of first argument. */
for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
/* nothing */ ;
+ /* Truncate and skip leading white-space. */
while ((*cp == ' ') || (*cp == '\t'))
*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.
+ */
+
/*
* OK, we've parsed out the interpreter name and
* (optional) argument.
@@ -68,34 +101,77 @@ static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
* 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 clean 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;
+ 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.
+ */
+ 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 a copy from new bprm->mm anyway.
+ */
+ bprm->interp = bprm_old_interp_name;
+ /*
+ * Since bprm is already modified, we cannot continue if the the
+ * handlers for starting the new interpreter have failed.
+ * Make sure that we do not return -ENOEXEC, as that would
+ * allow searching for handlers to continue.
+ */
+ if (retval == -ENOEXEC)
+ retval = -EINVAL;
+
+ return retval;
}
static struct linux_binfmt script_format = {
--
1.7.9.5
--
Kees Cook
Chrome OS Security
On Thu, Oct 11, 2012 at 07:32:40PM -0700, Kees Cook wrote:
> + /*
> + * Since bprm is already modified, we cannot continue if the the
> + * handlers for starting the new interpreter have failed.
> + * Make sure that we do not return -ENOEXEC, as that would
> + * allow searching for handlers to continue.
> + */
> + if (retval == -ENOEXEC)
> + retval = -EINVAL;
After looking at this some more, I wonder if this should be -ELOOP
instead? Or maybe that should happen if/when the recursion depth problem is
fixed?
This is much more obvious, instead of "Invalid argument":
$ ./dotest.sh
file-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAfile-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA: bad interpreter: Too many levels of symbolic links
More importantly, I also wonder if interp handling to just be
changed to be an allocation that needs to be cleaned up, as done with
argv? Right now interp just points to the filename argument handed to
do_execve. Especially since it looks like binfmt_misc is vulnerable
to this as well, since it runs the risk of getting -ENOEXEC from
search_binary_handler, leaving bprm->interp pointing into the stack,
only to get it recalled after module loading attempts:
static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
{
...
char iname[BINPRM_BUF_SIZE];
...
bprm->interp = iname; /* for binfmt_script */
...
retval = search_binary_handler (bprm, regs);
if (retval < 0)
goto _error;
...
_ret:
return retval;
_error:
if (fd_binary > 0)
sys_close(fd_binary);
bprm->interp_flags = 0;
bprm->interp_data = 0;
goto _ret;
}
:(
-Kees
--
Kees Cook
Chrome OS Security
Kees Cook wrote:
> On Thu, Oct 11, 2012 at 07:32:40PM -0700, Kees Cook wrote:
>> + /*
>> + * Since bprm is already modified, we cannot continue if the the
>> + * handlers for starting the new interpreter have failed.
>> + * Make sure that we do not return -ENOEXEC, as that would
>> + * allow searching for handlers to continue.
>> + */
>> + if (retval == -ENOEXEC)
>> + retval = -EINVAL;
>
> After looking at this some more, I wonder if this should be -ELOOP
> instead? Or maybe that should happen if/when the recursion depth problem is
> fixed?
>
> This is much more obvious, instead of "Invalid argument":
> $ ./dotest.sh
> file-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAfile-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA: bad interpreter: Too many levels of symbolic links
In my opinion, a different, more specific error code is nice, but when
not self-explanatory, it would need to be documented to avoid confusion.
I do not know, what would be the most accepted way to change syscall
return value semantics, if to change semantics or add new ones. From
man-pages, many have already some meaning and only some could be
re-interpreted in that way:
E2BIG: The total number of bytes in the environment (envp) and argument
list (argv) is too large. (not perfect, because usually only associated
with mem/file size issues)
ELOOP: Too many symbolic links were encountered in resolving filename
or the name of a script or ELF interpreter. (currently no distinction
from real symlink problems)
EMFILE: The process has the maximum number of files open. (too generic?)
This one has already a meaning, but only for ELF not script (but since
script might also call ELF in the end, user cannot know):
EINVAL: An ELF executable had more than one PT_INTERP segment (i.e.,
tried to name more than one interpreter).
Those are not yet unused, but I think it is a bad idea to add them,
since some programs might be confused by unexpected error code:
ELIBMAX: Attempting to link in too many shared libraries (not a really
good match)
EMLINK: Too many links (somehow generic, do not know if usually used
another way).
It is strange: from current description, this one suits best, the only
reason why we want to get rid of it is, that it triggers module
reloading and another round of execution.
ENOEXEC: An executable is not in a recognized format, is for the wrong
architecture, or has some other format error that means it cannot be
executed.
Perhaps it would be better to continue returning ENOEXEC from syscall in
that case but change the logic for module-reloading (use some other
return value meaning in binfmt handlers in kernel internally)?
> More importantly, I also wonder if interp handling to just be
> changed to be an allocation that needs to be cleaned up, as done with
> argv?
You mean like an allocation on the stack of the new process' growing
stack? This would be cleaned automatically if something goes wrong
during exec.
> Right now interp just points to the filename argument handed to
> do_execve. Especially since it looks like binfmt_misc is vulnerable
> to this as well, since it runs the risk of getting -ENOEXEC from
> search_binary_handler, leaving bprm->interp pointing into the stack,
> only to get it recalled after module loading attempts:
>
> static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> {
> ...
> char iname[BINPRM_BUF_SIZE];
> ...
> bprm->interp = iname; /* for binfmt_script */
> ...
> retval = search_binary_handler (bprm, regs);
> if (retval < 0)
> goto _error;
> ...
> _ret:
> return retval;
> _error:
> if (fd_binary > 0)
> sys_close(fd_binary);
> bprm->interp_flags = 0;
> bprm->interp_data = 0;
> goto _ret;
> }
Correct. I hope the patch should be a formality, as soon as discussion
on this one is done.
--
http://www.halfdog.net/
PGP: 156A AE98 B91F 0114 FE88 2BD8 C459 9386 feed a bee
Kees Cook <[email protected]> writes:
> + /*
> + * Since bprm is already modified, we cannot continue if the the
s/the the/the/
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
On Fri, Oct 12, 2012 at 10:50 PM, halfdog <[email protected]> wrote:
> Kees Cook wrote:
>> More importantly, I also wonder if interp handling to just be
>> changed to be an allocation that needs to be cleaned up, as done with
>> argv?
>
> You mean like an allocation on the stack of the new process' growing
> stack? This would be cleaned automatically if something goes wrong
> during exec.
Either in userspace like argv, or just a straight kmalloc.
Looking at the code, the problem is that binfmt_script and binfmt_misc
are "rewrite" hacks (they change the bprm instead of actually starting
a process), and the module loading is a hack in that it retries all
the loaders a second time. These two hacks together aren't very
compatible if the error path of the rewrite hacks expects to see the
bprm go away instead of getting retried.
I'm concerned that the proposed patch is really just a band-aid on top
of a broken design.
To make this safe, either the bprm needs to be explicitly copied for
each recursion attempt (so the post-module-load retry starts with a
clean bprm), or everything about the bprm needs to stay off the stack
(to allow rewrite modifications to be stable).
I think the latter approach is best since it means we don't have to
execute the rewrite logic twice. It just means we must take a closer
look at the lifetime of that structure and make sure we're cleaning it
up correctly.
-Kees
--
Kees Cook
Chrome OS Security