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[0]:
The argument arg0 should point to a filename string that is
associated with the process being started by one of the exec
functions.
To ensure that execve(2) with argc < 1 is not a useful tool for
shellcode to use, we can validate this in do_execveat_common() and
fail for this scenario, effectively blocking successful exploitation
of CVE-2021-4034 and similar bugs which depend on execve(2) working
with argc < 1.
We use -EINVAL for this case, mirroring recent changes to FreeBSD and
OpenBSD. -EINVAL is also used by QNX for this, while Solaris uses
-EFAULT.
In earlier versions of the patch, it was proposed that we create a
fake argv for applications to use when argc < 1, but it was concluded
that it would be better to just fail the execve(2) in these cases, as
launching a process with an empty or NULL argv[0] was likely to just
cause more problems.
Interestingly, Michael Kerrisk opened an issue about this in 2008[1],
but there was no consensus to support fixing this issue then.
Hopefully now that CVE-2021-4034 shows practical exploitative use[2]
of this bug in a shellcode, we can reconsider.
This issue is being tracked in the KSPP issue tracker[3].
There are a few[4][5] minor edge cases (primarily in test suites) that
are caught by this, but we plan to work with the projects to fix those
edge cases.
[0]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=8408
[2]: https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt
[3]: https://github.com/KSPP/linux/issues/176
[4]: https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0
[5]: https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0
Changes from v2:
- Switch to using -EINVAL as the error code for this.
- Use pr_warn_once() to warn when an execve(2) is rejected due to NULL
argv.
Changes from v1:
- Rework commit message significantly.
- Make the argv[0] check explicit rather than hijacking the error-check
for count().
Reported-by: Michael Kerrisk <[email protected]>
To: Andrew Morton <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Rich Felker <[email protected]>
Cc: Eric Biederman <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Ariadne Conill <[email protected]>
---
fs/exec.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/exec.c b/fs/exec.c
index 79f2c9483302..982730cfe3b8 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1897,6 +1897,10 @@ static int do_execveat_common(int fd, struct filename *filename,
}
retval = count(argv, MAX_ARG_STRINGS);
+ if (retval == 0) {
+ pr_warn_once("Attempted to run process '%s' with NULL argv\n", bprm->filename);
+ retval = -EINVAL;
+ }
if (retval < 0)
goto out_free;
bprm->argc = retval;
--
2.34.1
On Thu, Jan 27, 2022 at 12:07:24AM +0000, Ariadne Conill wrote:
> 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[0]:
>
> The argument arg0 should point to a filename string that is
> associated with the process being started by one of the exec
> functions.
>
> To ensure that execve(2) with argc < 1 is not a useful tool for
> shellcode to use, we can validate this in do_execveat_common() and
> fail for this scenario, effectively blocking successful exploitation
> of CVE-2021-4034 and similar bugs which depend on execve(2) working
> with argc < 1.
>
> We use -EINVAL for this case, mirroring recent changes to FreeBSD and
> OpenBSD. -EINVAL is also used by QNX for this, while Solaris uses
> -EFAULT.
>
> In earlier versions of the patch, it was proposed that we create a
> fake argv for applications to use when argc < 1, but it was concluded
> that it would be better to just fail the execve(2) in these cases, as
> launching a process with an empty or NULL argv[0] was likely to just
> cause more problems.
Let's do it and see what breaks. :)
I do see at least tools/testing/selftests/exec/recursion-depth.c will
need a fix. And maybe testcases/kernel/syscalls/execveat/execveat.h
in LTP.
Acked-by: Kees Cook <[email protected]>
>
> Interestingly, Michael Kerrisk opened an issue about this in 2008[1],
> but there was no consensus to support fixing this issue then.
> Hopefully now that CVE-2021-4034 shows practical exploitative use[2]
> of this bug in a shellcode, we can reconsider.
>
> This issue is being tracked in the KSPP issue tracker[3].
>
> There are a few[4][5] minor edge cases (primarily in test suites) that
> are caught by this, but we plan to work with the projects to fix those
> edge cases.
>
> [0]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=8408
> [2]: https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt
> [3]: https://github.com/KSPP/linux/issues/176
> [4]: https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0
> [5]: https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0
>
> Changes from v2:
> - Switch to using -EINVAL as the error code for this.
> - Use pr_warn_once() to warn when an execve(2) is rejected due to NULL
> argv.
>
> Changes from v1:
> - Rework commit message significantly.
> - Make the argv[0] check explicit rather than hijacking the error-check
> for count().
>
> Reported-by: Michael Kerrisk <[email protected]>
> To: Andrew Morton <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Rich Felker <[email protected]>
> Cc: Eric Biederman <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Ariadne Conill <[email protected]>
> ---
> fs/exec.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 79f2c9483302..982730cfe3b8 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1897,6 +1897,10 @@ static int do_execveat_common(int fd, struct filename *filename,
> }
>
> retval = count(argv, MAX_ARG_STRINGS);
> + if (retval == 0) {
> + pr_warn_once("Attempted to run process '%s' with NULL argv\n", bprm->filename);
> + retval = -EINVAL;
> + }
> if (retval < 0)
> goto out_free;
> bprm->argc = retval;
> --
> 2.34.1
>
--
Kees Cook
Kees Cook <[email protected]> writes:
> On Thu, Jan 27, 2022 at 12:07:24AM +0000, Ariadne Conill wrote:
>> 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[0]:
>>
>> The argument arg0 should point to a filename string that is
>> associated with the process being started by one of the exec
>> functions.
>>
>> To ensure that execve(2) with argc < 1 is not a useful tool for
>> shellcode to use, we can validate this in do_execveat_common() and
>> fail for this scenario, effectively blocking successful exploitation
>> of CVE-2021-4034 and similar bugs which depend on execve(2) working
>> with argc < 1.
>>
>> We use -EINVAL for this case, mirroring recent changes to FreeBSD and
>> OpenBSD. -EINVAL is also used by QNX for this, while Solaris uses
>> -EFAULT.
>>
>> In earlier versions of the patch, it was proposed that we create a
>> fake argv for applications to use when argc < 1, but it was concluded
>> that it would be better to just fail the execve(2) in these cases, as
>> launching a process with an empty or NULL argv[0] was likely to just
>> cause more problems.
>
> Let's do it and see what breaks. :)
>
> I do see at least tools/testing/selftests/exec/recursion-depth.c will
> need a fix. And maybe testcases/kernel/syscalls/execveat/execveat.h
> in LTP.
>
> Acked-by: Kees Cook <[email protected]>
Yes since this only appears to be tests that will break.
Acked-by: "Eric W. Biederman" <[email protected]>
Especially since you are signing up to help fix the tests.
>>
>> Interestingly, Michael Kerrisk opened an issue about this in 2008[1],
>> but there was no consensus to support fixing this issue then.
>> Hopefully now that CVE-2021-4034 shows practical exploitative use[2]
>> of this bug in a shellcode, we can reconsider.
>>
>> This issue is being tracked in the KSPP issue tracker[3].
>>
>> There are a few[4][5] minor edge cases (primarily in test suites) that
>> are caught by this, but we plan to work with the projects to fix those
>> edge cases.
>>
>> [0]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=8408
>> [2]: https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt
>> [3]: https://github.com/KSPP/linux/issues/176
>> [4]: https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0
>> [5]: https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0
>>
>> Changes from v2:
>> - Switch to using -EINVAL as the error code for this.
>> - Use pr_warn_once() to warn when an execve(2) is rejected due to NULL
>> argv.
>>
>> Changes from v1:
>> - Rework commit message significantly.
>> - Make the argv[0] check explicit rather than hijacking the error-check
>> for count().
>>
>> Reported-by: Michael Kerrisk <[email protected]>
>> To: Andrew Morton <[email protected]>
>> Cc: Matthew Wilcox <[email protected]>
>> Cc: Christian Brauner <[email protected]>
>> Cc: Rich Felker <[email protected]>
>> Cc: Eric Biederman <[email protected]>
>> Cc: Alexander Viro <[email protected]>
>> Cc: Kees Cook <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Ariadne Conill <[email protected]>
>> ---
>> fs/exec.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 79f2c9483302..982730cfe3b8 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1897,6 +1897,10 @@ static int do_execveat_common(int fd, struct filename *filename,
>> }
>>
>> retval = count(argv, MAX_ARG_STRINGS);
>> + if (retval == 0) {
>> + pr_warn_once("Attempted to run process '%s' with NULL argv\n", bprm->filename);
>> + retval = -EINVAL;
>> + }
>> if (retval < 0)
>> goto out_free;
>> bprm->argc = retval;
>> --
>> 2.34.1
>>
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 80bd5afdd8568e41fc3a75c695bb179e0d9eee4d ("[PATCH v3] fs/exec: require argv[0] presence in do_execveat_common()")
url: https://github.com/0day-ci/linux/commits/Ariadne-Conill/fs-exec-require-argv-0-presence-in-do_execveat_common/20220127-080829
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
patch link: https://lore.kernel.org/lkml/[email protected]
in testcase: xfstests
version: xfstests-x86_64-972d710-1_20220127
with following parameters:
disk: 4HDD
fs: f2fs
test: generic-group-31
ucode: 0xe2
test-description: xfstests is a regression test suite for xfs and other files ystems.
test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
user :warn : [ 208.077271] run fstests generic/633 at 2022-01-30 04:50:49
kern :warn : [ 208.529090] Attempted to run process '/dev/fd/5/file1' with NULL argv
user :notice: [ 208.806756] generic/633 [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/633.out.bad)
user :notice: [ 208.826454] --- tests/generic/633.out 2022-01-27 11:54:16.000000000 +0000
user :notice: [ 208.842458] +++ /lkp/benchmarks/xfstests/results//generic/633.out.bad 2022-01-30 04:50:49.769538285 +0000
user :notice: [ 208.859622] @@ -1,2 +1,4 @@
user :warn : [ 208.860623] run fstests generic/634 at 2022-01-30 04:50:49
user :notice: [ 208.866037] QA output created by 633
user :notice: [ 208.889262] Silence is golden
user :notice: [ 208.901240] +idmapped-mounts.c: 3608: setid_binaries - Invalid argument - failure: sys_execveat
user :notice: [ 208.918907] +idmapped-mounts.c: 13953: run_test - Success - failure: setid binaries on regular mounts
user :notice: [ 208.935261] ...
user :notice: [ 208.949376] (Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/633.out /lkp/benchmarks/xfstests/results//generic/633.out.bad' to see the entire diff)
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang
On Mon, Jan 31, 2022 at 10:43:52PM +0800, kernel test robot wrote:
>
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 80bd5afdd8568e41fc3a75c695bb179e0d9eee4d ("[PATCH v3] fs/exec: require argv[0] presence in do_execveat_common()")
> url: https://github.com/0day-ci/linux/commits/Ariadne-Conill/fs-exec-require-argv-0-presence-in-do_execveat_common/20220127-080829
> base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
> patch link: https://lore.kernel.org/lkml/[email protected]
>
> in testcase: xfstests
> version: xfstests-x86_64-972d710-1_20220127
> with following parameters:
>
> disk: 4HDD
> fs: f2fs
> test: generic-group-31
> ucode: 0xe2
>
> test-description: xfstests is a regression test suite for xfs and other files ystems.
> test-url: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
>
>
> on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory
>
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
>
>
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot <[email protected]>
>
>
>
> user :warn : [ 208.077271] run fstests generic/633 at 2022-01-30 04:50:49
> kern :warn : [ 208.529090] Attempted to run process '/dev/fd/5/file1' with NULL argv
> user :notice: [ 208.806756] generic/633 [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/633.out.bad)
>
> user :notice: [ 208.826454] --- tests/generic/633.out 2022-01-27 11:54:16.000000000 +0000
>
> user :notice: [ 208.842458] +++ /lkp/benchmarks/xfstests/results//generic/633.out.bad 2022-01-30 04:50:49.769538285 +0000
>
> user :notice: [ 208.859622] @@ -1,2 +1,4 @@
>
> user :warn : [ 208.860623] run fstests generic/634 at 2022-01-30 04:50:49
> user :notice: [ 208.866037] QA output created by 633
>
> user :notice: [ 208.889262] Silence is golden
>
> user :notice: [ 208.901240] +idmapped-mounts.c: 3608: setid_binaries - Invalid argument - failure: sys_execveat
This is from the generic part of the vfs testsuite.
It verifies that set*id binaries are executed with the right e{g,u}id.
Part of that test calls execveat() as:
static char *argv[] = {
NULL,
};
static char *envp[] = {
"EXPECTED_EUID=5000",
"EXPECTED_EGID=5000",
NULL,
};
syscall(__NR_execveat, fd, some_path, argv, envp, 0);
I can fix this rather simply in our upstream fstests with:
static char *argv[] = {
"",
};
I guess.
But doesn't
static char *argv[] = {
NULL,
};
seem something that should work especially with execveat()?
Christian
On Mon, Jan 31, 2022 at 04:37:07PM +0100, Christian Brauner wrote:
> On Mon, Jan 31, 2022 at 03:19:22PM +0000, Matthew Wilcox wrote:
> > On Mon, Jan 31, 2022 at 04:08:19PM +0100, Christian Brauner wrote:
> > > On Mon, Jan 31, 2022 at 10:43:52PM +0800, kernel test robot wrote:
> > > I can fix this rather simply in our upstream fstests with:
> > >
> > > static char *argv[] = {
> > > "",
> > > };
> > >
> > > I guess.
> > >
> > > But doesn't
> > >
> > > static char *argv[] = {
> > > NULL,
> > > };
> > >
> > > seem something that should work especially with execveat()?
> >
> > The problem is that the exec'ed program sees an argc of 0, which is the
> > problem we're trying to work around in the kernel (instead of leaving
> > it to ld.so to fix for suid programs).
>
> Ok, just seems a bit more intuitive for path-based exec than for
> fd-based execveat().
>
> What's argv[0] supposed to contain in these cases?
>
> 1. execveat(fd, NULL, ..., AT_EMPTY_PATH)
> 2. execveat(fd, "my-file", ..., )
>
> "" in both 1. and 2.?
> "" in 1. and "my-file" in 2.?
You didn't specify argv for either of those, so I have no idea.
Programs shouldn't be assuming anything about argv[0]; it's purely
advisory. Unfortunately, some of them do. And some of them are suid.
On Mon, Jan 31, 2022 at 04:08:19PM +0100, Christian Brauner wrote:
> On Mon, Jan 31, 2022 at 10:43:52PM +0800, kernel test robot wrote:
> I can fix this rather simply in our upstream fstests with:
>
> static char *argv[] = {
> "",
> };
>
> I guess.
>
> But doesn't
>
> static char *argv[] = {
> NULL,
> };
>
> seem something that should work especially with execveat()?
The problem is that the exec'ed program sees an argc of 0, which is the
problem we're trying to work around in the kernel (instead of leaving
it to ld.so to fix for suid programs).
On Mon, Jan 31, 2022 at 03:19:22PM +0000, Matthew Wilcox wrote:
> On Mon, Jan 31, 2022 at 04:08:19PM +0100, Christian Brauner wrote:
> > On Mon, Jan 31, 2022 at 10:43:52PM +0800, kernel test robot wrote:
> > I can fix this rather simply in our upstream fstests with:
> >
> > static char *argv[] = {
> > "",
> > };
> >
> > I guess.
> >
> > But doesn't
> >
> > static char *argv[] = {
> > NULL,
> > };
> >
> > seem something that should work especially with execveat()?
>
> The problem is that the exec'ed program sees an argc of 0, which is the
> problem we're trying to work around in the kernel (instead of leaving
> it to ld.so to fix for suid programs).
Ok, just seems a bit more intuitive for path-based exec than for
fd-based execveat().
What's argv[0] supposed to contain in these cases?
1. execveat(fd, NULL, ..., AT_EMPTY_PATH)
2. execveat(fd, "my-file", ..., )
"" in both 1. and 2.?
"" in 1. and "my-file" in 2.?
On Mon, Jan 31, 2022 at 03:51:21PM +0000, Matthew Wilcox wrote:
> On Mon, Jan 31, 2022 at 04:37:07PM +0100, Christian Brauner wrote:
> > On Mon, Jan 31, 2022 at 03:19:22PM +0000, Matthew Wilcox wrote:
> > > On Mon, Jan 31, 2022 at 04:08:19PM +0100, Christian Brauner wrote:
> > > > On Mon, Jan 31, 2022 at 10:43:52PM +0800, kernel test robot wrote:
> > > > I can fix this rather simply in our upstream fstests with:
> > > >
> > > > static char *argv[] = {
> > > > "",
> > > > };
> > > >
> > > > I guess.
> > > >
> > > > But doesn't
> > > >
> > > > static char *argv[] = {
> > > > NULL,
> > > > };
> > > >
> > > > seem something that should work especially with execveat()?
> > >
> > > The problem is that the exec'ed program sees an argc of 0, which is the
> > > problem we're trying to work around in the kernel (instead of leaving
> > > it to ld.so to fix for suid programs).
> >
> > Ok, just seems a bit more intuitive for path-based exec than for
> > fd-based execveat().
> >
> > What's argv[0] supposed to contain in these cases?
> >
> > 1. execveat(fd, NULL, ..., AT_EMPTY_PATH)
> > 2. execveat(fd, "my-file", ..., )
> >
> > "" in both 1. and 2.?
> > "" in 1. and "my-file" in 2.?
>
> You didn't specify argv for either of those, so I have no idea.
> Programs shouldn't be assuming anything about argv[0]; it's purely
> advisory. Unfortunately, some of them do. And some of them are suid.
Yes, programs shouldn't assume anything about argv[0]. But a lot of
programs are used to setting argv[0] to the name of the executed binary.
The exec* manpages examples do this. Just looking at a random selftest, e.g.
bpf/prog_tests/test_lsm.c
where we find:
char *CMD_ARGS[] = {"true", NULL};
execvp(CMD_ARGS[0], CMD_ARGS);
I'm just wondering how common this is for execveat() because it is not
as clear what the actual name of the binary is in these two examples
1.
fd = open("/bin/true", );
char *CMD_ARGS[] = {"", NULL};
execveat(fd, NULL, ..., AT_EMPTY_PATH)
2.
fd = open("/bin", );
char *CMD_ARGS[] = {"true", NULL};
execveat(fd, CMD_ARGS[0], CMD_ARGS 0)
in other words, the changes that you see CMD_ARGS[0] == NULL for
execveat() seem higher than for path-based exec.
To counter that we should probably at least update the execveat()
manpage with a recommendation what CMD_ARGS[0] should be set to if it
isn't allowed to be set to NULL anymore. This is why was asking what
argv[0] is supposed to be if the binary doesn't take any arguments.
On Mon, Jan 31, 2022 at 05:14:15PM +0100, Christian Brauner wrote:
> On Mon, Jan 31, 2022 at 03:51:21PM +0000, Matthew Wilcox wrote:
> > On Mon, Jan 31, 2022 at 04:37:07PM +0100, Christian Brauner wrote:
> > > On Mon, Jan 31, 2022 at 03:19:22PM +0000, Matthew Wilcox wrote:
> > > > On Mon, Jan 31, 2022 at 04:08:19PM +0100, Christian Brauner wrote:
> > > > > On Mon, Jan 31, 2022 at 10:43:52PM +0800, kernel test robot wrote:
> > > > > I can fix this rather simply in our upstream fstests with:
> > > > >
> > > > > static char *argv[] = {
> > > > > "",
> > > > > };
> > > > >
> > > > > I guess.
> > > > >
> > > > > But doesn't
> > > > >
> > > > > static char *argv[] = {
> > > > > NULL,
> > > > > };
> > > > >
> > > > > seem something that should work especially with execveat()?
> > > >
> > > > The problem is that the exec'ed program sees an argc of 0, which is the
> > > > problem we're trying to work around in the kernel (instead of leaving
> > > > it to ld.so to fix for suid programs).
> > >
> > > Ok, just seems a bit more intuitive for path-based exec than for
> > > fd-based execveat().
> > >
> > > What's argv[0] supposed to contain in these cases?
> > >
> > > 1. execveat(fd, NULL, ..., AT_EMPTY_PATH)
> > > 2. execveat(fd, "my-file", ..., )
> > >
> > > "" in both 1. and 2.?
> > > "" in 1. and "my-file" in 2.?
> >
> > You didn't specify argv for either of those, so I have no idea.
> > Programs shouldn't be assuming anything about argv[0]; it's purely
> > advisory. Unfortunately, some of them do. And some of them are suid.
>
> Yes, programs shouldn't assume anything about argv[0]. But a lot of
> programs are used to setting argv[0] to the name of the executed binary.
> The exec* manpages examples do this. Just looking at a random selftest, e.g.
>
> bpf/prog_tests/test_lsm.c
>
> where we find:
>
> char *CMD_ARGS[] = {"true", NULL};
> execvp(CMD_ARGS[0], CMD_ARGS);
>
> I'm just wondering how common this is for execveat() because it is not
> as clear what the actual name of the binary is in these two examples
>
> 1.
> fd = open("/bin/true", );
> char *CMD_ARGS[] = {"", NULL};
> execveat(fd, NULL, ..., AT_EMPTY_PATH)
>
> 2.
> fd = open("/bin", );
> char *CMD_ARGS[] = {"true", NULL};
> execveat(fd, CMD_ARGS[0], CMD_ARGS 0)
>
> in other words, the changes that you see CMD_ARGS[0] == NULL for
> execveat() seem higher than for path-based exec.
>
> To counter that we should probably at least update the execveat()
> manpage with a recommendation what CMD_ARGS[0] should be set to if it
> isn't allowed to be set to NULL anymore. This is why was asking what
> argv[0] is supposed to be if the binary doesn't take any arguments.
Sent a fix to our fstests now replacing the argv[0] as NULL with "".
On Mon, Jan 31, 2022 at 01:59:40PM -0800, Andrew Morton wrote:
> On Mon, 31 Jan 2022 18:13:44 +0100 Christian Brauner <[email protected]> wrote:
>
> > > in other words, the changes that you see CMD_ARGS[0] == NULL for
> > > execveat() seem higher than for path-based exec.
> > >
> > > To counter that we should probably at least update the execveat()
> > > manpage with a recommendation what CMD_ARGS[0] should be set to if it
> > > isn't allowed to be set to NULL anymore. This is why was asking what
> > > argv[0] is supposed to be if the binary doesn't take any arguments.
> >
> > Sent a fix to our fstests now replacing the argv[0] as NULL with "".
>
> As we hit this check so quickly, I'm thinking that Ariadne's patch
> "fs/exec: require argv[0] presence in do_execveat_common()" (which
> added the check) isn't something we'll be able to merge into mainline?
I think the next best would be to mutate an NULL argv into { "", NULL }.
However, I still think we should do the pr_warn().
Thoughts?
--
Kees Cook
On Mon, 31 Jan 2022 18:13:44 +0100 Christian Brauner <[email protected]> wrote:
> > in other words, the changes that you see CMD_ARGS[0] == NULL for
> > execveat() seem higher than for path-based exec.
> >
> > To counter that we should probably at least update the execveat()
> > manpage with a recommendation what CMD_ARGS[0] should be set to if it
> > isn't allowed to be set to NULL anymore. This is why was asking what
> > argv[0] is supposed to be if the binary doesn't take any arguments.
>
> Sent a fix to our fstests now replacing the argv[0] as NULL with "".
As we hit this check so quickly, I'm thinking that Ariadne's patch
"fs/exec: require argv[0] presence in do_execveat_common()" (which
added the check) isn't something we'll be able to merge into mainline?
On Mon, Jan 31, 2022 at 01:59:40PM -0800, Andrew Morton wrote:
> On Mon, 31 Jan 2022 18:13:44 +0100 Christian Brauner <[email protected]> wrote:
>
> > > in other words, the changes that you see CMD_ARGS[0] == NULL for
> > > execveat() seem higher than for path-based exec.
> > >
> > > To counter that we should probably at least update the execveat()
> > > manpage with a recommendation what CMD_ARGS[0] should be set to if it
> > > isn't allowed to be set to NULL anymore. This is why was asking what
> > > argv[0] is supposed to be if the binary doesn't take any arguments.
> >
> > Sent a fix to our fstests now replacing the argv[0] as NULL with "".
>
> As we hit this check so quickly, I'm thinking that Ariadne's patch
> "fs/exec: require argv[0] presence in do_execveat_common()" (which
> added the check) isn't something we'll be able to merge into mainline?
Not in the originally envisioned form. But I think Kees patch to make a
argv[0] the empty string when it's NULL has a better chance of
surviving.
On Mon, Jan 31, 2022 at 02:49:36PM -0800, Kees Cook wrote:
> On Mon, Jan 31, 2022 at 01:59:40PM -0800, Andrew Morton wrote:
> > On Mon, 31 Jan 2022 18:13:44 +0100 Christian Brauner <[email protected]> wrote:
> >
> > > > in other words, the changes that you see CMD_ARGS[0] == NULL for
> > > > execveat() seem higher than for path-based exec.
> > > >
> > > > To counter that we should probably at least update the execveat()
> > > > manpage with a recommendation what CMD_ARGS[0] should be set to if it
> > > > isn't allowed to be set to NULL anymore. This is why was asking what
> > > > argv[0] is supposed to be if the binary doesn't take any arguments.
> > >
> > > Sent a fix to our fstests now replacing the argv[0] as NULL with "".
> >
> > As we hit this check so quickly, I'm thinking that Ariadne's patch
> > "fs/exec: require argv[0] presence in do_execveat_common()" (which
> > added the check) isn't something we'll be able to merge into mainline?
>
> I think the next best would be to mutate an NULL argv into { "", NULL }.
> However, I still think we should do the pr_warn().
>
> Thoughts?
+1