execve: disallow argc == 0

[kevans]

Summary

Two commits:

execve: disallow argc == 0

The manpage already contains the following verbiage:

"At least one argument must be present in the array"

Carry through and document it the rest of the way.  Allowing argc == 0
has been a source of security issues in the past, and it's hard to
imagine a valid use-case for allowing it.  Toss back EINVAL if we ended
up not copying in any args for *execve().

The manpage change can be considered "Obtained from: OpenBSD"

tests: add a basic test for argc == 0

The kernel should reject such exec()s now, early on. Instead of adding
the needed boilerplate to write a test in C, just add an -n argument for
"(n)ull argv" to the execve helper and exec this other helper that just
exits silently with argv count.

[kib]

You did not handled compat32 at least.
What about linux?

[jrtc27]

sys/kern/kern_exec.c

1316

freebsd32 has its own freebsd32_exec_copyin_args. Maybe better to punt this to kern_execve?

[kevans]

sys/kern/kern_exec.c

1316

Yeah, or top of do_execve...

[kevans]

Take two: other ABIs exist

[emaste]

OK, a couple of bikesheddy comments if you like

sys/kern/kern_exec.c

363

I don't see what you mean by "probably not a big deal right now."

tests/sys/kern/execve/execve_helper.c

51–53 ↗

(On Diff #101939)

this seems a bit convoluted, what about just making it

if (argc == 2)
        ....
else if (argc == 3 && strcmp...)
        ....
else {
        fprintf(..., "usage

tests/sys/kern/execve/execve_test.sh

110 ↗

(On Diff #101939)

ah, so we either get exit 1 because execve_argc_helper exited with argc == 1 ...

116 ↗

(On Diff #101939)

... or because execve_helper failed to exec it

OK I guess, would it be clearer if execve_argc_helper just did printf("%d", argc)?

[kevans]

sys/kern/kern_exec.c

363

Yeah, I'll drop that. That was an addition from the thought that right now one can surmise from an EINVAL failure that argc == 0 if the manpage is to be believed, but a future change could return EINVAL for some other scenario we aren't thinking of. Either way, this comment could likely be reduced again to just "Must have at least one argument." because auditing the args should generally happen even if they're invalid anyways.

[kevans]

sys/kern/kern_exec.c

367

Sigh, this leaks; added exec_free_args(args); locally

[adridg]

#include <unistd.h>

int main(int argc, char **argv)
{
    char * const bogus_argv[] = {NULL, NULL, NULL, NULL};
    char * const bogus_envp[] = {"PS1=$", NULL};
    int r = execve("/bin/ls", bogus_argv, bogus_envp);
    return 0;
}

What should this do? It's quite surprising to me, suggesting there's another code path that doesn't handle ugly-argvs very well. Presumably this code will, with the patch in this review, error out because of the NULL value for argv[0], but it seems worth investigating.

[kib]

In D34045#769797, @adridg wrote:

#include <unistd.h>

int main(int argc, char **argv)
{
    char * const bogus_argv[] = {NULL, NULL, NULL, NULL};
    char * const bogus_envp[] = {"PS1=$", NULL};
    int r = execve("/bin/ls", bogus_argv, bogus_envp);
    return 0;
}

What should this do? It's quite surprising to me, suggesting there's another code path that doesn't handle ugly-argvs very well. Presumably this code will, with the patch in this review, error out because of the NULL value for argv[0], but it seems worth investigating.

Well it is quite clear what happens there, and it is quite explicitly written out in the CVE analysis, I believe. I wrote a standalone tool to dump argc/argc/env, without relying on crt1/libc to fetch and parse the strings passed by kernel. You may find it there https://github.com/kostikbel/argvdump . Also added noargs to trigger your case, so that you can see how kernel passes the args.

[val_packett.cool]

I wonder if https://github.com/freebsd/freebsd-src/blob/e5b431fc0c20771a2a18cb5169d75cf337d0f1dd/sys/kern/imgact_shell.c#L206-L214 can be removed now. (Not saying it *should*)