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

Kees Cook
2019-02-15T06:27:46Z

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