| Message ID | 20220126175747.3270945-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fs/binfmt_elf: Add padding NULL when argc == 0 | expand |
On Wed, Jan 26, 2022 at 6:58 PM Kees Cook <keescook@chromium.org> wrote: > Quoting Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > first 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[1]: > > 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[2], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > of this bug in a shellcode, we can reconsider." > > An examination of existing[4] users of execve(..., NULL, NULL) shows > mostly test code, or example rootkit code. While rejecting a NULL argv > would be preferred, it looks like the main cause of userspace confusion > is an assumption that argc >= 1, and buggy programs may skip argv[0] > when iterating. To protect against userspace bugs of this nature, insert > an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > > Note that this is only done in the argc == 0 case because some userspace > programs expect to find envp at exactly argv[argc]. The overlap of these > two misguided assumptions is believed to be zero. Will this result in the executed program being told that argc==0 but having an extra NULL pointer on the stack? If so, I believe this breaks the x86-64 ABI documented at https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf - page 29, figure 3.9 describes the layout of the initial process stack. Actually, does this even work? Can a program still properly access its environment variables when invoked with argc==0 with this patch applied? AFAIU the way userspace locates envv on x86-64 is by calculating 8*(argc+1)?
Hi, On Wed, 26 Jan 2022, Jann Horn wrote: > On Wed, Jan 26, 2022 at 6:58 PM Kees Cook <keescook@chromium.org> wrote: >> Quoting Ariadne Conill: >> >> "In several other operating systems, it is a hard requirement that the >> first 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[1]: >> >> 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[2], >> but there was no consensus to support fixing this issue then. >> Hopefully now that CVE-2021-4034 shows practical exploitative use[3] >> of this bug in a shellcode, we can reconsider." >> >> An examination of existing[4] users of execve(..., NULL, NULL) shows >> mostly test code, or example rootkit code. While rejecting a NULL argv >> would be preferred, it looks like the main cause of userspace confusion >> is an assumption that argc >= 1, and buggy programs may skip argv[0] >> when iterating. To protect against userspace bugs of this nature, insert >> an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. >> >> Note that this is only done in the argc == 0 case because some userspace >> programs expect to find envp at exactly argv[argc]. The overlap of these >> two misguided assumptions is believed to be zero. > > Will this result in the executed program being told that argc==0 but > having an extra NULL pointer on the stack? > If so, I believe this breaks the x86-64 ABI documented at > https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf - page 29, > figure 3.9 describes the layout of the initial process stack. I'm presently compiling a kernel with the patch to see if it works or not. > Actually, does this even work? Can a program still properly access its > environment variables when invoked with argc==0 with this patch > applied? AFAIU the way userspace locates envv on x86-64 is by > calculating 8*(argc+1)? In the other thread, it was suggested that perhaps we should set up an argv of {"", NULL}. In that case, it seems like it would be safe to claim argc == 1. What do you think? Ariadne
On Wed, Jan 26, 2022 at 7:42 PM Ariadne Conill <ariadne@dereferenced.org> wrote: > On Wed, 26 Jan 2022, Jann Horn wrote: > > On Wed, Jan 26, 2022 at 6:58 PM Kees Cook <keescook@chromium.org> wrote: > >> Quoting Ariadne Conill: > >> > >> "In several other operating systems, it is a hard requirement that the > >> first 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[1]: > >> > >> 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[2], > >> but there was no consensus to support fixing this issue then. > >> Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > >> of this bug in a shellcode, we can reconsider." > >> > >> An examination of existing[4] users of execve(..., NULL, NULL) shows > >> mostly test code, or example rootkit code. While rejecting a NULL argv > >> would be preferred, it looks like the main cause of userspace confusion > >> is an assumption that argc >= 1, and buggy programs may skip argv[0] > >> when iterating. To protect against userspace bugs of this nature, insert > >> an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > >> > >> Note that this is only done in the argc == 0 case because some userspace > >> programs expect to find envp at exactly argv[argc]. The overlap of these > >> two misguided assumptions is believed to be zero. > > > > Will this result in the executed program being told that argc==0 but > > having an extra NULL pointer on the stack? > > If so, I believe this breaks the x86-64 ABI documented at > > https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf - page 29, > > figure 3.9 describes the layout of the initial process stack. > > I'm presently compiling a kernel with the patch to see if it works or not. > > > Actually, does this even work? Can a program still properly access its > > environment variables when invoked with argc==0 with this patch > > applied? AFAIU the way userspace locates envv on x86-64 is by > > calculating 8*(argc+1)? > > In the other thread, it was suggested that perhaps we should set up an > argv of {"", NULL}. In that case, it seems like it would be safe to claim > argc == 1. > > What do you think? Sounds good to me, since that's something that could also happen normally if userspace calls execve(..., {"", NULL}, ...). (I'd like it even better if we could just bail out with an error code, but I guess the risk of breakage might be too high with that approach?)
On Wed, Jan 26, 2022 at 07:07:20PM +0100, Jann Horn wrote: > On Wed, Jan 26, 2022 at 6:58 PM Kees Cook <keescook@chromium.org> wrote: > > Quoting Ariadne Conill: > > > > "In several other operating systems, it is a hard requirement that the > > first 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[1]: > > > > 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[2], > > but there was no consensus to support fixing this issue then. > > Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > > of this bug in a shellcode, we can reconsider." > > > > An examination of existing[4] users of execve(..., NULL, NULL) shows > > mostly test code, or example rootkit code. While rejecting a NULL argv > > would be preferred, it looks like the main cause of userspace confusion > > is an assumption that argc >= 1, and buggy programs may skip argv[0] > > when iterating. To protect against userspace bugs of this nature, insert > > an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > > > > Note that this is only done in the argc == 0 case because some userspace > > programs expect to find envp at exactly argv[argc]. The overlap of these > > two misguided assumptions is believed to be zero. > > Will this result in the executed program being told that argc==0 but > having an extra NULL pointer on the stack? > If so, I believe this breaks the x86-64 ABI documented at > https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf - page 29, > figure 3.9 describes the layout of the initial process stack. > > Actually, does this even work? Can a program still properly access its > environment variables when invoked with argc==0 with this patch > applied? AFAIU the way userspace locates envv on x86-64 is by > calculating 8*(argc+1)? Hrm, yeah, I guess it's libc providing the envp pointer; it's not passes separately. Hrm.
On Wed, Jan 26, 2022 at 08:50:39PM +0100, Jann Horn wrote: > On Wed, Jan 26, 2022 at 7:42 PM Ariadne Conill <ariadne@dereferenced.org> wrote: > > On Wed, 26 Jan 2022, Jann Horn wrote: > > > On Wed, Jan 26, 2022 at 6:58 PM Kees Cook <keescook@chromium.org> wrote: > > >> Quoting Ariadne Conill: > > >> > > >> "In several other operating systems, it is a hard requirement that the > > >> first 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[1]: > > >> > > >> 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[2], > > >> but there was no consensus to support fixing this issue then. > > >> Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > > >> of this bug in a shellcode, we can reconsider." > > >> > > >> An examination of existing[4] users of execve(..., NULL, NULL) shows > > >> mostly test code, or example rootkit code. While rejecting a NULL argv > > >> would be preferred, it looks like the main cause of userspace confusion > > >> is an assumption that argc >= 1, and buggy programs may skip argv[0] > > >> when iterating. To protect against userspace bugs of this nature, insert > > >> an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > > >> > > >> Note that this is only done in the argc == 0 case because some userspace > > >> programs expect to find envp at exactly argv[argc]. The overlap of these > > >> two misguided assumptions is believed to be zero. > > > > > > Will this result in the executed program being told that argc==0 but > > > having an extra NULL pointer on the stack? > > > If so, I believe this breaks the x86-64 ABI documented at > > > https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf - page 29, > > > figure 3.9 describes the layout of the initial process stack. > > > > I'm presently compiling a kernel with the patch to see if it works or not. > > > > > Actually, does this even work? Can a program still properly access its > > > environment variables when invoked with argc==0 with this patch > > > applied? AFAIU the way userspace locates envv on x86-64 is by > > > calculating 8*(argc+1)? > > > > In the other thread, it was suggested that perhaps we should set up an > > argv of {"", NULL}. In that case, it seems like it would be safe to claim > > argc == 1. > > > > What do you think? > > Sounds good to me, since that's something that could also happen > normally if userspace calls execve(..., {"", NULL}, ...). > > (I'd like it even better if we could just bail out with an error code, > but I guess the risk of breakage might be too high with that > approach?) We can't mutate argc; it'll turn at least some userspace into an infinite loop: https://sources.debian.org/src/valgrind/1:3.18.1-1/none/tests/execve.c/?hl=22#L22
On Wed, Jan 26, 2022 at 11:58:39AM -0800, Kees Cook wrote: > On Wed, Jan 26, 2022 at 08:50:39PM +0100, Jann Horn wrote: > > On Wed, Jan 26, 2022 at 7:42 PM Ariadne Conill <ariadne@dereferenced.org> wrote: > > > On Wed, 26 Jan 2022, Jann Horn wrote: > > > > On Wed, Jan 26, 2022 at 6:58 PM Kees Cook <keescook@chromium.org> wrote: > > > >> Quoting Ariadne Conill: > > > >> > > > >> "In several other operating systems, it is a hard requirement that the > > > >> first 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[1]: > > > >> > > > >> 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[2], > > > >> but there was no consensus to support fixing this issue then. > > > >> Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > > > >> of this bug in a shellcode, we can reconsider." > > > >> > > > >> An examination of existing[4] users of execve(..., NULL, NULL) shows > > > >> mostly test code, or example rootkit code. While rejecting a NULL argv > > > >> would be preferred, it looks like the main cause of userspace confusion > > > >> is an assumption that argc >= 1, and buggy programs may skip argv[0] > > > >> when iterating. To protect against userspace bugs of this nature, insert > > > >> an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > > > >> > > > >> Note that this is only done in the argc == 0 case because some userspace > > > >> programs expect to find envp at exactly argv[argc]. The overlap of these > > > >> two misguided assumptions is believed to be zero. > > > > > > > > Will this result in the executed program being told that argc==0 but > > > > having an extra NULL pointer on the stack? > > > > If so, I believe this breaks the x86-64 ABI documented at > > > > https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf - page 29, > > > > figure 3.9 describes the layout of the initial process stack. > > > > > > I'm presently compiling a kernel with the patch to see if it works or not. > > > > > > > Actually, does this even work? Can a program still properly access its > > > > environment variables when invoked with argc==0 with this patch > > > > applied? AFAIU the way userspace locates envv on x86-64 is by > > > > calculating 8*(argc+1)? > > > > > > In the other thread, it was suggested that perhaps we should set up an > > > argv of {"", NULL}. In that case, it seems like it would be safe to claim > > > argc == 1. > > > > > > What do you think? > > > > Sounds good to me, since that's something that could also happen > > normally if userspace calls execve(..., {"", NULL}, ...). > > > > (I'd like it even better if we could just bail out with an error code, > > but I guess the risk of breakage might be too high with that > > approach?) > > We can't mutate argc; it'll turn at least some userspace into an > infinite loop: > https://sources.debian.org/src/valgrind/1:3.18.1-1/none/tests/execve.c/?hl=22#L22 How does that become an infinite loop? We obviously wouldn't mutate argc in the caller, just the callee. Also, there's a version of this where we only mutate argc if we're executing a setuid program, which would remove the privilege escalation part of things.
Hi, On Wed, 26 Jan 2022, Kees Cook wrote: > Quoting Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > first 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[1]: > > 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[2], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > of this bug in a shellcode, we can reconsider." > > An examination of existing[4] users of execve(..., NULL, NULL) shows > mostly test code, or example rootkit code. While rejecting a NULL argv > would be preferred, it looks like the main cause of userspace confusion > is an assumption that argc >= 1, and buggy programs may skip argv[0] > when iterating. To protect against userspace bugs of this nature, insert > an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > > Note that this is only done in the argc == 0 case because some userspace > programs expect to find envp at exactly argv[argc]. The overlap of these > two misguided assumptions is believed to be zero. > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > [2] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > [3] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > [4] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > > 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> Tested-by: Ariadne Conill <ariadne@dereferenced.org> It seems to work, but I still think bailing early with -EINVAL is a more reasonable position to take. For example, the following code, when used with BusyBox applets results in a segfault, as the multicall stub does not support scenarios where argc < 1: #include <stdio.h> #include <unistd.h> #include <sys/syscall.h> int main(int argc, const char **argv) { if (syscall(SYS_execve, "/bin/date", NULL, NULL) < 0) perror("execve"); return 0; } Ariadne
On Wed, Jan 26, 2022 at 08:08:14PM +0000, Matthew Wilcox wrote: > On Wed, Jan 26, 2022 at 11:58:39AM -0800, Kees Cook wrote: > > We can't mutate argc; it'll turn at least some userspace into an > > infinite loop: > > https://sources.debian.org/src/valgrind/1:3.18.1-1/none/tests/execve.c/?hl=22#L22 > > How does that become an infinite loop? We obviously wouldn't mutate > argc in the caller, just the callee. Oh, sorry, I misread. It's using /bin/true, not argv[0] (another bit of code I found was using argv[0]). Yeah, {"", NULL} could work. > Also, there's a version of this where we only mutate argc if we're > executing a setuid program, which would remove the privilege > escalation part of things. True; though I'd like to keep the logic as non-specialized as possible. I don't like making stuff conditional on privilege boundaries if we can make it always happen.
Kees Cook <keescook@chromium.org> writes: > On Wed, Jan 26, 2022 at 08:08:14PM +0000, Matthew Wilcox wrote: >> On Wed, Jan 26, 2022 at 11:58:39AM -0800, Kees Cook wrote: >> > We can't mutate argc; it'll turn at least some userspace into an >> > infinite loop: >> > https://sources.debian.org/src/valgrind/1:3.18.1-1/none/tests/execve.c/?hl=22#L22 >> >> How does that become an infinite loop? We obviously wouldn't mutate >> argc in the caller, just the callee. > > Oh, sorry, I misread. It's using /bin/true, not argv[0] (another bit of > code I found was using argv[0]). Yeah, {"", NULL} could work. > >> Also, there's a version of this where we only mutate argc if we're >> executing a setuid program, which would remove the privilege >> escalation part of things. > > True; though I'd like to keep the logic as non-specialized as possible. > I don't like making stuff conditional on privilege boundaries if we can > make it always happen. Which I think means turning the argc == 0 case into { "", NULL }. I think we can always do that, and it is already valid in userspace. The only case I can imagine breaking would be an explicitly testing for argc == 0 and behaving completely differently if that is passed to the program. Eric
Hi, On Wed, 26 Jan 2022, Ariadne Conill wrote: > Hi, > > On Wed, 26 Jan 2022, Kees Cook wrote: > >> Quoting Ariadne Conill: >> >> "In several other operating systems, it is a hard requirement that the >> first 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[1]: >> >> 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[2], >> but there was no consensus to support fixing this issue then. >> Hopefully now that CVE-2021-4034 shows practical exploitative use[3] >> of this bug in a shellcode, we can reconsider." >> >> An examination of existing[4] users of execve(..., NULL, NULL) shows >> mostly test code, or example rootkit code. While rejecting a NULL argv >> would be preferred, it looks like the main cause of userspace confusion >> is an assumption that argc >= 1, and buggy programs may skip argv[0] >> when iterating. To protect against userspace bugs of this nature, insert >> an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. >> >> Note that this is only done in the argc == 0 case because some userspace >> programs expect to find envp at exactly argv[argc]. The overlap of these >> two misguided assumptions is believed to be zero. >> >> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html >> [2] https://bugzilla.kernel.org/show_bug.cgi?id=8408 >> [3] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt >> [4] >> https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 >> >> 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> > > Tested-by: Ariadne Conill <ariadne@dereferenced.org> > > It seems to work, but I still think bailing early with -EINVAL is a more > reasonable position to take. For example, the following code, when used with > BusyBox applets results in a segfault, as the multicall stub does not support > scenarios where argc < 1: > > #include <stdio.h> > #include <unistd.h> > #include <sys/syscall.h> > > int main(int argc, const char **argv) { > if (syscall(SYS_execve, "/bin/date", NULL, NULL) < 0) > perror("execve"); > return 0; > } > Further testing indicates that while things *mostly* work, it results in memory corruption in various tasks, for example, trying to build a new kernel hung, and the gcc process's name was a bunch of uninitialized memory. So, I don't think { NULL, NULL } is a good way to go. Ariadne
On Wed, Jan 26, 2022 at 09:57:47AM -0800, Kees Cook wrote: > Quoting Ariadne Conill: > > "In several other operating systems, it is a hard requirement that the > first 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[1]: > > 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[2], > but there was no consensus to support fixing this issue then. > Hopefully now that CVE-2021-4034 shows practical exploitative use[3] > of this bug in a shellcode, we can reconsider." > > An examination of existing[4] users of execve(..., NULL, NULL) shows > mostly test code, or example rootkit code. While rejecting a NULL argv > would be preferred, it looks like the main cause of userspace confusion > is an assumption that argc >= 1, and buggy programs may skip argv[0] > when iterating. To protect against userspace bugs of this nature, insert > an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. > > Note that this is only done in the argc == 0 case because some userspace > programs expect to find envp at exactly argv[argc]. The overlap of these > two misguided assumptions is believed to be zero. > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html > [2] https://bugzilla.kernel.org/show_bug.cgi?id=8408 > [3] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt > [4] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 > > 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/binfmt_elf.c | 10 +++++++++- > fs/exec.c | 7 ++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 605017eb9349..e456c48658ad 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -297,7 +297,8 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, > ei_index = elf_info - (elf_addr_t *)mm->saved_auxv; > sp = STACK_ADD(p, ei_index); > > - items = (argc + 1) + (envc + 1) + 1; > + /* Make room for extra pointer when argc == 0. See below. */ > + items = (min(argc, 1) + 1) + (envc + 1) + 1; > bprm->p = STACK_ROUND(sp, items); > > /* Point sp at the lowest address on the stack */ > @@ -326,6 +327,13 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, > > /* Populate list of argv pointers back to argv strings. */ > p = mm->arg_end = mm->arg_start; > + /* > + * Include an extra NULL pointer in argv when argc == 0 so > + * that argv[1] != envp[0] to help userspace programs from > + * mishandling argc == 0. See fs/exec.c bprm_stack_limits(). > + */ > + if (argc == 0 && put_user(0, sp++)) > + return -EFAULT; > while (argc-- > 0) { > size_t len; > if (put_user((elf_addr_t)p, sp++)) > diff --git a/fs/exec.c b/fs/exec.c > index 79f2c9483302..0b36384e55b1 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -495,8 +495,13 @@ 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 < 1, make sure there is a NULL pointer gap > + * between argv and envp to ensure confused userspace programs don't > + * start processing from argv[1], thinking argc can never be 0, > + * to block them from walking envp by accident. See fs/binfmt_elf.c. > */ > - 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; > -- > 2.30.2 > This patch is not just wrong, but extremely dangerously wrong, to the point that it may make all suid-root binaries exploitable (at least dynamic linked ones). The ELF entry point contract is that argv+argc+1==envp, and in fact this is the "preferred" way of computing envp so as to avoid linear search over argv. In musl's dynamic linker we do exactly that; I'm not sure about glibc's. See: https://git.musl-libc.org/cgit/musl/tree/ldso/dynlink.c?id=v1.2.2#n1740 If argv[argc+1] wrongly contains a null pointer, semantically, that means the environment is empty and auxv starts at the next stack slot. It's an exercise for the reader to populate the environment in a way that this memory wrongly gets interpreted as a meaningful auxv. I'm not sure this is possible, but I wouldn't automatically rule it out. In short: YOU CANNOT CHANGE/BREAK CONTRACTS TO MITIGATE A VULN. Doing so just makes new vulns in the programs that were correct before. Silently replacing argc==0 with argc==1 and argv[0]=="" would be a safe variant of this, but I'm really in favor of just erroring out, but *only doing it when the exec is a privilege boundary* (suid/etc.) to minimize the chance of breaking software dependent on allowing argc==0. Rich
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 605017eb9349..e456c48658ad 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -297,7 +297,8 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, ei_index = elf_info - (elf_addr_t *)mm->saved_auxv; sp = STACK_ADD(p, ei_index); - items = (argc + 1) + (envc + 1) + 1; + /* Make room for extra pointer when argc == 0. See below. */ + items = (min(argc, 1) + 1) + (envc + 1) + 1; bprm->p = STACK_ROUND(sp, items); /* Point sp at the lowest address on the stack */ @@ -326,6 +327,13 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, /* Populate list of argv pointers back to argv strings. */ p = mm->arg_end = mm->arg_start; + /* + * Include an extra NULL pointer in argv when argc == 0 so + * that argv[1] != envp[0] to help userspace programs from + * mishandling argc == 0. See fs/exec.c bprm_stack_limits(). + */ + if (argc == 0 && put_user(0, sp++)) + return -EFAULT; while (argc-- > 0) { size_t len; if (put_user((elf_addr_t)p, sp++)) diff --git a/fs/exec.c b/fs/exec.c index 79f2c9483302..0b36384e55b1 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -495,8 +495,13 @@ 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 < 1, make sure there is a NULL pointer gap + * between argv and envp to ensure confused userspace programs don't + * start processing from argv[1], thinking argc can never be 0, + * to block them from walking envp by accident. See fs/binfmt_elf.c. */ - 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;
Quoting Ariadne Conill: "In several other operating systems, it is a hard requirement that the first 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[1]: 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[2], but there was no consensus to support fixing this issue then. Hopefully now that CVE-2021-4034 shows practical exploitative use[3] of this bug in a shellcode, we can reconsider." An examination of existing[4] users of execve(..., NULL, NULL) shows mostly test code, or example rootkit code. While rejecting a NULL argv would be preferred, it looks like the main cause of userspace confusion is an assumption that argc >= 1, and buggy programs may skip argv[0] when iterating. To protect against userspace bugs of this nature, insert an extra NULL pointer in argv when argc == 0, so that argv[1] != envp[0]. Note that this is only done in the argc == 0 case because some userspace programs expect to find envp at exactly argv[argc]. The overlap of these two misguided assumptions is believed to be zero. [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html [2] https://bugzilla.kernel.org/show_bug.cgi?id=8408 [3] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt [4] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0 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/binfmt_elf.c | 10 +++++++++- fs/exec.c | 7 ++++++- 2 files changed, 15 insertions(+), 2 deletions(-)