From 87ce69246869d9b9d69be278e29e0fc6a3cabdb9 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Tue, 21 Feb 2023 20:01:13 -0700 Subject: [PATCH] Fix potential double free for rules that include a CHROOT= option. If a rule with a CHROOT= option matches the user, host and runas, the user_cmnd variable could be freed twice. --- MANIFEST | 2 ++ plugins/sudoers/match_command.c | 6 +++++- plugins/sudoers/regress/fuzz/fuzz_sudoers.c | 17 ++++++++++++---- .../sudoers/regress/testsudoers/test20.out.ok | 15 ++++++++++++++ plugins/sudoers/regress/testsudoers/test20.sh | 18 +++++++++++++++++ plugins/sudoers/testsudoers.c | 20 ++++++++++++++----- plugins/sudoers/visudo.c | 4 +++- 7 files changed, 71 insertions(+), 11 deletions(-) create mode 100644 plugins/sudoers/regress/testsudoers/test20.out.ok create mode 100644 plugins/sudoers/regress/testsudoers/test20.sh diff --git a/MANIFEST b/MANIFEST index 07ea2b3d5..d30307c4b 100644 --- a/MANIFEST +++ b/MANIFEST @@ -1056,6 +1056,8 @@ plugins/sudoers/regress/testsudoers/test19.sh plugins/sudoers/regress/testsudoers/test2.inc plugins/sudoers/regress/testsudoers/test2.out.ok plugins/sudoers/regress/testsudoers/test2.sh +plugins/sudoers/regress/testsudoers/test20.out.ok +plugins/sudoers/regress/testsudoers/test20.sh plugins/sudoers/regress/testsudoers/test3.out.ok plugins/sudoers/regress/testsudoers/test3.sh plugins/sudoers/regress/testsudoers/test4.out.ok diff --git a/plugins/sudoers/match_command.c b/plugins/sudoers/match_command.c index ff5d0b7f2..768f2fd26 100644 --- a/plugins/sudoers/match_command.c +++ b/plugins/sudoers/match_command.c @@ -825,12 +825,16 @@ command_matches(const char *sudoers_cmnd, const char *sudoers_args, /* Rule-specific runchroot, set user_cmnd and user_stat after pivot. */ int status; + /* Save old user_cmnd first, set_cmnd_path() will free it. */ saved_user_cmnd = user_cmnd; + user_cmnd = NULL; if (user_stat != NULL) saved_user_stat = *user_stat; status = set_cmnd_path(NULL); - if (status != FOUND) + if (status != FOUND) { + user_cmnd = saved_user_cmnd; saved_user_cmnd = NULL; + } if (info != NULL) info->status = status; } diff --git a/plugins/sudoers/regress/fuzz/fuzz_sudoers.c b/plugins/sudoers/regress/fuzz/fuzz_sudoers.c index 5c38a8be2..15dc72a21 100644 --- a/plugins/sudoers/regress/fuzz/fuzz_sudoers.c +++ b/plugins/sudoers/regress/fuzz/fuzz_sudoers.c @@ -45,6 +45,9 @@ static int fuzz_conversation(int num_msgs, const struct sudo_conv_message msgs[] static int fuzz_printf(int msg_type, const char *fmt, ...); int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size); +/* For set_cmnd_path() */ +static const char *orig_cmnd; + /* Required to link with parser. */ struct sudo_user sudo_user; struct passwd *list_pw; @@ -104,8 +107,13 @@ init_envtables(void) int set_cmnd_path(const char *runchroot) { - /* Cannot return FOUND without also setting user_cmnd to a new value. */ - return NOT_FOUND; + /* Reallocate user_cmnd to catch bugs in command_matches(). */ + char *new_cmnd = strdup(orig_cmnd); + if (new_cmnd == NULL) + return NOT_FOUND_ERROR; + free(user_cmnd); + user_cmnd = new_cmnd; + return FOUND; } /* STUB */ @@ -277,11 +285,12 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) /* The minimum needed to perform matching (user_cmnd must be dynamic). */ user_host = user_shost = user_runhost = user_srunhost = (char *)"localhost"; - user_cmnd = strdup("/usr/bin/id"); + orig_cmnd = (char *)"/usr/bin/id"; + user_cmnd = strdup(orig_cmnd); if (user_cmnd == NULL) goto done; user_args = (char *)"-u"; - user_base = (char *)"id"; + user_base = sudo_basename(user_cmnd); /* Add a fake network interfaces. */ interfaces = get_interfaces(); diff --git a/plugins/sudoers/regress/testsudoers/test20.out.ok b/plugins/sudoers/regress/testsudoers/test20.out.ok new file mode 100644 index 000000000..95f4c31fc --- /dev/null +++ b/plugins/sudoers/regress/testsudoers/test20.out.ok @@ -0,0 +1,15 @@ +Parses OK + +Entries for user root: + +ALL = CHROOT=/ /bin/ls + host matched + runas matched + cmnd allowed + +ALL = CWD=/ /bin/pwd + host matched + runas matched + cmnd allowed + +Command allowed diff --git a/plugins/sudoers/regress/testsudoers/test20.sh b/plugins/sudoers/regress/testsudoers/test20.sh new file mode 100644 index 000000000..432517529 --- /dev/null +++ b/plugins/sudoers/regress/testsudoers/test20.sh @@ -0,0 +1,18 @@ +#!/bin/sh +# +# Verify CHROOT and CWD support +# This will catch an unpatched double-free in set_cmnd_path() under ASAN. +# + +: ${TESTSUDOERS=testsudoers} + +exec 2>&1 + +# Exercise double free of user_cmnd in set_cmnd_path() under ASAN. +# We need more than one rule where the last rule matches and has CHROOT. +$TESTSUDOERS root /bin/ls <<'EOF' +root ALL = CWD=/ /bin/pwd +root ALL = CHROOT=/ /bin/ls +EOF + +exit 0 diff --git a/plugins/sudoers/testsudoers.c b/plugins/sudoers/testsudoers.c index b74745500..02d8f5bf3 100644 --- a/plugins/sudoers/testsudoers.c +++ b/plugins/sudoers/testsudoers.c @@ -82,6 +82,7 @@ extern int (*trace_print)(const char *msg); */ struct sudo_user sudo_user; struct passwd *list_pw; +static const char *orig_cmnd; static char *runas_group, *runas_user; #if defined(SUDO_DEVEL) && defined(__OpenBSD__) @@ -203,14 +204,18 @@ main(int argc, char *argv[]) if (!dflag) usage(); user_name = argc ? *argv++ : (char *)"root"; - user_cmnd = user_base = (char *)"true"; + orig_cmnd = "true"; argc = 0; } else { user_name = *argv++; - user_cmnd = *argv++; - user_base = sudo_basename(user_cmnd); + orig_cmnd = *argv++; argc -= 2; } + user_cmnd = strdup(orig_cmnd); + if (user_cmnd == NULL) + sudo_fatalx(U_("%s: %s"), __func__, U_("unable to allocate memory")); + user_base = sudo_basename(user_cmnd); + if ((sudo_user.pw = sudo_getpwnam(user_name)) == NULL) sudo_fatalx(U_("unknown user %s"), user_name); @@ -521,8 +526,13 @@ unpivot_root(int fds[2]) int set_cmnd_path(const char *runchroot) { - /* Cannot return FOUND without also setting user_cmnd to a new value. */ - return NOT_FOUND; + /* Reallocate user_cmnd to catch bugs in command_matches(). */ + char *new_cmnd = strdup(orig_cmnd); + if (new_cmnd == NULL) + return NOT_FOUND_ERROR; + free(user_cmnd); + user_cmnd = new_cmnd; + return FOUND; } static bool diff --git a/plugins/sudoers/visudo.c b/plugins/sudoers/visudo.c index fea18933e..9c9feccc4 100644 --- a/plugins/sudoers/visudo.c +++ b/plugins/sudoers/visudo.c @@ -260,7 +260,9 @@ main(int argc, char *argv[]) } /* Mock up a fake sudo_user struct. */ - user_cmnd = user_base = (char *)""; + user_cmnd = user_base = strdup("true"); + if (user_cmnd == NULL) + sudo_fatalx(U_("%s: %s"), __func__, U_("unable to allocate memory")); if (geteuid() == 0) { const char *user = getenv("SUDO_USER"); if (user != NULL && *user != '\0')