[PATCH] exec/binfmt_script: trip zero bytes from the buffer

[PATCH] exec/binfmt_script: trip zero bytes from the buffer

[Andrei Vagin]

Without this fix, if we try to run a script that contains only the
interpreter line, the interpreter is executed with one extra empty
argument.

The code is written so that i_end has to be set to the end of valuable
data in the buffer.

Fixes: ccbb18b67323 ("exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC")
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 fs/binfmt_script.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 1b6625e95958..e242680f96e1 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -68,6 +68,9 @@ static int load_script(struct linux_binprm *bprm)
 		if (!next_terminator(i_end, buf_end))
 			return -ENOEXEC;
 		i_end = buf_end;
+		/* Trim zero bytes from i_end */
+		while (i_end[-1] == 0)
+			i_end--;
 	}
 	/* Trim any trailing spaces/tabs from i_end */
 	while (spacetab(i_end[-1]))
-- 
2.31.1

Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer

[Eric W. Biederman]

Andrei Vagin <avagin@gmail.com> writes:

> Without this fix, if we try to run a script that contains only the
> interpreter line, the interpreter is executed with one extra empty
> argument.
>
> The code is written so that i_end has to be set to the end of valuable
> data in the buffer.

Out of curiosity how did you spot this change in behavior?

> Fixes: ccbb18b67323 ("exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC")
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Andrei Vagin <avagin@gmail.com>
> ---
>  fs/binfmt_script.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> index 1b6625e95958..e242680f96e1 100644
> --- a/fs/binfmt_script.c
> +++ b/fs/binfmt_script.c
> @@ -68,6 +68,9 @@ static int load_script(struct linux_binprm *bprm)
>  		if (!next_terminator(i_end, buf_end))
>  			return -ENOEXEC;
>  		i_end = buf_end;
> +		/* Trim zero bytes from i_end */
> +		while (i_end[-1] == 0)
> +			i_end--;
>  	}
>  	/* Trim any trailing spaces/tabs from i_end */
>  	while (spacetab(i_end[-1]))

Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer

[Andrei Vagin]

On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Andrei Vagin <avagin@gmail.com> writes:
>
> > Without this fix, if we try to run a script that contains only the
> > interpreter line, the interpreter is executed with one extra empty
> > argument.
> >
> > The code is written so that i_end has to be set to the end of valuable
> > data in the buffer.
>
> Out of curiosity how did you spot this change in behavior?

gVisor tests started failing with this change:
https://github.com/google/gvisor/blob/5e05950c1c520724e2e03963850868befb95efeb/test/syscalls/linux/exec.cc#L307

We run these tests on Ubuntu 20.04 and this is the reason why we
caught this issue just a few days ago.

>
> > Fixes: ccbb18b67323 ("exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC")
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Signed-off-by: Andrei Vagin <avagin@gmail.com>
> > ---
> >  fs/binfmt_script.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> > index 1b6625e95958..e242680f96e1 100644
> > --- a/fs/binfmt_script.c
> > +++ b/fs/binfmt_script.c
> > @@ -68,6 +68,9 @@ static int load_script(struct linux_binprm *bprm)
> >               if (!next_terminator(i_end, buf_end))
> >                       return -ENOEXEC;
> >               i_end = buf_end;
> > +             /* Trim zero bytes from i_end */
> > +             while (i_end[-1] == 0)
> > +                     i_end--;
> >       }
> >       /* Trim any trailing spaces/tabs from i_end */
> >       while (spacetab(i_end[-1]))

Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer

[Eric W. Biederman]

Andrei Vagin <avagin@gmail.com> writes:

> On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> Andrei Vagin <avagin@gmail.com> writes:
>>
>> > Without this fix, if we try to run a script that contains only the
>> > interpreter line, the interpreter is executed with one extra empty
>> > argument.
>> >
>> > The code is written so that i_end has to be set to the end of valuable
>> > data in the buffer.
>>
>> Out of curiosity how did you spot this change in behavior?
>
> gVisor tests started failing with this change:
> https://github.com/google/gvisor/blob/5e05950c1c520724e2e03963850868befb95efeb/test/syscalls/linux/exec.cc#L307
>
> We run these tests on Ubuntu 20.04 and this is the reason why we
> caught this issue just a few days ago.

I like where you are going, but starting at the end of the buffer
there is the potential to skip deliberately embedded '\0' characters.

While looking at this I realized that your patch should not have
made a difference but there is a subtle bug in the logic of
next_non_spacetab, that allowed your code to make it that far.

Can you test my patch below?

I think I have simplified the logic enough to prevent bugs from getting
in.

Eric

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 1b6625e95958..7d204693326c 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -26,7 +26,7 @@ static inline const char *next_non_spacetab(const char *first, const char *last)
 static inline const char *next_terminator(const char *first, const char *last)
 {
 	for (; first <= last; first++)
-		if (spacetab(*first) || !*first)
+		if (spacetab(*first))
 			return first;
 	return NULL;
 }
@@ -44,9 +44,9 @@ static int load_script(struct linux_binprm *bprm)
 	/*
 	 * 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
-	 * (though the buffer will have trailing NUL padding when the
-	 * file size was smaller than the buffer size).
+	 * because bprm->buf is not guaranteed to be NUL-terminated
+	 * (the buffer will have trailing NUL padding when the file
+	 * size was smaller than the buffer size).
 	 *
 	 * We do not want to exec a truncated interpreter path, so either
 	 * we find a newline (which indicates nothing is truncated), or
@@ -57,33 +57,37 @@ static int load_script(struct linux_binprm *bprm)
 	 */
 	buf_end = bprm->buf + sizeof(bprm->buf) - 1;
 	i_end = strnchr(bprm->buf, sizeof(bprm->buf), '\n');
-	if (!i_end) {
-		i_end = next_non_spacetab(bprm->buf + 2, buf_end);
-		if (!i_end)
-			return -ENOEXEC; /* Entire buf is spaces/tabs */
-		/*
-		 * If there is no later space/tab/NUL we must assume the
-		 * interpreter path is truncated.
-		 */
-		if (!next_terminator(i_end, buf_end))
-			return -ENOEXEC;
-		i_end = buf_end;
+	if (i_end) {
+		/* Hide the trailing newline */
+		i_end = i_end - 1;
+	} else {
+		/* Find the end of the text */
+		i_end = memchr(bprm->buf + 2, '\0', sizeof(bprm->buf));
+		i_end = i_end ? i_end - 1 : buf_end;
 	}
+
 	/* Trim any trailing spaces/tabs from i_end */
-	while (spacetab(i_end[-1]))
+	while (spacetab(i_end[0]))
 		i_end--;
 
 	/* Skip over leading spaces/tabs */
 	i_name = next_non_spacetab(bprm->buf+2, i_end);
-	if (!i_name || (i_name == i_end))
+	if (!i_name)
 		return -ENOEXEC; /* No interpreter name found */
 
 	/* Is there an optional argument? */
 	i_arg = NULL;
 	i_sep = next_terminator(i_name, i_end);
-	if (i_sep && (*i_sep != '\0'))
+	if (i_sep)
 		i_arg = next_non_spacetab(i_sep, i_end);
 
+	/*
+	 * If there is no space/tab/NUL after the interpreter we must
+	 * assume the interpreter path is truncated.
+	 */
+	if (!i_sep && (i_end == buf_end))
+		return -ENOEXEC;
+
 	/*
 	 * If the script filename will be inaccessible after exec, typically
 	 * because it is a "/dev/fd/<fd>/.." path against an O_CLOEXEC fd, give

Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer

[Andrei Vagin]

On Mon, Jun 21, 2021 at 02:27:47PM -0500, Eric W. Biederman wrote:
> Andrei Vagin <avagin@gmail.com> writes:
> 
> > On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
> > <ebiederm@xmission.com> wrote:
> >>
> >> Andrei Vagin <avagin@gmail.com> writes:
> >>
> >> > Without this fix, if we try to run a script that contains only the
> >> > interpreter line, the interpreter is executed with one extra empty
> >> > argument.
> >> >
> >> > The code is written so that i_end has to be set to the end of valuable
> >> > data in the buffer.
> >>
> >> Out of curiosity how did you spot this change in behavior?
> >
> > gVisor tests started failing with this change:
> > https://github.com/google/gvisor/blob/5e05950c1c520724e2e03963850868befb95efeb/test/syscalls/linux/exec.cc#L307
> >
> > We run these tests on Ubuntu 20.04 and this is the reason why we
> > caught this issue just a few days ago.
> 
> I like where you are going, but starting at the end of the buffer
> there is the potential to skip deliberately embedded '\0' characters.
> 
> While looking at this I realized that your patch should not have
> made a difference but there is a subtle bug in the logic of
> next_non_spacetab, that allowed your code to make it that far.
> 
> Can you test my patch below?
> 
> I think I have simplified the logic enough to prevent bugs from getting
> in.
> 
> Eric
> 
> diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> index 1b6625e95958..7d204693326c 100644
> --- a/fs/binfmt_script.c
> +++ b/fs/binfmt_script.c
> @@ -26,7 +26,7 @@ static inline const char *next_non_spacetab(const char *first, const char *last)
>  static inline const char *next_terminator(const char *first, const char *last)
>  {
>  	for (; first <= last; first++)
> -		if (spacetab(*first) || !*first)
> +		if (spacetab(*first))
>  			return first;
>  	return NULL;
>  }
> @@ -44,9 +44,9 @@ static int load_script(struct linux_binprm *bprm)
>  	/*
>  	 * 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
> -	 * (though the buffer will have trailing NUL padding when the
> -	 * file size was smaller than the buffer size).
> +	 * because bprm->buf is not guaranteed to be NUL-terminated
> +	 * (the buffer will have trailing NUL padding when the file
> +	 * size was smaller than the buffer size).
>  	 *
>  	 * We do not want to exec a truncated interpreter path, so either
>  	 * we find a newline (which indicates nothing is truncated), or
> @@ -57,33 +57,37 @@ static int load_script(struct linux_binprm *bprm)
>  	 */
>  	buf_end = bprm->buf + sizeof(bprm->buf) - 1;
>  	i_end = strnchr(bprm->buf, sizeof(bprm->buf), '\n');
> -	if (!i_end) {
> -		i_end = next_non_spacetab(bprm->buf + 2, buf_end);
> -		if (!i_end)
> -			return -ENOEXEC; /* Entire buf is spaces/tabs */
> -		/*
> -		 * If there is no later space/tab/NUL we must assume the
> -		 * interpreter path is truncated.
> -		 */
> -		if (!next_terminator(i_end, buf_end))
> -			return -ENOEXEC;
> -		i_end = buf_end;
> +	if (i_end) {
> +		/* Hide the trailing newline */
> +		i_end = i_end - 1;

Your patch changes the meaning of i_end. Now it points to the last
symbol, but this function contains the line:
	*((char *)i_end) = '\0';

and it drops the last meaningful symbol. With the following tiny fix, my
test passes:


@@ -114,7 +115,7 @@ static int load_script(struct linux_binprm *bprm)
        if (retval < 0)
                return retval;
        bprm->argc++;
-       *((char *)i_end) = '\0';
+       *((char *)(i_end + 1)) = '\0';
        if (i_arg) {
                *((char *)i_sep) = '\0';
                retval = copy_string_kernel(i_arg, bprm);

Thanks,
Andrei