| Message ID | 20220201000947.2453721-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | dcd46d897adb70d63e025f175a00a89797d31a43 |
| Headers | show |
| Series | exec: Force single empty string when argv is empty | expand |
Hi, On Mon, 31 Jan 2022, Kees Cook wrote: > Quoting[1] Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > second argument to execve(2) be the name of a program, thus prohibiting > a scenario where argc < 1. POSIX 2017 also recommends this behaviour, > but it is not an explicit requirement[2]: > > The argument arg0 should point to a filename string that is > associated with the process being started by one of the exec > functions. > ... > Interestingly, Michael Kerrisk opened an issue about this in 2008[3], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[4] > of this bug in a shellcode, we can reconsider. > > This issue is being tracked in the KSPP issue tracker[5]." > > While the initial code searches[6][7] turned up what appeared to be > mostly corner case tests, trying to that just reject argv == NULL > (or an immediately terminated pointer list) quickly started tripping[8] > existing userspace programs. Yes, it's a shame this is the case, but we do what we have to do, I guess :) > > The next best approach is forcing a single empty string into argv and > adjusting argc to match. The number of programs depending on argc == 0 > seems a smaller set than those calling execve with a NULL argv. > > Account for the additional stack space in bprm_stack_limits(). Inject an > empty string when argc == 0 (and set argc = 1). Warn about the case so > userspace has some notice about the change: > > process './argc0' launched './argc0' with NULL argv: empty string added > > Additionally WARN() and reject NULL argv usage for kernel threads. > > [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ > [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > [5] https://github.com/KSPP/linux/issues/176 > [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 > [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ > > Reported-by: Ariadne Conill <ariadne@dereferenced.org> > Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Rich Felker <dalias@libc.org> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> In terms of going with this approach as an alternative verses my original patch, Acked-by: Ariadne Conill <ariadne@dereferenced.org> Ariadne
On 1/31/22 16:09, Kees Cook wrote: > Quoting[1] Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > second argument to execve(2) be the name of a program, thus prohibiting > a scenario where argc < 1. POSIX 2017 also recommends this behaviour, > but it is not an explicit requirement[2]: > > The argument arg0 should point to a filename string that is > associated with the process being started by one of the exec > functions. > ... > Interestingly, Michael Kerrisk opened an issue about this in 2008[3], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[4] > of this bug in a shellcode, we can reconsider. > > This issue is being tracked in the KSPP issue tracker[5]." > > While the initial code searches[6][7] turned up what appeared to be > mostly corner case tests, trying to that just reject argv == NULL > (or an immediately terminated pointer list) quickly started tripping[8] > existing userspace programs. > > The next best approach is forcing a single empty string into argv and > adjusting argc to match. The number of programs depending on argc == 0 > seems a smaller set than those calling execve with a NULL argv. > > Account for the additional stack space in bprm_stack_limits(). Inject an > empty string when argc == 0 (and set argc = 1). Warn about the case so > userspace has some notice about the change: > > process './argc0' launched './argc0' with NULL argv: empty string added > > Additionally WARN() and reject NULL argv usage for kernel threads. > > [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ > [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > [5] https://github.com/KSPP/linux/issues/176 > [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 > [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ Acked-by: Andy Lutomirski <luto@kernel.org> and cc-ing linux-api. I agree that this should be done regardless of any security context change. > > Reported-by: Ariadne Conill <ariadne@dereferenced.org> > Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Rich Felker <dalias@libc.org> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/exec.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 79f2c9483302..bbf3aadf7ce1 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -495,8 +495,14 @@ static int bprm_stack_limits(struct linux_binprm *bprm) > * the stack. They aren't stored until much later when we can't > * signal to the parent that the child has run out of stack space. > * Instead, calculate it here so it's possible to fail gracefully. > + * > + * In the case of argc = 0, make sure there is space for adding a > + * empty string (which will bump argc to 1), to ensure confused > + * userspace programs don't start processing from argv[1], thinking > + * argc can never be 0, to keep them from walking envp by accident. > + * See do_execveat_common(). > */ > - ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > + ptr_size = (min(bprm->argc, 1) + bprm->envc) * sizeof(void *); > if (limit <= ptr_size) > return -E2BIG; > limit -= ptr_size; > @@ -1897,6 +1903,9 @@ static int do_execveat_common(int fd, struct filename *filename, > } > > retval = count(argv, MAX_ARG_STRINGS); > + if (retval == 0) > + pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n", > + current->comm, bprm->filename); > if (retval < 0) > goto out_free; > bprm->argc = retval; > @@ -1923,6 +1932,19 @@ static int do_execveat_common(int fd, struct filename *filename, > if (retval < 0) > goto out_free; > > + /* > + * When argv is empty, add an empty string ("") as argv[0] to > + * ensure confused userspace programs that start processing > + * from argv[1] won't end up walking envp. See also > + * bprm_stack_limits(). > + */ > + if (bprm->argc == 0) { > + retval = copy_string_kernel("", bprm); > + if (retval < 0) > + goto out_free; > + bprm->argc = 1; > + } > + > retval = bprm_execve(bprm, fd, filename, flags); > out_free: > free_bprm(bprm); > @@ -1951,6 +1973,8 @@ int kernel_execve(const char *kernel_filename, > } > > retval = count_strings_kernel(argv); > + if (WARN_ON_ONCE(retval == 0)) > + retval = -EINVAL; > if (retval < 0) > goto out_free; > bprm->argc = retval;
From: Kees Cook > Sent: 01 February 2022 00:10 ... > While the initial code searches[6][7] turned up what appeared to be > mostly corner case tests, trying to that just reject argv == NULL > (or an immediately terminated pointer list) quickly started tripping[8] > existing userspace programs. > > The next best approach is forcing a single empty string into argv and > adjusting argc to match. The number of programs depending on argc == 0 > seems a smaller set than those calling execve with a NULL argv. Has anyone considered using the pathname for argv[0]? So converting: execl(path, NULL); into: execl(path, path, NULL); I've not spotted any such suggestion. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Jan 31, 2022 at 04:09:47PM -0800, Kees Cook wrote: > Quoting[1] Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > second argument to execve(2) be the name of a program, thus prohibiting > a scenario where argc < 1. POSIX 2017 also recommends this behaviour, > but it is not an explicit requirement[2]: > > The argument arg0 should point to a filename string that is > associated with the process being started by one of the exec > functions. > ... > Interestingly, Michael Kerrisk opened an issue about this in 2008[3], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[4] > of this bug in a shellcode, we can reconsider. > > This issue is being tracked in the KSPP issue tracker[5]." > > While the initial code searches[6][7] turned up what appeared to be > mostly corner case tests, trying to that just reject argv == NULL > (or an immediately terminated pointer list) quickly started tripping[8] > existing userspace programs. > > The next best approach is forcing a single empty string into argv and > adjusting argc to match. The number of programs depending on argc == 0 > seems a smaller set than those calling execve with a NULL argv. > > Account for the additional stack space in bprm_stack_limits(). Inject an > empty string when argc == 0 (and set argc = 1). Warn about the case so > userspace has some notice about the change: > > process './argc0' launched './argc0' with NULL argv: empty string added > > Additionally WARN() and reject NULL argv usage for kernel threads. > > [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ > [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > [5] https://github.com/KSPP/linux/issues/176 > [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 > [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ > > Reported-by: Ariadne Conill <ariadne@dereferenced.org> > Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Rich Felker <dalias@libc.org> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- Looks good, Acked-by: Christian Brauner <brauner@kernel.org>
On Mon, Jan 31, 2022 at 04:09:47PM -0800, Kees Cook wrote: > Quoting[1] Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > second argument to execve(2) be the name of a program, thus prohibiting > a scenario where argc < 1. POSIX 2017 also recommends this behaviour, > but it is not an explicit requirement[2]: > > The argument arg0 should point to a filename string that is > associated with the process being started by one of the exec > functions. > .... > Interestingly, Michael Kerrisk opened an issue about this in 2008[3], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[4] > of this bug in a shellcode, we can reconsider. > > This issue is being tracked in the KSPP issue tracker[5]." > > While the initial code searches[6][7] turned up what appeared to be > mostly corner case tests, trying to that just reject argv == NULL > (or an immediately terminated pointer list) quickly started tripping[8] > existing userspace programs. > > The next best approach is forcing a single empty string into argv and > adjusting argc to match. The number of programs depending on argc == 0 > seems a smaller set than those calling execve with a NULL argv. > > Account for the additional stack space in bprm_stack_limits(). Inject an > empty string when argc == 0 (and set argc = 1). Warn about the case so > userspace has some notice about the change: > > process './argc0' launched './argc0' with NULL argv: empty string added > > Additionally WARN() and reject NULL argv usage for kernel threads. > > [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ > [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > [5] https://github.com/KSPP/linux/issues/176 > [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 > [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ > > Reported-by: Ariadne Conill <ariadne@dereferenced.org> > Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Rich Felker <dalias@libc.org> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/exec.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 79f2c9483302..bbf3aadf7ce1 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -495,8 +495,14 @@ static int bprm_stack_limits(struct linux_binprm *bprm) > * the stack. They aren't stored until much later when we can't > * signal to the parent that the child has run out of stack space. > * Instead, calculate it here so it's possible to fail gracefully. > + * > + * In the case of argc = 0, make sure there is space for adding a > + * empty string (which will bump argc to 1), to ensure confused > + * userspace programs don't start processing from argv[1], thinking > + * argc can never be 0, to keep them from walking envp by accident. > + * See do_execveat_common(). > */ > - ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > + ptr_size = (min(bprm->argc, 1) + bprm->envc) * sizeof(void *); From #musl: <mixi> kees: shouldn't the min(bprm->argc, 1) be max(...) in your patch? I'm pretty sure without fixing that, you're introducing a giant vuln here. I believe this is the second time a patch attempting to fix this non-vuln has proposed adding a new vuln... Rich
On February 1, 2022 6:53:25 AM PST, Rich Felker <dalias@libc.org> wrote: >On Mon, Jan 31, 2022 at 04:09:47PM -0800, Kees Cook wrote: >> Quoting[1] Ariadne Conill: >> >> "In several other operating systems, it is a hard requirement that the >> second argument to execve(2) be the name of a program, thus prohibiting >> a scenario where argc < 1. POSIX 2017 also recommends this behaviour, >> but it is not an explicit requirement[2]: >> >> The argument arg0 should point to a filename string that is >> associated with the process being started by one of the exec >> functions. >> .... >> Interestingly, Michael Kerrisk opened an issue about this in 2008[3], >> but there was no consensus to support fixing this issue then. >> Hopefully now that CVE-2021-4034 shows practical exploitative use[4] >> of this bug in a shellcode, we can reconsider. >> >> This issue is being tracked in the KSPP issue tracker[5]." >> >> While the initial code searches[6][7] turned up what appeared to be >> mostly corner case tests, trying to that just reject argv == NULL >> (or an immediately terminated pointer list) quickly started tripping[8] >> existing userspace programs. >> >> The next best approach is forcing a single empty string into argv and >> adjusting argc to match. The number of programs depending on argc == 0 >> seems a smaller set than those calling execve with a NULL argv. >> >> Account for the additional stack space in bprm_stack_limits(). Inject an >> empty string when argc == 0 (and set argc = 1). Warn about the case so >> userspace has some notice about the change: >> >> process './argc0' launched './argc0' with NULL argv: empty string added >> >> Additionally WARN() and reject NULL argv usage for kernel threads. >> >> [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ >> [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html >> [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 >> [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt >> [5] https://github.com/KSPP/linux/issues/176 >> [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 >> [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 >> [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ >> >> Reported-by: Ariadne Conill <ariadne@dereferenced.org> >> Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> >> Cc: Matthew Wilcox <willy@infradead.org> >> Cc: Christian Brauner <brauner@kernel.org> >> Cc: Rich Felker <dalias@libc.org> >> Cc: Eric Biederman <ebiederm@xmission.com> >> Cc: Alexander Viro <viro@zeniv.linux.org.uk> >> Cc: linux-fsdevel@vger.kernel.org >> Cc: stable@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> fs/exec.c | 26 +++++++++++++++++++++++++- >> 1 file changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 79f2c9483302..bbf3aadf7ce1 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -495,8 +495,14 @@ static int bprm_stack_limits(struct linux_binprm *bprm) >> * the stack. They aren't stored until much later when we can't >> * signal to the parent that the child has run out of stack space. >> * Instead, calculate it here so it's possible to fail gracefully. >> + * >> + * In the case of argc = 0, make sure there is space for adding a >> + * empty string (which will bump argc to 1), to ensure confused >> + * userspace programs don't start processing from argv[1], thinking >> + * argc can never be 0, to keep them from walking envp by accident. >> + * See do_execveat_common(). >> */ >> - ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); >> + ptr_size = (min(bprm->argc, 1) + bprm->envc) * sizeof(void *); > >From #musl: > ><mixi> kees: shouldn't the min(bprm->argc, 1) be max(...) in your patch? Fix has already been sent, yup. >I'm pretty sure without fixing that, you're introducing a giant vuln >here. I wouldn't say "giant", but yes, it weakened a defense in depth for avoiding high stack utilization. > I believe this is the second time a patch attempting to fix this >non-vuln has proposed adding a new vuln... Mistakes happen, and that's why there is review and testing. Thank you for being part of the review process! :) -Kees
On Wed, Feb 02, 2022 at 07:50:42AM -0800, Kees Cook wrote: > > > On February 1, 2022 6:53:25 AM PST, Rich Felker <dalias@libc.org> wrote: > >On Mon, Jan 31, 2022 at 04:09:47PM -0800, Kees Cook wrote: > >> Quoting[1] Ariadne Conill: > >> > >> "In several other operating systems, it is a hard requirement that the > >> second argument to execve(2) be the name of a program, thus prohibiting > >> a scenario where argc < 1. POSIX 2017 also recommends this behaviour, > >> but it is not an explicit requirement[2]: > >> > >> The argument arg0 should point to a filename string that is > >> associated with the process being started by one of the exec > >> functions. > >> .... > >> Interestingly, Michael Kerrisk opened an issue about this in 2008[3], > >> but there was no consensus to support fixing this issue then. > >> Hopefully now that CVE-2021-4034 shows practical exploitative use[4] > >> of this bug in a shellcode, we can reconsider. > >> > >> This issue is being tracked in the KSPP issue tracker[5]." > >> > >> While the initial code searches[6][7] turned up what appeared to be > >> mostly corner case tests, trying to that just reject argv == NULL > >> (or an immediately terminated pointer list) quickly started tripping[8] > >> existing userspace programs. > >> > >> The next best approach is forcing a single empty string into argv and > >> adjusting argc to match. The number of programs depending on argc == 0 > >> seems a smaller set than those calling execve with a NULL argv. > >> > >> Account for the additional stack space in bprm_stack_limits(). Inject an > >> empty string when argc == 0 (and set argc = 1). Warn about the case so > >> userspace has some notice about the change: > >> > >> process './argc0' launched './argc0' with NULL argv: empty string added > >> > >> Additionally WARN() and reject NULL argv usage for kernel threads. > >> > >> [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ > >> [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > >> [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > >> [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > >> [5] https://github.com/KSPP/linux/issues/176 > >> [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > >> [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 > >> [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ > >> > >> Reported-by: Ariadne Conill <ariadne@dereferenced.org> > >> Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> > >> Cc: Matthew Wilcox <willy@infradead.org> > >> Cc: Christian Brauner <brauner@kernel.org> > >> Cc: Rich Felker <dalias@libc.org> > >> Cc: Eric Biederman <ebiederm@xmission.com> > >> Cc: Alexander Viro <viro@zeniv.linux.org.uk> > >> Cc: linux-fsdevel@vger.kernel.org > >> Cc: stable@vger.kernel.org > >> Signed-off-by: Kees Cook <keescook@chromium.org> > >> --- > >> fs/exec.c | 26 +++++++++++++++++++++++++- > >> 1 file changed, 25 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/exec.c b/fs/exec.c > >> index 79f2c9483302..bbf3aadf7ce1 100644 > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -495,8 +495,14 @@ static int bprm_stack_limits(struct linux_binprm *bprm) > >> * the stack. They aren't stored until much later when we can't > >> * signal to the parent that the child has run out of stack space. > >> * Instead, calculate it here so it's possible to fail gracefully. > >> + * > >> + * In the case of argc = 0, make sure there is space for adding a > >> + * empty string (which will bump argc to 1), to ensure confused > >> + * userspace programs don't start processing from argv[1], thinking > >> + * argc can never be 0, to keep them from walking envp by accident. > >> + * See do_execveat_common(). > >> */ > >> - ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + ptr_size = (min(bprm->argc, 1) + bprm->envc) * sizeof(void *); > > > >From #musl: > > > ><mixi> kees: shouldn't the min(bprm->argc, 1) be max(...) in your patch? > > Fix has already been sent, yup. > > >I'm pretty sure without fixing that, you're introducing a giant vuln > >here. > > I wouldn't say "giant", but yes, it weakened a defense in depth for > avoiding high stack utilization. I thought it was deciding the amount of memory to allocate/reserve for the arg slots, but based on the comment it looks like it's just a way to fail early rather than making the new process image fault later if they don't fit. > > I believe this is the second time a patch attempting to fix this > >non-vuln has proposed adding a new vuln... > > Mistakes happen, and that's why there is review and testing. Thank > you for being part of the review process! :) I know, and I'm sorry for being a bit hostile over it, and for jumping the gun about the severity. I just get frustrated when I see a rush to make changes over an incidental part of a popularized vuln, with disproportionate weight on "doing something" and not enough on being careful. Rich
On Wed, Feb 02, 2022 at 12:12:19PM -0500, Rich Felker wrote: > On Wed, Feb 02, 2022 at 07:50:42AM -0800, Kees Cook wrote: > > On February 1, 2022 6:53:25 AM PST, Rich Felker <dalias@libc.org> wrote: > > >From #musl: > > > > > ><mixi> kees: shouldn't the min(bprm->argc, 1) be max(...) in your patch? > > > > Fix has already been sent, yup. > > > > >I'm pretty sure without fixing that, you're introducing a giant vuln > > >here. > > > > I wouldn't say "giant", but yes, it weakened a defense in depth for > > avoiding high stack utilization. > > I thought it was deciding the amount of memory to allocate/reserve for > the arg slots, but based on the comment it looks like it's just a way > to fail early rather than making the new process image fault later if > they don't fit. Right. > > > I believe this is the second time a patch attempting to fix this > > >non-vuln has proposed adding a new vuln... > > > > Mistakes happen, and that's why there is review and testing. Thank > > you for being part of the review process! :) > > I know, and I'm sorry for being a bit hostile over it, and for jumping > the gun about the severity. I just get frustrated when I see a rush to > make changes over an incidental part of a popularized vuln, with > disproportionate weight on "doing something" and not enough on being > careful. Sure, I can see it looks that way. My sense of urgency on this in particular is that we're early in the development cycle, and it's an ABI-breaking change, so I want to maximize how much time it has to get tested out in real workloads. (i.e. we've now seen that just rejecting NULL argv is likely too painful, etc.) All that said, I regularly bemoan the lack of sufficient regression tests for execve() and the binfmt_*.c loaders. I've written a couple, and so have others, but what I really want is a library of "binary that got broken by exec change" for doing regression testing against. That gets hampered by both size, redistribution issues, etc, so I've wanted to have minimal reproducers for each, but creating those take time, etc, etc. (And you'll note I wrote[1] a test for this particular behavior, because I'm trying to avoid falling further behind in test coverage.) I would _love_ it if someone had the time and attention to go through all the past binaries and make a regression test series. :) -Kees [1] https://lore.kernel.org/linux-hardening/20220201011637.2457646-1-keescook@chromium.org/
On Tue, Feb 01, 2022 at 09:17:47AM +0000, David Laight wrote: > From: Kees Cook > > Sent: 01 February 2022 00:10 > ... > > While the initial code searches[6][7] turned up what appeared to be > > mostly corner case tests, trying to that just reject argv == NULL > > (or an immediately terminated pointer list) quickly started tripping[8] > > existing userspace programs. > > > > The next best approach is forcing a single empty string into argv and > > adjusting argc to match. The number of programs depending on argc == 0 > > seems a smaller set than those calling execve with a NULL argv. > > Has anyone considered using the pathname for argv[0]? > So converting: > execl(path, NULL); > into: > execl(path, path, NULL); > > I've not spotted any such suggestion. It came up on some IRC discussions at some point. I'm personally not a fan of this because it creates a bit of "new" ABI that has a lot of variability (depending on "" is one thing, but depending on a "missing" argv matching the exec path is very different). I think there were also concerns about dealing with fd-based exec ("what is the 'right' name"), etc. I'd prefer we stay as simple as possible for this change.
diff --git a/fs/exec.c b/fs/exec.c index 79f2c9483302..bbf3aadf7ce1 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -495,8 +495,14 @@ static int bprm_stack_limits(struct linux_binprm *bprm) * the stack. They aren't stored until much later when we can't * signal to the parent that the child has run out of stack space. * Instead, calculate it here so it's possible to fail gracefully. + * + * In the case of argc = 0, make sure there is space for adding a + * empty string (which will bump argc to 1), to ensure confused + * userspace programs don't start processing from argv[1], thinking + * argc can never be 0, to keep them from walking envp by accident. + * See do_execveat_common(). */ - ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); + ptr_size = (min(bprm->argc, 1) + bprm->envc) * sizeof(void *); if (limit <= ptr_size) return -E2BIG; limit -= ptr_size; @@ -1897,6 +1903,9 @@ static int do_execveat_common(int fd, struct filename *filename, } retval = count(argv, MAX_ARG_STRINGS); + if (retval == 0) + pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n", + current->comm, bprm->filename); if (retval < 0) goto out_free; bprm->argc = retval; @@ -1923,6 +1932,19 @@ static int do_execveat_common(int fd, struct filename *filename, if (retval < 0) goto out_free; + /* + * When argv is empty, add an empty string ("") as argv[0] to + * ensure confused userspace programs that start processing + * from argv[1] won't end up walking envp. See also + * bprm_stack_limits(). + */ + if (bprm->argc == 0) { + retval = copy_string_kernel("", bprm); + if (retval < 0) + goto out_free; + bprm->argc = 1; + } + retval = bprm_execve(bprm, fd, filename, flags); out_free: free_bprm(bprm); @@ -1951,6 +1973,8 @@ int kernel_execve(const char *kernel_filename, } retval = count_strings_kernel(argv); + if (WARN_ON_ONCE(retval == 0)) + retval = -EINVAL; if (retval < 0) goto out_free; bprm->argc = retval;
Quoting[1] Ariadne Conill: "In several other operating systems, it is a hard requirement that the second argument to execve(2) be the name of a program, thus prohibiting a scenario where argc < 1. POSIX 2017 also recommends this behaviour, but it is not an explicit requirement[2]: The argument arg0 should point to a filename string that is associated with the process being started by one of the exec functions. ... Interestingly, Michael Kerrisk opened an issue about this in 2008[3], but there was no consensus to support fixing this issue then. Hopefully now that CVE-2021-4034 shows practical exploitative use[4] of this bug in a shellcode, we can reconsider. This issue is being tracked in the KSPP issue tracker[5]." While the initial code searches[6][7] turned up what appeared to be mostly corner case tests, trying to that just reject argv == NULL (or an immediately terminated pointer list) quickly started tripping[8] existing userspace programs. The next best approach is forcing a single empty string into argv and adjusting argc to match. The number of programs depending on argc == 0 seems a smaller set than those calling execve with a NULL argv. Account for the additional stack space in bprm_stack_limits(). Inject an empty string when argc == 0 (and set argc = 1). Warn about the case so userspace has some notice about the change: process './argc0' launched './argc0' with NULL argv: empty string added Additionally WARN() and reject NULL argv usage for kernel threads. [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/ [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408 [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt [5] https://github.com/KSPP/linux/issues/176 [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0 [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/ Reported-by: Ariadne Conill <ariadne@dereferenced.org> Reported-by: Michael Kerrisk <mtk.manpages@gmail.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Christian Brauner <brauner@kernel.org> Cc: Rich Felker <dalias@libc.org> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: linux-fsdevel@vger.kernel.org Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/exec.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)