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

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

[Kees Cook]

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@chromium.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

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Kees Cook]

On Thu, Feb 14, 2019 at 8:49 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
>
> On Thu, Feb 14, 2019, 19:18 Kees Cook <keescook@chromium.org wrote:
>>
>>
>>  fs/binfmt_script.c | 97 +++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 82 insertions(+), 15 deletions(-)
>
> No, why?

In my defense, 56 of those 82 lines are comments. ;)

>> -       if ((cp = strchr(bprm->buf, '\n')) == NULL)
>> -               cp = bprm->buf+BINPRM_BUF_SIZE-1;
>
>
> This statement when we've not seen a newline should simply get a "is there at least a whitespace in there somewhere" test. Nothing more than that, afaik.

Unfortunately I don't think this is true for two reasons. First, the
leading spaces don't indicate that we won't later truncate the
interpreter path.

i.e. this is valid:
"#!    /usr/bin/perl"
but this could get truncated, even though we see whitespace:
"#!    /some/insane/nfs/or/hexified/path/longer/than/127/bytes"

The only way we know the interpreter wasn't truncated in the
no-newline case is if we see whitespace after first skipping any
leading whitespace, and it seemed really ugly to add a special scan
there. This is what I had for that, minus comments:

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 7cde3f46ad26..6d7ef98bc949 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -43,8 +43,18 @@ static int load_script(struct linux_binprm *bprm)
        bprm->file = NULL;

        bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-       if ((cp = strchr(bprm->buf, '\n')) == NULL)
-               cp = bprm->buf+BINPRM_BUF_SIZE-1;
+       if ((cp = strchr(bprm->buf, '\n')) == NULL) {
+               bool whitespace = false;
+
+               for (cp = bprm->buf+2; cp < bprm->buf+BINPRM_BUF_SIZE-1 &&
+                                      ((*cp == ' ') || (*cp == '\t')); cp++);
+               for (; cp < bprm->buf+BINPRM_BUF_SIZE-1; cp++) {
+                       if ((*cp == ' ') || (*cp == '\t'))
+                               whitespace = true;
+               }
+               if (!whitespace)
+                       return -ENOEXEC; /* Interpreter truncated */
+       }
        *cp = '\0';
        while (cp > bprm->buf) {
                cp--;

But, as it turns out, the above is actually wrong too (yay for my test
cases): the NUL termination before the loop (line 45) may blow away
the newline at byte 127. So we need to actually manually scan for the
newline while doing the out-of-bounds checking. (This was part of
Oleg's original "don't blindly truncate" rationale in the reverted
patch.)

I had also tried building some state machine to walk the bytes and do
everything in a single pass, but it was horrible. In another attempt I
tried converting as much to using "standard" C string routines
(isspace(), strchr(), strsep(), etc), but that was even more terrible.

So I opted to keep most of the existing logic but add the places where
tests were needed (along with comments describing wtf is happening
along the way). Anyway, I'm open to suggestions, obviously. :)

-- 
Kees Cook

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Kees Cook]

On Thu, Feb 14, 2019 at 10:14 PM Kees Cook <keescook@chromium.org> wrote:
> But, as it turns out, the above is actually wrong too (yay for my test
> cases): the NUL termination before the loop (line 45) may blow away
> the newline at byte 127. So we need to actually manually scan for the
> newline while doing the out-of-bounds checking. (This was part of
> Oleg's original "don't blindly truncate" rationale in the reverted
> patch.)

Actually, though, this passes my regression tests:

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 7cde3f46ad26..6d7787e35d76 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -42,9 +42,18 @@ static int load_script(struct linux_binprm *bprm)
        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;
+       if ((cp = strnchr(bprm->buf, BINPRM_BUF_SIZE, '\n')) == NULL) {
+               bool truncated = true;
+
+               for (cp = bprm->buf+2; cp < bprm->buf+BINPRM_BUF_SIZE-1 &&
+                                      ((*cp == ' ') || (*cp == '\t')); cp++);
+               for (; cp < bprm->buf+BINPRM_BUF_SIZE-1; cp++) {
+                       if ((*cp == ' ') || (*cp == '\t'))
+                               truncated = false;
+               }
+               if (truncated)
+                       return -ENOEXEC; /* Interpreter truncated */
+       }
        *cp = '\0';
        while (cp > bprm->buf) {
                cp--;

I still want to add all the comments, though. :)

(The above still seems uglier to me than just collecting the
information as we need it, but I'll do whatever.)

What do you think?

-- 
Kees Cook

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On Thu, Feb 14, 2019 at 10:15 PM Kees Cook <keescook@chromium.org> wrote:
>
> The only way we know the interpreter wasn't truncated in the
> no-newline case is if we see whitespace after first skipping any
> leading whitespace, and it seemed really ugly to add a special scan
> there.

No, much easier (and likely better code too), to just use 'memchr()'.

What's wrong with this simple and fairly self-describing patch?

And I'll rather add a few lines due to helper functions with names to
make it more legible, rather than code in that already fairly long
existing function.

          Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1318 bytes --]

 fs/binfmt_script.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 7cde3f46ad26..c9a7afd477a9 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -14,6 +14,24 @@
 #include <linux/err.h>
 #include <linux/fs.h>
 
+/*
+ * Do we have whitespace (ignoring leading) between first and last
+ * (inclusive). This is the "we are truncating the script command
+ * line" case.
+ */
+static inline bool tabspc(char c) { return c == ' ' || c == '\t'; }
+static inline bool no_tab_or_space(const char *first, const char *last)
+{
+	/* Skip leading space (and first non-space) */
+	while (tabspc(*first++))
+		if (first > last)
+			return false;
+	while (first <= last)
+		if (tabspc(*first++))
+			return true;
+	return false;
+}
+
 static int load_script(struct linux_binprm *bprm)
 {
 	const char *i_arg, *i_name;
@@ -42,9 +60,12 @@ static int load_script(struct linux_binprm *bprm)
 	fput(bprm->file);
 	bprm->file = NULL;
 
-	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-	if ((cp = strchr(bprm->buf, '\n')) == NULL)
+	cp = memchr(bprm->buf, BINPRM_BUF_SIZE, '\n');
+	if (!cp) {
 		cp = bprm->buf+BINPRM_BUF_SIZE-1;
+		if (no_tab_or_space(bprm->buf+2, cp))
+			return -ENOEXEC;
+	}
 	*cp = '\0';
 	while (cp > bprm->buf) {
 		cp--;

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Linus Torvalds]

On Fri, Feb 15, 2019 at 7:54 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> What's wrong with this simple and fairly self-describing patch?

Simple but with two bugs.

First off, the trivial one: I transposed the arguments to memchr(). It
would need to be

        cp = memchr(bprm->buf, '\n', BINPRM_BUF_SIZE);

because I never use memchr().

And even that doesn't really fix it, because what it really wants is
"strnchr()" (to stop at a NUL too). Which doesn't exist.

So skip that patch, it's wrong.

                Linus

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Kees Cook]

On Fri, Feb 15, 2019 at 7:55 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Feb 14, 2019 at 10:15 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > The only way we know the interpreter wasn't truncated in the
> > no-newline case is if we see whitespace after first skipping any
> > leading whitespace, and it seemed really ugly to add a special scan
> > there.
>
> No, much easier (and likely better code too), to just use 'memchr()'.
>
> What's wrong with this simple and fairly self-describing patch?
>
> And I'll rather add a few lines due to helper functions with names to
> make it more legible, rather than code in that already fairly long
> existing function.

This fails to notice truncation when there is leading whitespace.

And I'm happy to add helper functions. We just have to pick which mess
we want to have. :)

-- 
Kees Cook

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Linus Torvalds]

On Fri, Feb 15, 2019 at 8:05 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And even that doesn't really fix it, because what it really wants is
> "strnchr()" (to stop at a NUL too). Which doesn't exist.

.. actually it does exist in the kernel, and I should read all my
emails before writing new ones, because you had a patch in one of your
later emails that looks fine to me and used it.

Although I think I'd prefer that inline  helper model instead to make
things more legible.

Would you be willing to make a frankenpatch that combines our two approaches?

                       Linus

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Oleg Nesterov]

On 02/14, Kees Cook wrote:
>
> --- a/fs/binfmt_script.c
> +++ b/fs/binfmt_script.c
> @@ -42,9 +42,18 @@ static int load_script(struct linux_binprm *bprm)
>         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;
> +       if ((cp = strnchr(bprm->buf, BINPRM_BUF_SIZE, '\n')) == NULL) {
> +               bool truncated = true;
> +
> +               for (cp = bprm->buf+2; cp < bprm->buf+BINPRM_BUF_SIZE-1 &&
> +                                      ((*cp == ' ') || (*cp == '\t')); cp++);
> +               for (; cp < bprm->buf+BINPRM_BUF_SIZE-1; cp++) {
> +                       if ((*cp == ' ') || (*cp == '\t'))
> +                               truncated = false;
> +               }
> +               if (truncated)
> +                       return -ENOEXEC; /* Interpreter truncated */

Not sure. Consider a script file which has a single line

	#!/path/to/interpreter

WITHOUT '\n' at the end. If I read load_script() correctly it should work,
so I think the 2nd for() loop should also reset "truncated" if *cp == '\0'.

Hmm. And cp < bprm->buf+BINPRM_BUF_SIZE-1 is off-by-one again...

Well. Probably nobody does this... but after regression caused by my patch
I am not 100% sure ;)

Oleg.

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Kees Cook]

On Fri, Feb 15, 2019 at 8:18 AM Oleg Nesterov <oleg@redhat.com> wrote:
> WITHOUT '\n' at the end. If I read load_script() correctly it should work,
> so I think the 2nd for() loop should also reset "truncated" if *cp == '\0'.

Correct. My original v3 handles this correctly. I'll work on a version
with small inline helpers.

-- 
Kees Cook

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

On Fri, Feb 15, 2019 at 8:18 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Not sure. Consider a script file which has a single line
>
>         #!/path/to/interpreter
>
> WITHOUT '\n' at the end.

Heh. I'm not sure how valid that is, but it's an interesting case for sure.

But it's actually fairly easy to fix with the franken-approach I did
that combines mine and Kees' patches.

Does this work?

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1563 bytes --]

 fs/binfmt_script.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 7cde3f46ad26..41be15701383 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -14,6 +14,29 @@
 #include <linux/err.h>
 #include <linux/fs.h>
 
+/*
+ * Do we have a terminating character between 'first' and 'last'
+ * (inclusive). This is the "we are truncating the script command
+ * line" case, and we know first < last.
+ *
+ * We skip leading whitespace, and then verify there's a space/tab
+ * or NUL before the end.
+ */
+static inline bool tabspc(char c) { return c == ' ' || c == '\t'; }
+static inline bool no_tab_or_space(const char *first, const char *last)
+{
+	// Skip leading space
+	for (;tabspc(*first) ; first++)
+		if (!*first || first == last)
+			return false;	// only space
+	// Ok, 'first' points to first non-spc/tab/NUL
+	// Can we find another terminator after this?
+	while (++first <= last)
+		if (!*first || tabspc(*first))
+			return true;
+	return false;
+}
+
 static int load_script(struct linux_binprm *bprm)
 {
 	const char *i_arg, *i_name;
@@ -42,9 +65,12 @@ static int load_script(struct linux_binprm *bprm)
 	fput(bprm->file);
 	bprm->file = NULL;
 
-	bprm->buf[BINPRM_BUF_SIZE - 1] = '\0';
-	if ((cp = strchr(bprm->buf, '\n')) == NULL)
+	cp = strnchr(bprm->buf, BINPRM_BUF_SIZE, '\n');
+	if (!cp) {
 		cp = bprm->buf+BINPRM_BUF_SIZE-1;
+		if (no_tab_or_space(bprm->buf+2, cp))
+			return -ENOEXEC;
+	}
 	*cp = '\0';
 	while (cp > bprm->buf) {
 		cp--;

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Kees Cook]

On Fri, Feb 15, 2019 at 8:39 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Feb 15, 2019 at 8:18 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Not sure. Consider a script file which has a single line
> >
> >         #!/path/to/interpreter
> >
> > WITHOUT '\n' at the end.
>
> Heh. I'm not sure how valid that is, but it's an interesting case for sure.
>
> But it's actually fairly easy to fix with the franken-approach I did
> that combines mine and Kees' patches.
>
> Does this work?

+static inline bool no_tab_or_space(const char *first, const char *last)
+{
+        // Skip leading space
+        for (;tabspc(*first) ; first++)
+            if (!*first || first == last)
+                return false; // only space

The !*first will never hit here (since it's been checked to be either
' ' or '\t', and if first == last it's whitespace all the way, so we
could just return true here to bail out early (there's no interpreter
at all, so we want to -ENOEXEC still).

I'll get a version written and tested...

-Kees

-- 
Kees Cook

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Oleg Nesterov]

On 02/15, Linus Torvalds wrote:
>
> +static inline bool tabspc(char c) { return c == ' ' || c == '\t'; }
> +static inline bool no_tab_or_space(const char *first, const char *last)
> +{
> +	// Skip leading space
> +	for (;tabspc(*first) ; first++)
> +		if (!*first || first == last)
                    ^^^^^^^
well, given that tabspc(*first) is true, I don't think that *first == 0
is ever possible ;)

Oleg.

Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path

[Linus Torvalds]

On Fri, Feb 15, 2019 at 9:01 AM Kees Cook <keescook@chromium.org> wrote:
>
> The !*first will never hit here (since it's been checked to be either
> ' ' or '\t', and if first == last it's whitespace all the way, so we
> could just return true here to bail out early (there's no interpreter
> at all, so we want to -ENOEXEC still).

Correct.

You do need the !*first test, but it needs to be after the loop
(because the next loop skips over the first non-space thing we found).

               Linus