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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Feb 2019 22:27:07 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Samuel Dionne-Riel <samuel@...nne-riel.com>,
        Richard Weinberger <richard.weinberger@...il.com>,
        Graham Christensen <graham@...hamc.com>,
        Michal Hocko <mhocko@...e.com>,
        LKML <linux-kernel@...r.kernel.org>
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@...omium.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