Page MenuHomeFreeBSD

execve: disallow argc == 0
ClosedPublic

Authored by kevans on Jan 26 2022, 1:29 AM.

Details

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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

jrtc27 added inline comments.
sys/kern/kern_exec.c
1315

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

sys/kern/kern_exec.c
1315

Yeah, or top of do_execve...

Take two: other ABIs exist

This revision is now accepted and ready to land.Jan 26 2022, 2:22 AM

OK, a couple of bikesheddy comments if you like

sys/kern/kern_exec.c
362

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 added inline comments.
sys/kern/kern_exec.c
362

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.

sys/kern/kern_exec.c
366

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

#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.

#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.

This revision was automatically updated to reflect the committed changes.