Skip to content

Use execl instead of popen in RADIUS NSS code to fix vulnerability. #15512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lguohan merged 9 commits into from
Aug 7, 2023
Merged

Use execl instead of popen in RADIUS NSS code to fix vulnerability. #15512

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bfe151c
Use execl instead of popen in RADIUS NSS code to fix vulnerability.
shdasari Jun 16, 2023
d16975e
Use execl instead of popen in RADIUS NSS code to fix vulnerability.
shdasari Jun 16, 2023
4ccb6e8
Merge branch 'sonic-net:master' into radius_execl_fix
shdasari Jun 30, 2023
2f9b568
Update nss_radius_common.c
shdasari Jun 30, 2023
1616bb9
Merge branch 'radius_execl_fix' of https://github.com/shdasari/sonic-…
shdasari Jul 3, 2023
ddd15a1
Updated code to remove memset altogether.
shdasari Jul 3, 2023
5b5741f
Merge branch 'master' into radius_execl_fix
shdasari Jul 6, 2023
31a775a
Handle RADIUS execl related code review comments.
shdasari Jul 10, 2023
f7e090a
Handle RADIUS execl related code review comments.
shdasari Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 143 additions & 71 deletions src/radius/nss/libnss-radius/nss_radius_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
#include <sys/file.h>
#include <regex.h>
#include <time.h>
#include <sys/wait.h>

#include "nss_radius_common.h"

Expand Down Expand Up @@ -167,6 +168,124 @@ static void init_rnm(RADIUS_NSS_CONF_B * conf) {

}

static int user_add(const char* name, char* gid, char* sec_grp, char* gecos,
char* home, char* shell, const char* unconfirmed_user, int many_to_one) {
pid_t pid, w;
int status = 0;
int wstatus;
char cmd[64];

snprintf(cmd, 63, "%s", USERADD);
Copy link
Contributor

@seiferteric seiferteric Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? Or can you use USERADD etc. directly in the execl call? Secondly, looking in nss_radius_common.h (https://github.com/shdasari/sonic-buildimage/blob/radius_execl_fix/src/radius/nss/libnss-radius/nss_radius_common.h#L69) I see this:

#define USERADD "/usr/sbin/useradd"
#define USERMOD "/usr/sbin/usermod"
#define USERDEL "/usr/sbin/userdel"

...

#undef USERADD
#define USERADD "/bin/echo"
#undef USERMOD
#define USERMOD "/bin/echo"
#undef USERDEL
#define USERDEL "/bin/echo"

Why is this being done? Have you tested if it is correctly set to /usr/bin/useradd etc. instead of /bin/echo?

Copy link
Contributor Author

@shdasari shdasari Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes have been tested and the user is added as expected. Note that the #define code is existing code and not added as a part of this code review. We can get rid of the defines and use the command directly however.


pid = fork();

if(pid > 0) {
do {
w = waitpid(pid, &wstatus, WUNTRACED | WCONTINUED);
if (w == -1)
return -1;
} while (!WIFEXITED(wstatus) && !WIFSIGNALED(wstatus));
if WIFEXITED(wstatus)
return WEXITSTATUS(wstatus);
else
return -1;

// Child

} else if(pid == 0) {

if (many_to_one)
execl(cmd, cmd, "-g", gid, "-G", sec_grp, "-c", gecos, "-m", "-s", shell, name, NULL);
else
execl(cmd, cmd, "-U", "-G", sec_grp, "-c", unconfirmed_user, "-d", home, "-m", "-s", shell, name, NULL);
syslog(LOG_ERR, "exec of %s failed with errno=%d", cmd, errno);
return -1;

// Error
} else {
fprintf(stderr, "error forking the child\n");
return -1;
}

return status;
}

static int user_del(const char* name) {
pid_t pid, w;
int status = 0;
int wstatus;
char cmd[64];

snprintf(cmd, 63, "%s", USERDEL);

pid = fork();

if(pid > 0) {
do {
w = waitpid(pid, &wstatus, WUNTRACED | WCONTINUED);
if (w == -1)
return -1;
} while (!WIFEXITED(wstatus) && !WIFSIGNALED(wstatus));
if WIFEXITED(wstatus)
return WEXITSTATUS(wstatus);
else
return -1;

// Child

} else if(pid == 0) {

execl(cmd, cmd, "-r", name, NULL);
syslog(LOG_ERR, "exec of %s failed with errno=%d", cmd, errno);
return -1;

// Error
} else {
fprintf(stderr, "error forking the child\n");
return -1;
}

return status;
}

static int user_mod(const char* name, char* sec_grp) {
pid_t pid, w;
int status = 0;
int wstatus;
char cmd[64];

snprintf(cmd, 63, "%s", USERMOD);

pid = fork();

if(pid > 0) {
do {
w = waitpid(pid, &wstatus, WUNTRACED | WCONTINUED);
if (w == -1)
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, "-c", name, name, NULL);
syslog(LOG_ERR, "exec of %s failed with errno=%d", cmd, errno);
return -1;

// Error
} else {
fprintf(stderr, "error forking the child\n");
return -1;
}

return status;
}

int parse_nss_config(RADIUS_NSS_CONF_B * conf, char * prog,
char * file_buf, int file_buf_sz, int * errnop, int * plockfd) {

Expand Down Expand Up @@ -379,22 +498,6 @@ int unparse_nss_config(RADIUS_NSS_CONF_B * conf, int * errnop, int * plockfd) {
return 0;
}

static int invoke_popen(RADIUS_NSS_CONF_B * conf, char * cmd) {
FILE * fp;
int status = 0;

if (conf->debug)
syslog(LOG_DEBUG, "%s:%s", conf->prog, cmd);

if (((fp = popen(cmd, "r")) == NULL) || (pclose(fp) == -1)) {
syslog(LOG_ERR, "%s: %s: popen()/pclose() failed %p, errno=%d",
conf->prog, cmd, fp, errno);
status = errno;
}

return status;
}

static int radius_getpwnam_r_cleanup(int status, FILE * fp) {
if (fp)
fclose(fp);
Expand Down Expand Up @@ -434,10 +537,8 @@ static int radius_update_user_cleanup(int status) {
int radius_update_user(RADIUS_NSS_CONF_B * conf, const char * user, int mpl) {

char buf[BUFLEN];
char usermod[4096];
struct passwd pw, *result = NULL;
RADIUS_NSS_MPL * rnm = NULL;
int written = 0;
int status;

/* Verify uid is not in the reserved range (<=1000).
Expand Down Expand Up @@ -466,82 +567,53 @@ int radius_update_user(RADIUS_NSS_CONF_B * conf, const char * user, int mpl) {
if (conf->trace)
dump_rnm(mpl, rnm, "update");

written = snprintf(usermod, sizeof(usermod),
"%s -G %s -c \"%s\" \"%s\"", USERMOD, rnm->groups, user, user);

if (written >= sizeof(usermod)) {
syslog(LOG_ERR,
"%s: truncated usermod cmd. Skipping:\"%s\"\n", conf->prog, usermod);
return radius_update_user_cleanup(STATUS_E2BIG);
if(0 != user_mod(user, rnm->groups)) {
syslog(LOG_ERR, "%s: %s %s failed", conf->prog, USERMOD, user);
return -1;
}

return radius_update_user_cleanup(invoke_popen(conf, usermod));
}

static int radius_create_user_cleanup(int status) {
return status;
return 0;
}

int radius_create_user(RADIUS_NSS_CONF_B * conf, const char * user, int mpl,
int unconfirmed) {

char buf[BUFLEN];
char useradd[4096];
char buf[BUFLEN] = {0};
RADIUS_NSS_MPL * rnm = &((conf->rnm)[mpl-1]);
int written = 0;

if (conf->trace)
dump_rnm(mpl, rnm, "create");

if(strlen(user) > 32) {
syslog(LOG_ERR, "%s: Username too long", conf->prog);
return -1;
}

if (conf->many_to_one) {
syslog(LOG_INFO, "%s: Creating user \"%s\"", conf->prog, user);

written = snprintf(useradd, sizeof(useradd),
"%s -g %d -G %s -c \"%s\" -m -s %s \"%s\"",
USERADD, rnm->gid, rnm->groups, rnm->gecos, rnm->shell, user);
char sgid[10] = {0};
char home[64] = {0};
snprintf(sgid, 10, "%d", rnm->gid);
snprintf(home, 63, "/home/%s", user);

} else {
snprintf(buf, sizeof(buf), "Unconfirmed-%ld", time(NULL));
Copy link

@Yarden-Z Yarden-Z Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use sizeof(buf)-1 to leave a null termination place for the buf?


snprintf(buf, sizeof(buf), "Unconfirmed-%ld", time(NULL));
written = snprintf(useradd, sizeof(useradd),
"%s -U -G %s -c \"%s\" -d \"/home/%s\" -m -s %s \"%s\"",
USERADD, rnm->groups, unconfirmed ? buf : user, user,
rnm->shell, user);

}
if(0 != user_add(user, sgid, rnm->groups, rnm->gecos, home, rnm->shell, unconfirmed ? buf : user, conf->many_to_one)) {
syslog(LOG_ERR, "%s: %s %s failed", conf->prog, USERADD, user);

if (written >= sizeof(useradd)) {
syslog(LOG_ERR,
"%s: truncated useradd cmd. Skipping:\"%s\"\n", conf->prog, useradd);
return radius_create_user_cleanup(STATUS_E2BIG);
return -1;
}

syslog(LOG_INFO, "%s: Creating user \"%s\"", conf->prog, user);

return radius_create_user_cleanup(invoke_popen(conf, useradd));
}

static int radius_delete_user_cleanup(int status) {
return status;
return 0;
}

int radius_delete_user(RADIUS_NSS_CONF_B * conf, const char * user) {

char buf[BUFLEN];
char userdel[4096];
int written = 0;

written = snprintf(userdel, sizeof(userdel), "%s -r \"%s\"", USERDEL, user);

if (written >= sizeof(userdel)) {
syslog(LOG_ERR,
"%s: truncated userdel cmd. Skipping:\"%s\"\n", conf->prog, userdel);
return radius_delete_user_cleanup(STATUS_E2BIG);
}

syslog(LOG_INFO, "%s: Deleting user \"%s\"", conf->prog, user);
if(0 != user_del(user)) {
syslog(LOG_ERR, "%s: %s %s failed", conf->prog, USERDEL, user);

return radius_delete_user_cleanup(invoke_popen(conf, userdel));
return -1;
}
return 0;
}

int radius_clear_unconfirmed_users_cleanup(int status, FILE * fp) {
Expand Down