Re: [LTP] [PATCH] syscalls/execve06: Add test for argv[0] = NULL

[Petr Vorel <pvorel@suse.cz>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp