| Message ID | 20220613125153.20423-1-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | syscalls/execve06: Add test for argv[0] = NULL | expand |
HI Cyril, > Adds a test that kernel sets argv[0] to a dummy empty string if NULL was > passed to the execve() syscall. This was introduced in commit: > commit dcd46d897adb70d63e025f175a00a89797d31a43 > Author: Kees Cook <keescook@chromium.org> > Date: Mon Jan 31 16:09:47 2022 -0800 > exec: Force single empty string when argv is empty > in order to fix all potential CVEs where userspace programs attempt to > blindly process the argv[] list starting at argv[1]. There was at least > one example of this caught in the wild CVE-2021-4034 in polkit but there > are likely more. Great, thanks for addressing this. > Fixes: #911 > testcases/kernel/syscalls/execve/.gitignore | 2 + > testcases/kernel/syscalls/execve/execve06.c | 49 +++++++++++++++++++ > .../kernel/syscalls/execve/execve06_child.c | 27 ++++++++++ > 3 files changed, 78 insertions(+) > create mode 100644 testcases/kernel/syscalls/execve/execve06.c > create mode 100644 testcases/kernel/syscalls/execve/execve06_child.c This should go to runtest/syscalls and runtest/cve, right? (can be fixed before merge). Kind regards, Petr
Hi Cyril, ... > in order to fix all potential CVEs where userspace programs attempt to > blindly process the argv[] list starting at argv[1]. There was at least > one example of this caught in the wild CVE-2021-4034 in polkit but there > are likely more. Thanks for implementing this! > Fixes: #911 ... > diff --git a/testcases/kernel/syscalls/execve/execve06.c b/testcases/kernel/syscalls/execve/execve06.c > new file mode 100644 > index 000000000..b3280cf76 > --- /dev/null > +++ b/testcases/kernel/syscalls/execve/execve06.c > @@ -0,0 +1,49 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2022 Cyril Hrubis <chrubis@suse.cz> > + */ > + > +/*\ > + * [Description] > + * > + * Test that kernel adds dummy argv[0] if empty argument list was passed to > + * execve(). This fixes at least one CVE where userspace programs start to > + * process argument list blindly from argv[1] such as polkit pkexec > + * CVE-2021-4034. We might also add link to LWM article :) https://lwn.net/Articles/883547/ > + */ > + > +#include <stdlib.h> > +#include <stdio.h> > +#include "tst_test.h" > + > +static void verify_execve(void) > +{ > + pid_t pid; > + char path[512]; > + char ipc_env_var[1024]; > + > + sprintf(ipc_env_var, IPC_ENV_VAR "=%s", getenv(IPC_ENV_VAR)); > + > + char *const envp[] = {ipc_env_var, NULL}; > + char *const argv[] = {NULL}; > + > + if (tst_get_path("execve06_child", path, sizeof(path))) > + tst_brk(TCONF, "Couldn't find execve06_child in $PATH"); I wonder why this does not work: .needs_cmds = (const char *[]) { "execve06_child", NULL }, tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s execve06.c:33: TFAIL: Failed to execute execl01_child: ENOENT (2) tst_test.c:395: TBROK: Invalid child (23451) exit value 1 > + > + pid = SAFE_FORK(); > + if (pid == 0) { > + execve(path, argv, envp); > + tst_brk(TFAIL | TERRNO, "Failed to execute execl01_child"); > + } > +} > + > +static struct tst_test test = { > + .forks_child = 1, > + .child_needs_reinit = 1, > + .test_all = verify_execve, > + .tags = (const struct tst_tag[]) { > + {"linux-git", "dcd46d897adb"}, > + {"CVE", "2021-4034"}, > + {} > + } > +}; > diff --git a/testcases/kernel/syscalls/execve/execve06_child.c b/testcases/kernel/syscalls/execve/execve06_child.c > new file mode 100644 > index 000000000..17280d58a > --- /dev/null > +++ b/testcases/kernel/syscalls/execve/execve06_child.c > @@ -0,0 +1,27 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2022 Cyril Hrubis chrubis@suse.cz very nit: <chrubis@suse.cz> (missing < >) > + */ > + > +#define TST_NO_DEFAULT_MAIN > +#include <stdlib.h> > +#include "tst_test.h" > + > +int main(int argc, char *argv[]) > +{ > + tst_reinit(); > + > + if (argc != 1) { > + tst_res(TFAIL, "argc is %d, expected 1", argc); > + return 0; > + } > + > + if (!argv[0]) { > + tst_res(TFAIL, "argv[0] == NULL"); > + return 0; > + } > + > + tst_res(TPASS, "argv[0] was filled in by kernel"); Testing matches the description from kernel commit. Maybe also test for argv[0] being empty string (to make sure behavior does not change, although unlikely it'd change)? I tested it on various kernels, works as expected. Reviewed-by: Petr Vorel <pvorel@suse.cz> Tested-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
Hi! > > in order to fix all potential CVEs where userspace programs attempt to > > blindly process the argv[] list starting at argv[1]. There was at least > > one example of this caught in the wild CVE-2021-4034 in polkit but there > > are likely more. > > Great, thanks for addressing this. > > > Fixes: #911 > > > testcases/kernel/syscalls/execve/.gitignore | 2 + > > testcases/kernel/syscalls/execve/execve06.c | 49 +++++++++++++++++++ > > .../kernel/syscalls/execve/execve06_child.c | 27 ++++++++++ > > 3 files changed, 78 insertions(+) > > create mode 100644 testcases/kernel/syscalls/execve/execve06.c > > create mode 100644 testcases/kernel/syscalls/execve/execve06_child.c > > This should go to runtest/syscalls and runtest/cve, right? > (can be fixed before merge). Sigh, sorry, I did forget about these.
Hi! > I wonder why this does not work: > .needs_cmds = (const char *[]) { > "execve06_child", > NULL > }, > > tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s > execve06.c:33: TFAIL: Failed to execute execl01_child: ENOENT (2) > tst_test.c:395: TBROK: Invalid child (23451) exit value 1 The difference is because the test library adds current directory and test start directory into $PATH before a test is executed, see add_paths() in the tst_test.c. The .needs_cmds is for binaries installed on the system in /usr/bin/ etc. so it does not make much sense to call the add_paths() before we process the array. > > + > > + pid = SAFE_FORK(); > > + if (pid == 0) { > > + execve(path, argv, envp); > > + tst_brk(TFAIL | TERRNO, "Failed to execute execl01_child"); > > + } > > +} > > + > > +static struct tst_test test = { > > + .forks_child = 1, > > + .child_needs_reinit = 1, > > + .test_all = verify_execve, > > + .tags = (const struct tst_tag[]) { > > + {"linux-git", "dcd46d897adb"}, > > + {"CVE", "2021-4034"}, > > + {} > > + } > > +}; > > diff --git a/testcases/kernel/syscalls/execve/execve06_child.c b/testcases/kernel/syscalls/execve/execve06_child.c > > new file mode 100644 > > index 000000000..17280d58a > > --- /dev/null > > +++ b/testcases/kernel/syscalls/execve/execve06_child.c > > @@ -0,0 +1,27 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (C) 2022 Cyril Hrubis chrubis@suse.cz > very nit: <chrubis@suse.cz> (missing < >) Will fix. > > + */ > > + > > +#define TST_NO_DEFAULT_MAIN > > +#include <stdlib.h> > > +#include "tst_test.h" > > + > > +int main(int argc, char *argv[]) > > +{ > > + tst_reinit(); > > + > > + if (argc != 1) { > > + tst_res(TFAIL, "argc is %d, expected 1", argc); > > + return 0; > > + } > > + > > + if (!argv[0]) { > > + tst_res(TFAIL, "argv[0] == NULL"); > > + return 0; > > + } > > + > > + tst_res(TPASS, "argv[0] was filled in by kernel"); > > Testing matches the description from kernel commit. > Maybe also test for argv[0] being empty string (to make sure behavior does not > change, although unlikely it'd change)? It should be safe either way but I decided to be less specific and only check that something have been added there. > I tested it on various kernels, works as expected. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Tested-by: Petr Vorel <pvorel@suse.cz> I will push it with the added runtest entries and fixed copyright then.
Hi Cyril, > > I wonder why this does not work: > > .needs_cmds = (const char *[]) { > > "execve06_child", > > NULL > > }, > > tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s > > execve06.c:33: TFAIL: Failed to execute execl01_child: ENOENT (2) > > tst_test.c:395: TBROK: Invalid child (23451) exit value 1 > The difference is because the test library adds current directory and > test start directory into $PATH before a test is executed, see > add_paths() in the tst_test.c. > The .needs_cmds is for binaries installed on the system in /usr/bin/ > etc. so it does not make much sense to call the add_paths() before we > process the array. Thx! ... > > Testing matches the description from kernel commit. > > Maybe also test for argv[0] being empty string (to make sure behavior does not > > change, although unlikely it'd change)? > It should be safe either way but I decided to be less specific and only > check that something have been added there. Sure. > > I tested it on various kernels, works as expected. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Tested-by: Petr Vorel <pvorel@suse.cz> > I will push it with the added runtest entries and fixed copyright then. Thx! Kind regards, Petr
diff --git a/testcases/kernel/syscalls/execve/.gitignore b/testcases/kernel/syscalls/execve/.gitignore index 50cabbb83..fee81faf7 100644 --- a/testcases/kernel/syscalls/execve/.gitignore +++ b/testcases/kernel/syscalls/execve/.gitignore @@ -4,4 +4,6 @@ /execve03 /execve04 /execve05 +/execve06 +/execve06_child /execve_child diff --git a/testcases/kernel/syscalls/execve/execve06.c b/testcases/kernel/syscalls/execve/execve06.c new file mode 100644 index 000000000..b3280cf76 --- /dev/null +++ b/testcases/kernel/syscalls/execve/execve06.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 Cyril Hrubis <chrubis@suse.cz> + */ + +/*\ + * [Description] + * + * Test that kernel adds dummy argv[0] if empty argument list was passed to + * execve(). This fixes at least one CVE where userspace programs start to + * process argument list blindly from argv[1] such as polkit pkexec + * CVE-2021-4034. + */ + +#include <stdlib.h> +#include <stdio.h> +#include "tst_test.h" + +static void verify_execve(void) +{ + pid_t pid; + char path[512]; + char ipc_env_var[1024]; + + sprintf(ipc_env_var, IPC_ENV_VAR "=%s", getenv(IPC_ENV_VAR)); + + char *const envp[] = {ipc_env_var, NULL}; + char *const argv[] = {NULL}; + + if (tst_get_path("execve06_child", path, sizeof(path))) + tst_brk(TCONF, "Couldn't find execve06_child in $PATH"); + + pid = SAFE_FORK(); + if (pid == 0) { + execve(path, argv, envp); + tst_brk(TFAIL | TERRNO, "Failed to execute execl01_child"); + } +} + +static struct tst_test test = { + .forks_child = 1, + .child_needs_reinit = 1, + .test_all = verify_execve, + .tags = (const struct tst_tag[]) { + {"linux-git", "dcd46d897adb"}, + {"CVE", "2021-4034"}, + {} + } +}; diff --git a/testcases/kernel/syscalls/execve/execve06_child.c b/testcases/kernel/syscalls/execve/execve06_child.c new file mode 100644 index 000000000..17280d58a --- /dev/null +++ b/testcases/kernel/syscalls/execve/execve06_child.c @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2022 Cyril Hrubis chrubis@suse.cz + */ + +#define TST_NO_DEFAULT_MAIN +#include <stdlib.h> +#include "tst_test.h" + +int main(int argc, char *argv[]) +{ + tst_reinit(); + + if (argc != 1) { + tst_res(TFAIL, "argc is %d, expected 1", argc); + return 0; + } + + if (!argv[0]) { + tst_res(TFAIL, "argv[0] == NULL"); + return 0; + } + + tst_res(TPASS, "argv[0] was filled in by kernel"); + + return 0; +}
Adds a test that kernel sets argv[0] to a dummy empty string if NULL was passed to the execve() syscall. This was introduced in commit: commit dcd46d897adb70d63e025f175a00a89797d31a43 Author: Kees Cook <keescook@chromium.org> Date: Mon Jan 31 16:09:47 2022 -0800 exec: Force single empty string when argv is empty in order to fix all potential CVEs where userspace programs attempt to blindly process the argv[] list starting at argv[1]. There was at least one example of this caught in the wild CVE-2021-4034 in polkit but there are likely more. Fixes: #911 Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- testcases/kernel/syscalls/execve/.gitignore | 2 + testcases/kernel/syscalls/execve/execve06.c | 49 +++++++++++++++++++ .../kernel/syscalls/execve/execve06_child.c | 27 ++++++++++ 3 files changed, 78 insertions(+) create mode 100644 testcases/kernel/syscalls/execve/execve06.c create mode 100644 testcases/kernel/syscalls/execve/execve06_child.c