* [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
@ 2022-01-26 17:57 Kees Cook
2022-01-26 18:07 ` Jann Horn
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Kees Cook @ 2022-01-26 17:57 UTC (permalink / raw)
To: Ariadne Conill
Cc: Kees Cook, Michael Kerrisk, Matthew Wilcox, Christian Brauner,
Rich Felker, Eric Biederman, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 17:57 [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0 Kees Cook
@ 2022-01-26 18:07 ` Jann Horn
2022-01-26 18:42 ` Ariadne Conill
2022-01-26 19:56 ` Kees Cook
2022-01-26 20:10 ` Ariadne Conill
2022-01-26 20:52 ` Rich Felker
2 siblings, 2 replies; 12+ messages in thread
From: Jann Horn @ 2022-01-26 18:07 UTC (permalink / raw)
To: Kees Cook
Cc: Ariadne Conill, Michael Kerrisk, Matthew Wilcox,
Christian Brauner, Rich Felker, Eric Biederman, Alexander Viro,
linux-fsdevel, stable, linux-kernel, linux-hardening
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)?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 18:07 ` Jann Horn
@ 2022-01-26 18:42 ` Ariadne Conill
2022-01-26 19:50 ` Jann Horn
2022-01-26 19:56 ` Kees Cook
1 sibling, 1 reply; 12+ messages in thread
From: Ariadne Conill @ 2022-01-26 18:42 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, Ariadne Conill, Michael Kerrisk, Matthew Wilcox,
Christian Brauner, Rich Felker, Eric Biederman, Alexander Viro,
linux-fsdevel, stable, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 18:42 ` Ariadne Conill
@ 2022-01-26 19:50 ` Jann Horn
2022-01-26 19:58 ` Kees Cook
0 siblings, 1 reply; 12+ messages in thread
From: Jann Horn @ 2022-01-26 19:50 UTC (permalink / raw)
To: Ariadne Conill
Cc: Kees Cook, Michael Kerrisk, Matthew Wilcox, Christian Brauner,
Rich Felker, Eric Biederman, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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?)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 18:07 ` Jann Horn
2022-01-26 18:42 ` Ariadne Conill
@ 2022-01-26 19:56 ` Kees Cook
1 sibling, 0 replies; 12+ messages in thread
From: Kees Cook @ 2022-01-26 19:56 UTC (permalink / raw)
To: Jann Horn
Cc: Ariadne Conill, Michael Kerrisk, Matthew Wilcox,
Christian Brauner, Rich Felker, Eric Biederman, Alexander Viro,
linux-fsdevel, stable, linux-kernel, linux-hardening
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.
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 19:50 ` Jann Horn
@ 2022-01-26 19:58 ` Kees Cook
2022-01-26 20:08 ` Matthew Wilcox
0 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-01-26 19:58 UTC (permalink / raw)
To: Jann Horn
Cc: Ariadne Conill, Michael Kerrisk, Matthew Wilcox,
Christian Brauner, Rich Felker, Eric Biederman, Alexander Viro,
linux-fsdevel, stable, linux-kernel, linux-hardening
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
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 19:58 ` Kees Cook
@ 2022-01-26 20:08 ` Matthew Wilcox
2022-01-26 20:13 ` Kees Cook
0 siblings, 1 reply; 12+ messages in thread
From: Matthew Wilcox @ 2022-01-26 20:08 UTC (permalink / raw)
To: Kees Cook
Cc: Jann Horn, Ariadne Conill, Michael Kerrisk, Christian Brauner,
Rich Felker, Eric Biederman, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 17:57 [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0 Kees Cook
2022-01-26 18:07 ` Jann Horn
@ 2022-01-26 20:10 ` Ariadne Conill
2022-01-26 20:46 ` Ariadne Conill
2022-01-26 20:52 ` Rich Felker
2 siblings, 1 reply; 12+ messages in thread
From: Ariadne Conill @ 2022-01-26 20:10 UTC (permalink / raw)
To: Kees Cook
Cc: Ariadne Conill, Michael Kerrisk, Matthew Wilcox,
Christian Brauner, Rich Felker, Eric Biederman, Alexander Viro,
linux-fsdevel, stable, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 20:08 ` Matthew Wilcox
@ 2022-01-26 20:13 ` Kees Cook
2022-01-26 20:31 ` Eric W. Biederman
0 siblings, 1 reply; 12+ messages in thread
From: Kees Cook @ 2022-01-26 20:13 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jann Horn, Ariadne Conill, Michael Kerrisk, Christian Brauner,
Rich Felker, Eric Biederman, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 20:13 ` Kees Cook
@ 2022-01-26 20:31 ` Eric W. Biederman
0 siblings, 0 replies; 12+ messages in thread
From: Eric W. Biederman @ 2022-01-26 20:31 UTC (permalink / raw)
To: Kees Cook
Cc: Matthew Wilcox, Jann Horn, Ariadne Conill, Michael Kerrisk,
Christian Brauner, Rich Felker, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 20:10 ` Ariadne Conill
@ 2022-01-26 20:46 ` Ariadne Conill
0 siblings, 0 replies; 12+ messages in thread
From: Ariadne Conill @ 2022-01-26 20:46 UTC (permalink / raw)
To: Ariadne Conill
Cc: Kees Cook, Michael Kerrisk, Matthew Wilcox, Christian Brauner,
Rich Felker, Eric Biederman, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0
2022-01-26 17:57 [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0 Kees Cook
2022-01-26 18:07 ` Jann Horn
2022-01-26 20:10 ` Ariadne Conill
@ 2022-01-26 20:52 ` Rich Felker
2 siblings, 0 replies; 12+ messages in thread
From: Rich Felker @ 2022-01-26 20:52 UTC (permalink / raw)
To: Kees Cook
Cc: Ariadne Conill, Michael Kerrisk, Matthew Wilcox,
Christian Brauner, Eric Biederman, Alexander Viro, linux-fsdevel,
stable, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-26 21:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-26 17:57 [PATCH] fs/binfmt_elf: Add padding NULL when argc == 0 Kees Cook
2022-01-26 18:07 ` Jann Horn
2022-01-26 18:42 ` Ariadne Conill
2022-01-26 19:50 ` Jann Horn
2022-01-26 19:58 ` Kees Cook
2022-01-26 20:08 ` Matthew Wilcox
2022-01-26 20:13 ` Kees Cook
2022-01-26 20:31 ` Eric W. Biederman
2022-01-26 19:56 ` Kees Cook
2022-01-26 20:10 ` Ariadne Conill
2022-01-26 20:46 ` Ariadne Conill
2022-01-26 20:52 ` Rich Felker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).