Patchwork [42/88] exec: use -ELOOP for max recursion depth

login
register
mail settings
Submitter Luis Henriques
Date March 14, 2013, 10:35 a.m.
Message ID <1363257381-15900-43-git-send-email-luis.henriques@canonical.com>
Download mbox | patch
Permalink /patch/227635/
State New
Headers show

Comments

Luis Henriques - March 14, 2013, 10:35 a.m.
3.5.7.8 -stable review patch.  If anyone has any objections, please let me know.

------------------

From: Kees Cook <keescook@chromium.org>

commit d740269867021faf4ce38a449353d2b986c34a67 upstream.

To avoid an explosion of request_module calls on a chain of abusive
scripts, fail maximum recursion with -ELOOP instead of -ENOEXEC. As soon
as maximum recursion depth is hit, the error will fail all the way back
up the chain, aborting immediately.

This also has the side-effect of stopping the user's shell from attempting
to reexecute the top-level file as a shell script. As seen in the
dash source:

        if (cmd != path_bshell && errno == ENOEXEC) {
                *argv-- = cmd;
                *argv = cmd = path_bshell;
                goto repeat;
        }

The above logic was designed for running scripts automatically that lacked
the "#!" header, not to re-try failed recursion. On a legitimate -ENOEXEC,
things continue to behave as the shell expects.

Additionally, when tracking recursion, the binfmt handlers should not be
involved. The recursion being tracked is the depth of calls through
search_binary_handler(), so that function should be exclusively responsible
for tracking the depth.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: halfdog <me@halfdog.net>
Cc: P J P <ppandit@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[ luis: backport to 3.5 ]
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 fs/binfmt_em86.c        |  1 -
 fs/binfmt_misc.c        |  8 +-------
 fs/binfmt_script.c      |  4 +---
 fs/exec.c               | 10 +++++-----
 include/linux/binfmts.h |  2 --
 5 files changed, 7 insertions(+), 18 deletions(-)
Ben Hutchings - March 19, 2013, 2:53 a.m.
On Thu, 2013-03-14 at 10:35 +0000, Luis Henriques wrote:
> 3.5.7.8 -stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Kees Cook <keescook@chromium.org>
> 
> commit d740269867021faf4ce38a449353d2b986c34a67 upstream.
> 
> To avoid an explosion of request_module calls on a chain of abusive
> scripts, fail maximum recursion with -ELOOP instead of -ENOEXEC. As soon
> as maximum recursion depth is hit, the error will fail all the way back
> up the chain, aborting immediately.
> 
> This also has the side-effect of stopping the user's shell from attempting
> to reexecute the top-level file as a shell script. As seen in the
> dash source:
> 
>         if (cmd != path_bshell && errno == ENOEXEC) {
>                 *argv-- = cmd;
>                 *argv = cmd = path_bshell;
>                 goto repeat;
>         }
> 
> The above logic was designed for running scripts automatically that lacked
> the "#!" header, not to re-try failed recursion. On a legitimate -ENOEXEC,
> things continue to behave as the shell expects.
> 
> Additionally, when tracking recursion, the binfmt handlers should not be
> involved. The recursion being tracked is the depth of calls through
> search_binary_handler(), so that function should be exclusively responsible
> for tracking the depth.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: halfdog <me@halfdog.net>
> Cc: P J P <ppandit@redhat.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [ luis: backport to 3.5 ]
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
[...]

Greg, I also included this in 3.2.y (commit 511d07b) but it is missing
from 3.0.y and 3.4.y.  I hope one or other of these backports will be
suitable for them (it was just a context fix for 3.2.y).

Ben.
Greg KH - March 26, 2013, 8:25 p.m.
On Tue, Mar 19, 2013 at 02:53:07AM +0000, Ben Hutchings wrote:
> On Thu, 2013-03-14 at 10:35 +0000, Luis Henriques wrote:
> > 3.5.7.8 -stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Kees Cook <keescook@chromium.org>
> > 
> > commit d740269867021faf4ce38a449353d2b986c34a67 upstream.
> > 
> > To avoid an explosion of request_module calls on a chain of abusive
> > scripts, fail maximum recursion with -ELOOP instead of -ENOEXEC. As soon
> > as maximum recursion depth is hit, the error will fail all the way back
> > up the chain, aborting immediately.
> > 
> > This also has the side-effect of stopping the user's shell from attempting
> > to reexecute the top-level file as a shell script. As seen in the
> > dash source:
> > 
> >         if (cmd != path_bshell && errno == ENOEXEC) {
> >                 *argv-- = cmd;
> >                 *argv = cmd = path_bshell;
> >                 goto repeat;
> >         }
> > 
> > The above logic was designed for running scripts automatically that lacked
> > the "#!" header, not to re-try failed recursion. On a legitimate -ENOEXEC,
> > things continue to behave as the shell expects.
> > 
> > Additionally, when tracking recursion, the binfmt handlers should not be
> > involved. The recursion being tracked is the depth of calls through
> > search_binary_handler(), so that function should be exclusively responsible
> > for tracking the depth.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Cc: halfdog <me@halfdog.net>
> > Cc: P J P <ppandit@redhat.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > [ luis: backport to 3.5 ]
> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> [...]
> 
> Greg, I also included this in 3.2.y (commit 511d07b) but it is missing
> from 3.0.y and 3.4.y.  I hope one or other of these backports will be
> suitable for them (it was just a context fix for 3.2.y).

Applied, thanks.

greg k-h

Patch

diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index 2790c7e..575796a 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -42,7 +42,6 @@  static int load_em86(struct linux_binprm *bprm,struct pt_regs *regs)
 			return -ENOEXEC;
 	}
 
-	bprm->recursion_depth++; /* Well, the bang-shell is implicit... */
 	allow_write_access(bprm->file);
 	fput(bprm->file);
 	bprm->file = NULL;
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 772428d..233dcb4 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -117,10 +117,6 @@  static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 	if (!enabled)
 		goto _ret;
 
-	retval = -ENOEXEC;
-	if (bprm->recursion_depth > BINPRM_MAX_RECURSION)
-		goto _ret;
-
 	/* to keep locking time low, we copy the interpreter string */
 	read_lock(&entries_lock);
 	fmt = check_file(bprm);
@@ -200,9 +196,7 @@  static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 	if (retval < 0)
 		goto _error;
 
-	bprm->recursion_depth++;
-
-	retval = search_binary_handler (bprm, regs);
+	retval = search_binary_handler(bprm, regs);
 	if (retval < 0)
 		goto _error;
 
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index df49d48..8ae4be1 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -22,15 +22,13 @@  static int load_script(struct linux_binprm *bprm,struct pt_regs *regs)
 	char interp[BINPRM_BUF_SIZE];
 	int retval;
 
-	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!') ||
-	    (bprm->recursion_depth > BINPRM_MAX_RECURSION))
+	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!'))
 		return -ENOEXEC;
 	/*
 	 * This section does the #! interpretation.
 	 * Sorta complicated, but hopefully it will work.  -TYT
 	 */
 
-	bprm->recursion_depth++;
 	allow_write_access(bprm->file);
 	fput(bprm->file);
 	bprm->file = NULL;
diff --git a/fs/exec.c b/fs/exec.c
index 5ee73d2..858423a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1398,6 +1398,10 @@  int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 	struct linux_binfmt *fmt;
 	pid_t old_pid, old_vpid;
 
+	/* This allows 4 levels of binfmt rewrites before failing hard. */
+	if (depth > 5)
+		return -ELOOP;
+
 	retval = security_bprm_check(bprm);
 	if (retval)
 		return retval;
@@ -1422,12 +1426,8 @@  int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			if (!try_module_get(fmt->module))
 				continue;
 			read_unlock(&binfmt_lock);
+			bprm->recursion_depth = depth + 1;
 			retval = fn(bprm, regs);
-			/*
-			 * Restore the depth counter to its starting value
-			 * in this call, so we don't have to rely on every
-			 * load_binary function to restore it on return.
-			 */
 			bprm->recursion_depth = depth;
 			if (retval >= 0) {
 				if (depth == 0) {
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index eb53e15..5bab59b 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -68,8 +68,6 @@  struct linux_binprm {
 #define BINPRM_FLAGS_EXECFD_BIT 1
 #define BINPRM_FLAGS_EXECFD (1 << BINPRM_FLAGS_EXECFD_BIT)
 
-#define BINPRM_MAX_RECURSION 4
-
 /* Function parameter for binfmt->coredump */
 struct coredump_params {
 	long signr;