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