Messages in this thread Patch in this message | | | From | Kees Cook <> | Date | Thu, 14 Feb 2019 22:27:07 -0800 | Subject | Re: [PATCH v3] exec: load_script: Do not exec truncated interpreter path |
| |
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
| |