From 4e78f58b538e9a4b5e9cbaf6bbcff48461fc3cc3 Mon Sep 17 00:00:00 2001 From: Eric Seifert Date: Wed, 5 Jul 2023 14:41:44 -0700 Subject: [PATCH] Use execle instead of popen in tacas nss to avoid shell escape exploits (#15284) Why I did it Tacacs nss library uses popen to execute useradd and usermod commands. Popen executes using a shell (/bin/sh) which is passed the command string with "-c". This means that if untrusted user input is supplied, unexpected shell escapes can occur. In this case the username supplied can be untrusted user input when logging in via ssh or other methods when tacacs is enabled. Debian has very little limitation on usernames and as such characters such as quotes, braces, $, >, | etc are all allowed. Since the nss library is run by root, any shell escape will be ran as root. In the current community version of tacacs nss library, the issue is mitigated by the fact that the useradd command is only ran if the user is found to exist on the tacacs server, so the bad username would have to already exists there which is unlikely. However, internally (at Dell) we had to modify this behavior to support other tacacs servers that do not allow authorization messages to verify user existence prior to a successful authentication. These servers include Cisco ISE and Aruba ClearPass. In order to support these tacacs+ servers, we have to create a temporary user immediately, which means this would be a much bigger issue. I also plan to supply the patch to support ISE and ClearPass and as such, I would suggest taking this patch to remediate this issue first. How I did it Replace call to popen with fork/execl of the useradd/usermod binary directly. How to verify it Install patched version of libnss-tacplus and verify that tacacs+ user login still works as expected. --- ...en-shell-execution-with-safer-execle.patch | 108 ++++++++++++++++++ src/tacacs/nss/patch/series | 1 + 2 files changed, 109 insertions(+) create mode 100644 src/tacacs/nss/patch/0011-Replace-popen-shell-execution-with-safer-execle.patch diff --git a/src/tacacs/nss/patch/0011-Replace-popen-shell-execution-with-safer-execle.patch b/src/tacacs/nss/patch/0011-Replace-popen-shell-execution-with-safer-execle.patch new file mode 100644 index 0000000000..dfef93db17 --- /dev/null +++ b/src/tacacs/nss/patch/0011-Replace-popen-shell-execution-with-safer-execle.patch @@ -0,0 +1,108 @@ +From 99ff134e88ee698623ac5c09cbcd0e88e0fdfa40 Mon Sep 17 00:00:00 2001 +From: Eric Seifert +Date: Mon, 26 Jun 2023 09:18:44 -0700 +Subject: [PATCH] Replace popen shell execution with safer execle + +--- + nss_tacplus.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------- + 1 file changed, 52 insertions(+), 11 deletions(-) + +diff --git a/nss_tacplus.c b/nss_tacplus.c +index cd73870..7574374 100644 +--- a/nss_tacplus.c ++++ b/nss_tacplus.c +@@ -34,6 +34,8 @@ + #include + #include + #include ++#include ++#include + + #include + +@@ -439,6 +441,39 @@ static int delete_conf_line(const char *name) + return 0; + } + ++int user_mod_add(const char* cmd, const char* name, char* gid, char* sec_grp, char* gecos, char* home, char* shell) { ++ ++ pid_t pid; ++ int wstatus; ++ ++ pid = fork(); ++ ++ if(pid > 0) { ++ do { ++ if (waitpid(pid, &wstatus, WUNTRACED | WCONTINUED) == -1) { ++ int errsv = errno; ++ char serr[256] = {0}; ++ strerror_r(errsv, serr, 256); ++ syslog(LOG_ERR, "%s: exec of %s failed with error %d: %s", nssname, cmd, errsv, serr); ++ return -1; ++ } ++ } while (!WIFEXITED(wstatus) && !WIFSIGNALED(wstatus)); ++ if WIFEXITED(wstatus) ++ return WEXITSTATUS(wstatus); ++ else ++ return -1; ++ // Child ++ } else if(pid == 0) { ++ execl(cmd, cmd, "-G", sec_grp, name, "-g", gid, "-c", gecos, "-d", home, "-m", "-s", shell, NULL); ++ syslog(LOG_ERR, "%s: exec of %s failed with errno=%d", nssname, cmd, errno); ++ exit(EXIT_FAILURE); ++ // Error ++ } else { ++ syslog(LOG_ERR, "%s: error forking the child\n", nssname); ++ return -1; ++ } ++} ++ + /* + * If not found in local, look up in tacacs user conf. If user name is not in + * conf, it will be written in conf and created by command 'useradd'. When +@@ -454,6 +489,11 @@ static int create_or_modify_local_user(const char *name, int level, bool existin + bool found = false; + const char* command = existing_user ? "/usr/sbin/usermod": "/usr/sbin/useradd"; + ++ if(strlen(name) > 32) { ++ syslog(LOG_ERR, "%s: Username too long", nssname); ++ return -1; ++ } ++ + fp = fopen(user_conf, "ab+"); + if(!fp) { + syslog(LOG_ERR, "%s: %s fopen failed", nssname, user_conf); +@@ -495,18 +535,19 @@ static int create_or_modify_local_user(const char *name, int level, bool existin + while(lvl >= MIN_TACACS_USER_PRIV) { + user = &useradd_grp_list[lvl]; + if(user->info && user->secondary_grp && user->shell) { +- snprintf(buf, len, "%s -G %s \"%s\" -g %d -c \"%s\" -d /home/%s -m -s %s", +- command, user->secondary_grp, name, user->gid, user->info, name, user->shell); +- if(debug) syslog(LOG_DEBUG, "%s", buf); +- fp = popen(buf, "r"); +- if(!fp || -1 == pclose(fp)) { +- syslog(LOG_ERR, "%s: %s popen failed errno=%d %s", +- nssname, command, errno, strerror(errno)); +- delete_conf_line(name); +- return -1; +- } +- if(debug) ++ char sgid[10] = {0}; ++ char home[64] = {0}; ++ snprintf(sgid, 10, "%d", user->gid); ++ snprintf(home, 63, "/home/%s", name); ++ if(0 != user_mod_add(command, name, sgid, user->secondary_grp, user->info, home, user->shell)) { ++ if(debug) ++ syslog(LOG_ERR, "%s: %s %s failed", nssname, command, name); ++ delete_conf_line(name); ++ return -1; ++ } ++ if(debug) + syslog(LOG_DEBUG, "%s: %s %s success", nssname, command, name); ++ + delete_conf_line(name); + return 0; + } +-- +2.39.3 + diff --git a/src/tacacs/nss/patch/series b/src/tacacs/nss/patch/series index 7b9b5c5779..c348ae6f5d 100644 --- a/src/tacacs/nss/patch/series +++ b/src/tacacs/nss/patch/series @@ -8,3 +8,4 @@ 0008-do-not-create-or-modify-local-user-if-there-is-no-pr.patch 0009-fix-compile-error-strncpy.patch 0010-Send-remote-address-in-TACACS-authorization-message.patch +0011-Replace-popen-shell-execution-with-safer-execle.patch