Use execle instead of popen in tacas nss to avoid shell escape exploits #15284
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seiferteric
changed the title
|
@lguohan Can you review? Also looks like some testbed failure that has nothging to do with my changes, can we restart the failed test above? Thanks. |
|
@a-barboza pls help review the change |
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Okay the failure mentions lines in the patch file, but doesn't actually say what the problem is? Some formatting requirement? The lines are: 19: + +#include <sys/wait.h> 28: + +int user_mod_add(const char* cmd, const char* name, char* gid, char* sec_grp, char* gecos, char* home, char* shell) { But when you click on the errors it says build not found... Guess I will try the build again. |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Commenter does not have sufficient privileges for PR 15284 in repo sonic-net/sonic-buildimage |
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@adyeung @seiferteric @shdasari 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 |
|
Okay, the test was passing, I was just looking at the old test the first time it failed. Yes please review. |
a-barboza
approved these changes
Jun 22, 2023
|
@liuh-80 to review |
liuh-80
reviewed
Jun 25, 2023
src/tacacs/nss/patch/0011-Replace-popen-shell-execution-with-safer-execle.patch
Outdated
Show resolved
Hide resolved
liuh-80
reviewed
Jun 25, 2023
src/tacacs/nss/patch/0011-Replace-popen-shell-execution-with-safer-execle.patch
Show resolved
Hide resolved
liuh-80
approved these changes
Jun 27, 2023
maipbui
approved these changes
Jun 27, 2023
lguohan
pushed a commit
that referenced
this pull request
Aug 7, 2023
…ability. (#15512) 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 "/". 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.
sonic-otn
pushed a commit
to sonic-otn/sonic-buildimage
that referenced
this pull request
Sep 20, 2023
…ts (sonic-net#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.
sonic-otn
pushed a commit
to sonic-otn/sonic-buildimage
that referenced
this pull request
Sep 20, 2023
…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.
lixiaoyuner
pushed a commit
to lixiaoyuner/sonic-buildimage
that referenced
this pull request
Feb 6, 2024
Related work items: sonic-net#94, sonic-net#13789, sonic-net#14149, sonic-net#14515, sonic-net#14788, sonic-net#14922, sonic-net#14933, sonic-net#15284, sonic-net#15383, sonic-net#15464, sonic-net#15519, sonic-net#15521, sonic-net#15575, sonic-net#15636, sonic-net#15652, sonic-net#15684, sonic-net#15708, sonic-net#15725, sonic-net#15739, sonic-net#15755, sonic-net#15756, sonic-net#15757
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.