linux-kernel - [PATCH v3] exec: load_script: Do not exec truncated interpreter path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20190215031758.GA18776@beast>
Date:   Thu, 14 Feb 2019 19:17:58 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Samuel Dionne-Riel <samuel@...nne-riel.com>,
        Richard Weinberger <richard.weinberger@...il.com>,
        Graham Christensen <graham@...hamc.com>,
        Michal Hocko <mhocko@...e.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

Commit 8099b047ecc4 ("exec: load_script: don't blindly truncate
shebang string") was trying to protect against a confused exec of a
truncated interpreter path. However, it was overeager and also refused
to truncate arguments as well, which broke userspace, and it was
reverted. This attempts the protection again, but allows arguments to
remain truncated. Lots more comments are added, since the parsing here
is rather fiddly while dealing with whitespace.

Signed-off-by: Kees Cook <keescook@...omium.org>
---
 fs/binfmt_script.c | 97 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 82 insertions(+), 15 deletions(-)

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 7cde3f46ad26..8fca59f9ee03 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -20,7 +20,9 @@ static int load_script(struct linux_binprm *bprm)
 	char *cp;
 	struct file *file;
 	int retval;
+	bool truncated = false, end_of_interp = false;
 
+	/* Not ours to exec if we don't start with "#!". */
 	if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!'))
 		return -ENOEXEC;
 
@@ -33,37 +35,102 @@ static int load_script(struct linux_binprm *bprm)
 	if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
 		return -ENOENT;
 
-	/*
-	 * This section does the #! interpretation.
-	 * Sorta complicated, but hopefully it will work.  -TYT
-	 */
-
+	/* Release since we are not mapping a binary into memory. */
 	allow_write_access(bprm->file);
 	fput(bprm->file);
 	bprm->file = NULL;
 
-	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-	if ((cp = strchr(bprm->buf, '\n')) == NULL)
-		cp = bprm->buf+BINPRM_BUF_SIZE-1;
+	/*
+	 * This section handles parsing the #! line into separate
+	 * interpreter path and argument strings. We must be careful
+	 * because bprm->buf is not yet guaranteed to be NUL-terminated.
+	 *
+	 * Truncating interpreter arguments is okay: the interpreter
+	 * can re-read the script to parse them on its own. Truncating
+	 * the interpreter path itself, though, is bad. We can note
+	 * truncation here, but we cannot yet check for non-EOL whitespace
+	 * because any leading whitespace would not indicate the end of
+	 * the interpreter path string.
+	 */
+	for (cp = bprm->buf + 2;; cp++) {
+		if (!*cp || (*cp == '\n')) {
+			/*
+			 * If we see NUL (end of file) or newline it means
+			 * we hit the end of the #! line without truncation.
+			 */
+			end_of_interp = true;
+			break;
+		}
+		if (cp == bprm->buf + BINPRM_BUF_SIZE - 1) {
+			/*
+			 * Otherwise if we reach the end of the buffer,
+			 * we've been truncated, but we don't know if
+			 * it was in arguments or the interpreter path.
+			 */
+			truncated = true;
+			break;
+		}
+	}
 	*cp = '\0';
+	/* At this point, the bprm->buf array is a NUL-terminated string. */
+
+	/*
+	 * Truncate trailing whitespace so it cannot be included in either
+	 * interpreter or argument strings.
+	 */
 	while (cp > bprm->buf) {
 		cp--;
-		if ((*cp == ' ') || (*cp == '\t'))
+		if ((*cp == ' ') || (*cp == '\t')) {
+			/*
+			 * If we see whitespace at the end of the buffer,
+			 * we know we've at least found a full interpreter
+			 * path (even if it's zero length, which is checked
+			 * later).
+			 */
+			end_of_interp = true;
 			*cp = '\0';
-		else
+		} else
 			break;
 	}
+	/* Skip leading whitespace ahead of the interpreter path. */
 	for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
-	if (*cp == '\0')
-		return -ENOEXEC; /* No interpreter name found */
-	i_name = cp;
-	i_arg = NULL;
+	/*
+	 * We've successfully found the start of the interpreter path.
+	 * Fail if the interpreter path is already empty.
+	 */
+	if (*cp)
+		i_name = cp;
+	else
+		return -ENOEXEC;
+	/*
+	 * Find the end of the interpreter path. We will either hit NUL
+	 * termination or find whitespace which signals the start of
+	 * arguments.
+	 */
 	for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
 		/* nothing */ ;
-	while ((*cp == ' ') || (*cp == '\t'))
+	/*
+	 * In the case of whitespace, terminate the end of the interpreter
+	 * path and skip until we reach the start of the arguments.
+	 */
+	while ((*cp == ' ') || (*cp == '\t')) {
+		/*
+		 * Finding whitespace at the end of the interpreter path
+		 * means we've intentionally hit the end of the path
+		 * without truncation.
+		 */
+		end_of_interp = true;
 		*cp++ = '\0';
+	}
+	/* We've successfully found the start of any potential arguments. */
 	if (*cp)
 		i_arg = cp;
+	else
+		i_arg = NULL;
+	/* Fail if the interpreter path was cut off. */
+	if (truncated && !end_of_interp)
+		return -ENOEXEC;
+
 	/*
 	 * OK, we've parsed out the interpreter name and
 	 * (optional) argument.
-- 
2.17.1


-- 
Kees Cook