Skip to content
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

Jump to bottom

[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingProvider #36315

Closed
wants to merge 1 commit into from
Closed

[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingProvider #36315

wants to merge 1 commit into from

Conversation

HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR proposes to avoid using bash -c in ShellBasedGroupsMappingProvider. This could allow users a command injection.

Why are the changes needed?

For a security purpose.

Does this PR introduce any user-facing change?

Virtually no.

How was this patch tested?

Manually tested.

@github-actions github-actions bot added the CORE label Apr 22, 2022
@HyukjinKwon
Copy link
Member Author

cc @JoshRosen @gengliangwang mind taking a look please?

@HyukjinKwon HyukjinKwon changed the title [SPARK-38992][CORE]Avoid using bash -c in ShellBasedGroupsMappingProvider [SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingProvider Apr 22, 2022
JoshRosen
JoshRosen reviewed Apr 22, 2022
View reviewed changes
Copy link
Contributor

@JoshRosen JoshRosen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It looks likehadoop-common's Shell.getGroupsForUserCommand() implements similar functionality, except it invokes both id -Gn and id -gn so that the primary group can be returned first (see HADOOP-10401):

https://github.com/apache/hadoop/blame/7398a0f1b2ca434f41d1414f884731637b9855bf/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java#L220-L229

They use bash -c because they have to issue two id calls and want to avoid the added overhead of two separate fork() calls. They use a helper function to escape the username and thereby avoid command injection.

They were using bash -c even prior to that change, though. I'm not sure why, since I think the id command should be a built in on any POSIX system: https://pubs.opengroup.org/onlinepubs/009604499/utilities/id.html

Given this, maybe it's safe for us to drop the bash -c and just invoke id directly?

HyukjinKwon added a commit that referenced this pull request Apr 22, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
9cc2ae7 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes #36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit c83618e)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon added a commit that referenced this pull request Apr 22, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
1d524a8 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes #36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit c83618e)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@HyukjinKwon
Copy link
Member Author

Merged to master, branch-3.3, branch-3.2, branch-3.1, and branch-3.0.

HyukjinKwon added a commit that referenced this pull request Apr 22, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
ee1d0c8 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes #36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit c83618e)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon added a commit that referenced this pull request Apr 22, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
aeea56d 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes #36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit c83618e)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
zheniantoushipashi pushed a commit to zheniantoushipashi/spark that referenced this pull request Jul 18, 2022
@HyukjinKwon @zheniantoushipashi
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
c5613dd 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@zheniantoushipashi zheniantoushipashi mentioned this pull request Jul 18, 2022
Closed
Mrhs121 pushed a commit to Mrhs121/spark that referenced this pull request Jul 22, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
2e3c101 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Mrhs121 pushed a commit to Mrhs121/spark that referenced this pull request Jul 27, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
eb5428a 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@Mrhs121 Mrhs121 mentioned this pull request Jul 27, 2022
Merged
kazuyukitanimura pushed a commit to kazuyukitanimura/spark that referenced this pull request Aug 10, 2022
@HyukjinKwon @sunchao
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
951775b 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit c83618e)
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@leoluan2009 leoluan2009 mentioned this pull request Aug 22, 2022
Closed
Mrhs121 pushed a commit to Kyligence/spark that referenced this pull request Aug 30, 2022
@HyukjinKwon @Mrhs121
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
14dd2c7 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
skhandrikagmail added a commit to ravitejarp/spark that referenced this pull request Sep 7, 2022
@skhandrikagmail
Merging ShellBasedGroupsMappingProvider apache#36315
38cada6 
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingProvider apache#36315
chenzhx pushed a commit to chenzhx/spark that referenced this pull request Sep 9, 2022
@HyukjinKwon
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
01c2248 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
chenzhx pushed a commit to Kyligence/spark that referenced this pull request Sep 13, 2022
@HyukjinKwon @chenzhx
[SPARK-38992][CORE] Avoid using bash -c in ShellBasedGroupsMappingPro…
6c639cb 
…vider

### What changes were proposed in this pull request?

This PR proposes to avoid using `bash -c` in `ShellBasedGroupsMappingProvider`. This could allow users a command injection.

### Why are the changes needed?

For a security purpose.

### Does this PR introduce _any_ user-facing change?

Virtually no.

### How was this patch tested?

Manually tested.

Closes apache#36315 from HyukjinKwon/SPARK-38992.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
@HyukjinKwon HyukjinKwon deleted the SPARK-38992 branch January 15, 2024 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshRosen JoshRosen JoshRosen approved these changes

@gengliangwang gengliangwang gengliangwang approved these changes

Assignees
No one assigned
Labels
CORE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HyukjinKwon @JoshRosen @gengliangwang