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

shdasari · 2023-06-16T11:16:15Z

Why I did it

#15284 fixes a case of shell escape exploit for TACACS+. This applies to RADIUS as well. RADIUS creates an unconfirmed user locally on the switch while attempting authentication. popen() is used to execute useradd,usermod and userdel commands. This exposes a vulnerability where a tactically designed username (which could contain explicit linux commands) can lead to getting executed as root.

An example of such a username could be "asd";echo>remoteRCE2;#". This leads to remoteRCE2 getting created in "/".

Work item tracking

Microsoft ADO (number only):

How I did it

All calls to popen() used to execute useradd, usermod and userdel are replaced with fork()/execl().

How to verify it

Prior to the fix, following is the behavior:

[s@i vm] ssh "asd";echo>remoteRCE2;#"@1.1.1.1
asd";echo>remoteRCE2;#@1.1.1.1's password:
Permission denied, please try again.

On the SONiC switch,

root@sonic:/# ls
accton_as7816_monitor.log home lib64 remoteRCE2 sys
bin host libx32 root tmp
boot initrd.img media run usr
cache.tgz initrd.img.old mnt sbin var
dev lib opt sonic vmlinuz
etc lib32 proc srv vmlinuz.old
root@sonic:/# ls -l

With the fix:

[s@i vm] ssh "asd";echo>remoteRCE2;#"@1.1.1.1
asd";echo>remoteRCE2;#@1.1.1.1's password:
Permission denied, please try again.

root@sonic:/# ls
accton_as7816_monitor.log etc lib mnt sbin usr
bin home lib32 opt sonic var
boot host lib64 proc srv vmlinuz
cache.tgz initrd.img libx32 root sys vmlinuz.old
dev initrd.img.old media run tmp

Verified that RADIUS authentication works as expected for valid users as well.

a-barboza · 2023-06-16T17:42:06Z

Why I did it

#15284 fixes a case of shell escape exploit for TACACS+. This applies to RADIUS as well. RADIUS creates an unconfirmed user locally on the switch while attempting authentication. popen() is used to execute useradd,usermod and userdel commands. This exposes a vulnerability where a tactically designed username (which could contain explicit linux commands) can lead to getting executed as root.

An example of such a username could be "asd";echo>remoteRCE2;#". This leads to remoteRCE2 getting created in "/".

Work item tracking

Microsoft ADO (number only):

How I did it

All calls to popen() used to execute useradd, usermod and userdel are replaced with fork()/execl().

How to verify it

Prior to the fix, following is the behavior:

[s@i vm] ssh "asd";echo>remoteRCE2;#"@1.1.1.1 asd";echo>remoteRCE2;#@1.1.1.1's password: Permission denied, please try again.

On the SONiC switch,

root@sonic:/# ls accton_as7816_monitor.log home lib64 remoteRCE2 sys bin host libx32 root tmp boot initrd.img media run usr cache.tgz initrd.img.old mnt sbin var dev lib opt sonic vmlinuz etc lib32 proc srv vmlinuz.old root@sonic:/# ls -l

With the fix:

[s@i vm] ssh "asd";echo>remoteRCE2;#"@1.1.1.1 asd";echo>remoteRCE2;#@1.1.1.1's password: Permission denied, please try again.

root@sonic:/# ls accton_as7816_monitor.log etc lib mnt sbin usr bin home lib32 opt sonic var boot host lib64 proc srv vmlinuz cache.tgz initrd.img libx32 root sys vmlinuz.old dev initrd.img.old media run tmp

Verified that RADIUS authentication works as expected for valid users as well.

@adyeung @seiferteric @shdasari

The semgrep flagged failure appears to be a false +ve. The compiler cannot optimize that particular memset, since the target of the memset is modified inside the function, and returned from the function, and used else where.

I recommend merging this patch. There are some minor enhancements (replace integer constants with macros which could be done). But, I recommend merging this patch. Please let me know if anything else needs to be done from my side. thank you

lguohan · 2023-06-23T01:15:19Z

can you check the semgrep failure?

a-barboza · 2023-06-27T00:26:54Z

src/radius/nss/libnss-radius/nss_radius_common.c

-            "%s -U -G %s -c \"%s\" -d \"/home/%s\" -m -s %s \"%s\"",
-            USERADD, rnm->groups, unconfirmed ? buf : user, user,
-            rnm->shell, user);
+    memset(buf, 0, sizeof(buf));


This memset seems redundant? snprintf() Man Page: "The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str." semgrep appears to complain about it.

Updated memset to memset_s to handle semgrep failure.

a-barboza

semgrep run passes.

…buildimage into radius_execl_fix

shdasari · 2023-07-07T04:06:38Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2023-07-07T04:06:40Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2023-07-07T04:06:51Z

Azure Pipelines successfully started running 1 pipeline(s).

adyeung · 2023-07-07T20:06:45Z

@lguohan pls check if you are ok with the vulnerability fix

seiferteric

Added a couple minor comments, but otherwise looks good to me.

seiferteric · 2023-07-07T20:11:21Z

src/radius/nss/libnss-radius/nss_radius_common.c

@@ -167,6 +168,109 @@ static void init_rnm(RADIUS_NSS_CONF_B * conf) {

 }

+static int user_add(const char* cmd, const char* name, char* gid, char* sec_grp, char* gecos, 


What is the purpose of accepting cmd as first argument if you have one function for each command (user_add, user_del, user_mod)?

seiferteric · 2023-07-07T20:15:02Z

src/radius/nss/libnss-radius/nss_radius_common.c

+
+    } else if(pid == 0) {
+
+        return execl(cmd, cmd, "-G", sec_grp, "-c", name, name, NULL);


Maybe we can add log in case of execl failure.

lguohan · 2023-07-07T21:00:34Z

@adyeung , i see some comments, as soon as eric sign off, i can get it merged.

seiferteric · 2023-07-10T22:10:33Z

src/radius/nss/libnss-radius/nss_radius_common.c

+    int wstatus;
+    char cmd[64];
+
+    snprintf(cmd, 63, "%s", USERADD);


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?

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.

Yarden-Z · 2023-08-15T14:02:34Z

src/radius/nss/libnss-radius/nss_radius_common.c


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


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

…ability. (sonic-net#15512) Why I did it sonic-net#15284 fixes a case of shell escape exploit for TACACS+. This applies to RADIUS as well. RADIUS creates an unconfirmed user locally on the switch while attempting authentication. popen() is used to execute useradd,usermod and userdel commands. This exposes a vulnerability where a tactically designed username (which could contain explicit linux commands) can lead to getting executed as root. An example of such a username could be "asd";echo>remoteRCE2;#". This leads to remoteRCE2 getting created in "/". How I did it All calls to popen() used to execute useradd, usermod and userdel are replaced with fork()/execl(). How to verify it Prior to the fix, following is the behavior: [s@i vm] ssh "asd";echo>remoteRCE2;#"@1.1.1.1 asd";echo>remoteRCE2;#@1.1.1.1's password: Permission denied, please try again. On the SONiC switch, root@sonic:/# ls accton_as7816_monitor.log home lib64 remoteRCE2 sys bin host libx32 root tmp boot initrd.img media run usr cache.tgz initrd.img.old mnt sbin var dev lib opt sonic vmlinuz etc lib32 proc srv vmlinuz.old root@sonic:/# ls -l With the fix: [s@i vm] ssh "asd";echo>remoteRCE2;#"@1.1.1.1 asd";echo>remoteRCE2;#@1.1.1.1's password: Permission denied, please try again. root@sonic:/# ls accton_as7816_monitor.log etc lib mnt sbin usr bin home lib32 opt sonic var boot host lib64 proc srv vmlinuz cache.tgz initrd.img libx32 root sys vmlinuz.old dev initrd.img.old media run tmp Verified that RADIUS authentication works as expected for valid users as well.

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

bfe151c

a-barboza approved these changes Jun 22, 2023

View reviewed changes

a-barboza reviewed Jun 27, 2023

View reviewed changes

shdasari and others added 3 commits June 29, 2023 22:39

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

d16975e

Merge branch 'sonic-net:master' into radius_execl_fix

4ccb6e8

Update nss_radius_common.c

2f9b568

Updated memset to memset_s to handle semgrep failure.

a-barboza approved these changes Jun 30, 2023

View reviewed changes

shdasari added 2 commits July 3, 2023 00:30

Merge branch 'radius_execl_fix' of https://github.com/shdasari/sonic-…

1616bb9

…buildimage into radius_execl_fix

Updated code to remove memset altogether.

ddd15a1

a-barboza approved these changes Jul 4, 2023

View reviewed changes

Merge branch 'master' into radius_execl_fix

5b5741f

adyeung requested a review from seiferteric July 7, 2023 20:07

seiferteric reviewed Jul 7, 2023

View reviewed changes

shdasari added 2 commits July 9, 2023 22:09

Handle RADIUS execl related code review comments.

31a775a

Handle RADIUS execl related code review comments.

f7e090a

seiferteric reviewed Aug 3, 2023

View reviewed changes

seiferteric approved these changes Aug 4, 2023

View reviewed changes

lguohan enabled auto-merge (squash) August 7, 2023 16:47

lguohan disabled auto-merge August 7, 2023 16:47

lguohan merged commit d9393b0 into sonic-net:master Aug 7, 2023

Yarden-Z reviewed Aug 15, 2023

View reviewed changes

maipbui mentioned this pull request Mar 3, 2024

[radius]: Use execl instead of popen in RADIUS NSS code to fix vulner… Azure/sonic-buildimage-msft#277

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shdasari commented Jun 16, 2023

a-barboza commented Jun 16, 2023

Why I did it

Work item tracking

How I did it

How to verify it

lguohan commented Jun 23, 2023

a-barboza Jun 27, 2023

a-barboza left a comment

shdasari commented Jul 7, 2023

mssonicbld commented Jul 7, 2023

azure-pipelines bot commented Jul 7, 2023

adyeung commented Jul 7, 2023

seiferteric left a comment

seiferteric Jul 7, 2023

seiferteric Jul 7, 2023

lguohan commented Jul 7, 2023

seiferteric Jul 10, 2023

shdasari Aug 4, 2023

Yarden-Z Aug 15, 2023

		@@ -167,6 +168,109 @@ static void init_rnm(RADIUS_NSS_CONF_B * conf) {

		}

		static int user_add(const char* cmd, const char* name, char* gid, char* sec_grp, char* gecos,


		} else if(pid == 0) {

		return execl(cmd, cmd, "-G", sec_grp, "-c", name, name, NULL);


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

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

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

Conversation

shdasari commented Jun 16, 2023

Why I did it

Work item tracking

How I did it

How to verify it

a-barboza commented Jun 16, 2023

Why I did it

Work item tracking

How I did it

How to verify it

lguohan commented Jun 23, 2023

a-barboza Jun 27, 2023

Choose a reason for hiding this comment

a-barboza left a comment

Choose a reason for hiding this comment

shdasari commented Jul 7, 2023

mssonicbld commented Jul 7, 2023

azure-pipelines bot commented Jul 7, 2023

adyeung commented Jul 7, 2023

seiferteric left a comment

Choose a reason for hiding this comment

seiferteric Jul 7, 2023

Choose a reason for hiding this comment

seiferteric Jul 7, 2023

Choose a reason for hiding this comment

lguohan commented Jul 7, 2023

seiferteric Jul 10, 2023

Choose a reason for hiding this comment

shdasari Aug 4, 2023

Choose a reason for hiding this comment

Yarden-Z Aug 15, 2023

Choose a reason for hiding this comment