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

[Kees Cook]

On Thu, Feb 14, 2019 at 8:49 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
>
>
> On Thu, Feb 14, 2019, 19:18 Kees Cook <keescook@xxxxxxxxxxxx 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