* [PATCH] exec/binfmt_script: trip zero bytes from the buffer @ 2021-06-15 16:23 Andrei Vagin 2021-06-15 19:33 ` Eric W. Biederman 0 siblings, 1 reply; 5+ messages in thread From: Andrei Vagin @ 2021-06-15 16:23 UTC (permalink / raw) To: Eric W. Biederman; +Cc: linux-kernel, Kees Cook, linux-fsdevel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer 2021-06-15 16:23 [PATCH] exec/binfmt_script: trip zero bytes from the buffer Andrei Vagin @ 2021-06-15 19:33 ` Eric W. Biederman 2021-06-15 22:35 ` Andrei Vagin 0 siblings, 1 reply; 5+ messages in thread From: Eric W. Biederman @ 2021-06-15 19:33 UTC (permalink / raw) To: Andrei Vagin; +Cc: linux-kernel, Kees Cook, linux-fsdevel 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])) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer 2021-06-15 19:33 ` Eric W. Biederman @ 2021-06-15 22:35 ` Andrei Vagin 2021-06-21 19:27 ` Eric W. Biederman 0 siblings, 1 reply; 5+ messages in thread From: Andrei Vagin @ 2021-06-15 22:35 UTC (permalink / raw) To: Eric W. Biederman; +Cc: LKML, Kees Cook, linux-fsdevel 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])) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer 2021-06-15 22:35 ` Andrei Vagin @ 2021-06-21 19:27 ` Eric W. Biederman 2021-06-25 8:30 ` Andrei Vagin 0 siblings, 1 reply; 5+ messages in thread From: Eric W. Biederman @ 2021-06-21 19:27 UTC (permalink / raw) To: Andrei Vagin; +Cc: LKML, Kees Cook, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer 2021-06-21 19:27 ` Eric W. Biederman @ 2021-06-25 8:30 ` Andrei Vagin 0 siblings, 0 replies; 5+ messages in thread From: Andrei Vagin @ 2021-06-25 8:30 UTC (permalink / raw) To: Eric W. Biederman; +Cc: LKML, Kees Cook, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-25 8:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-15 16:23 [PATCH] exec/binfmt_script: trip zero bytes from the buffer Andrei Vagin 2021-06-15 19:33 ` Eric W. Biederman 2021-06-15 22:35 ` Andrei Vagin 2021-06-21 19:27 ` Eric W. Biederman 2021-06-25 8:30 ` Andrei Vagin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).