Fix code injection vulnerability of getGroupsForUserCommand #17256

LetianYuan · 2023-04-16T18:56:37Z

What changes are proposed in this pull request?

We split the ShellUtils.getGroupsForUserCommand into two seperate methods ShellUtils.getEffectiveGroupsForUserCommand and ShellUtils.getAllGroupsForUserCommand. Then change the corresponding test cases.

Why are the changes needed?

Executing commands like "bash -c some_command" will introduce code injection vulnerability.

For example, if CommonUtils.getUnixGroups("| echo 123 ") is invoked, the whole command is truncated by |and "echo 123" will be executed.

Therefore, the most simple way to fix it is to not to use "bash -c".

Does this PR introduce any user facing changes?

@Deprecated is marked on the ShellUtils.getGroupsForUserCommand in case some users' codes stongly rely on this method. But in the repository Alluxio/alluxio, ShellUtils.getGroupsForUserCommand is never used anymore. It is replaced by ShellUtils.getEffectiveGroupsForUserCommand and ShellUtils.getAllGroupsForUserCommand.

alluxio-bot · 2023-04-16T18:56:40Z

Thank you for your pull request.
In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement (CLA).
It's all electronic and will take just a few minutes. Please download CLA form here, sign, and e-mail back to [email protected]

alluxio-bot · 2023-04-16T18:56:42Z

Automated checks report:

Commits associated with Github account: FAIL
- It looks like your commits can't be linked to a valid Github account.
  Your commits are made with the email [email protected], which does not allow your contribution to be tracked by Github.
  See this link for possible reasons this might be happening.
  To change the author email address that your most recent commit was made under, you can run:
```
git -c user.name="Name" -c user.email="Email" commit --amend --reset-author
```
  See this answer for more details about how to change commit email addresses.
  Once the author email address has been updated, update the pull request by running:
```
git push --force https://github.com/LetianYuan/alluxio.git fix_code_injection
```
PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - Title is too long (81 characters). Must be at most 72 characters.
  - First word must be capitalized

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

…rUserCommand

alluxio-bot · 2023-04-16T19:15:34Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: PASS

All checks passed!

LetianYuan · 2023-04-17T11:21:18Z

alluxio-bot, check this please

alluxio-bot · 2023-04-17T11:21:22Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: PASS

All checks passed!

ChunxuTang · 2023-07-13T20:09:12Z

FYI: @yyongycy

MacChen01 · 2023-09-07T08:21:18Z

Hi, guys, when will this bug fix be merged?

yyongycy · 2023-09-07T09:23:44Z

core/common/src/test/java/alluxio/util/CommonUtilsTest.java

+    List<String> userAllGroups = new ArrayList<>();
+    userEffectiveGroups.add(userEffectiveGroup1);
+    userEffectiveGroups.add(userEffectiveGroup2);
+    userAllGroups.add(userAllGroup1);


Please add negative case like
ShellUtils.execCommand(ShellUtils.getEffectiveGroupsForUserCommand("| echo 123 ") to see if that works.

Sorry. I'm not quite familiar with PowerMockito. These codes are written after my one-hour's quick learning. So, I don't know how to add a negative case.

If possible, I'll do more researches on PowerMockito. But I'm much busy now.

And it's welcome that you or someone else to give me a hand to add a negative case.

lucyge2022 · 2024-02-28T02:09:51Z

core/common/src/main/java/alluxio/util/ShellUtils.java

   */
+  @Deprecated
  public static String[] getGroupsForUserCommand(final String user) {
    return new String[] {"bash", "-c", "id -gn " + user + "; id -Gn " + user};
  }


i think you can get rid of this function as it exposes vulnerability

yyongycy

lgtm

core/common/src/test/java/alluxio/util/ShellUtilsTest.java

add negative shellutil for "get effective group name from use"

lucyge2022 · 2024-02-28T22:41:47Z

core/common/src/main/java/alluxio/util/ShellUtils.java

+   * @param user the user name
+   * @return the Unix command to get a given user's effective groups list
+   */
+  public static String[] getEffectiveGroupsForUserCommand(final String user){


Suggested change

public static String[] getEffectiveGroupsForUserCommand(final String user){

public static String[] getEffectiveGroupsForUserCommand(final String user) {

core/common/src/test/java/alluxio/util/ShellUtilsTest.java

@LetianYuan

### What changes are proposed in this pull request? Port changes from [@LetianYuan](https://github.com/LetianYuan) in PR : #17256 in urgency for resolving injection vulnerability. ### Why are the changes needed? Resolve issue: https://nvd.nist.gov/vuln/detail/CVE-2023-38889 ### Does this PR introduce any user facing changes? No. pr-link: #18532 change-id: cid-de6273697887692ceda876d556e910bdd529d2ee

@LetianYuan

### What changes are proposed in this pull request? Port changes from [@LetianYuan](https://github.com/LetianYuan) in PR : #17256 in urgency for resolving injection vulnerability. ### Why are the changes needed? Resolve issue: https://nvd.nist.gov/vuln/detail/CVE-2023-38889 ### Does this PR introduce any user facing changes? No. pr-link: #18536 change-id: cid-a8122e3ff3324f1a9d881f55cc425ab30a020c35

lucyge2022 · 2024-03-05T19:37:38Z

Merged as part of #18536
Thanks for the contribution!

@LetianYuan

### What changes are proposed in this pull request? Port changes from [@LetianYuan](https://github.com/LetianYuan) in PR : #17256 in urgency for resolving injection vulnerability. ### Why are the changes needed? Resolve issue: https://nvd.nist.gov/vuln/detail/CVE-2023-38889 ### Does this PR introduce any user facing changes? No. pr-link: #18532 change-id: cid-de6273697887692ceda876d556e910bdd529d2ee

alluxio-bot added the cla-missing label Apr 16, 2023

LetianYuan changed the title ~~fix code injection vulnerability introduced by ShellUtils.getGroupsForUserCommand~~ Fix code injection vulnerability introduced by ShellUtils.getGroupsForUserCommand Apr 16, 2023

LetianYuan changed the title ~~Fix code injection vulnerability introduced by ShellUtils.getGroupsForUserCommand~~ Fix code injection vulnerability of getGroupsForUserCommand Apr 16, 2023

fix code injection vulnerability introduced by ShellUtils.getGroupsFo…

4b5ec25

…rUserCommand

LetianYuan force-pushed the fix_code_injection branch from ad5bf26 to 4b5ec25 Compare April 16, 2023 19:15

Zijian-Zhu removed the cla-missing label Apr 17, 2023

LetianYuan mentioned this pull request Jul 12, 2023

There's code injection vulnerability of alluxio.util.CommonUtils.getUnixGroups #17766

Open

yyongycy reviewed Sep 7, 2023

View reviewed changes

lucyge2022 reviewed Feb 28, 2024

View reviewed changes

yyongycy approved these changes Feb 28, 2024

View reviewed changes

lucyge2022 reviewed Feb 28, 2024

View reviewed changes

core/common/src/test/java/alluxio/util/ShellUtilsTest.java Show resolved Hide resolved

Update core/common/src/test/java/alluxio/util/ShellUtilsTest.java

2a7b95b

add negative shellutil for "get effective group name from use"

lucyge2022 reviewed Feb 28, 2024

View reviewed changes

lucyge2022 reviewed Feb 29, 2024

View reviewed changes

core/common/src/test/java/alluxio/util/ShellUtilsTest.java Outdated Show resolved Hide resolved

Update core/common/src/test/java/alluxio/util/ShellUtilsTest.java

7493cf3

This was referenced Feb 29, 2024

Fix code injection admin #18532

Merged

Fix code injection admin #18536

Merged

lucyge2022 closed this Mar 5, 2024

alluxio-bot mentioned this pull request Mar 12, 2024

Fix code injection admin; Port [#18532] to branch-2.10 #18545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix code injection vulnerability of getGroupsForUserCommand #17256

Fix code injection vulnerability of getGroupsForUserCommand #17256

LetianYuan commented Apr 16, 2023

alluxio-bot commented Apr 16, 2023

alluxio-bot commented Apr 16, 2023

alluxio-bot commented Apr 16, 2023

LetianYuan commented Apr 17, 2023

alluxio-bot commented Apr 17, 2023

ChunxuTang commented Jul 13, 2023

MacChen01 commented Sep 7, 2023

yyongycy Sep 7, 2023

LetianYuan Sep 11, 2023

lucyge2022 Feb 28, 2024 •

edited

Loading

yyongycy left a comment

lucyge2022 Feb 28, 2024

lucyge2022 commented Mar 5, 2024

	public static String[] getEffectiveGroupsForUserCommand(final String user){
	public static String[] getEffectiveGroupsForUserCommand(final String user) {

Fix code injection vulnerability of getGroupsForUserCommand #17256

Fix code injection vulnerability of getGroupsForUserCommand #17256

Conversation

LetianYuan commented Apr 16, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

alluxio-bot commented Apr 16, 2023

alluxio-bot commented Apr 16, 2023

alluxio-bot commented Apr 16, 2023

LetianYuan commented Apr 17, 2023

alluxio-bot commented Apr 17, 2023

ChunxuTang commented Jul 13, 2023

MacChen01 commented Sep 7, 2023

yyongycy Sep 7, 2023

Choose a reason for hiding this comment

LetianYuan Sep 11, 2023

Choose a reason for hiding this comment

lucyge2022 Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

yyongycy left a comment

Choose a reason for hiding this comment

lucyge2022 Feb 28, 2024

Choose a reason for hiding this comment

lucyge2022 commented Mar 5, 2024

lucyge2022 Feb 28, 2024 •

edited

Loading