[#3570] fix(integration-test): Fix possible command inject vulnerability in ProcessBuilder#4337
[#3570] fix(integration-test): Fix possible command inject vulnerability in ProcessBuilder#4337yuqi1129 wants to merge 1 commit intoapache:mainfrom
Conversation
| LOG.info("Sending command \"{}\" to localhost", mergedCommand); | ||
|
|
||
| ProcessBuilder processBuilder = new ProcessBuilder(command); | ||
| ProcessBuilder processBuilder = new ProcessBuilder(subCommandsAsList); |
There was a problem hiding this comment.
Can you please explain why changing this could avoid command injection?
There was a problem hiding this comment.
If the command is a string value, ProcessBuilder will take it as a whole command like ls -al; rm -rf, however, if we split it into a list, -al;, rm, -rf will be reviewed as the parameter of the first command ls, thus dangerous operations like rm -rf will not take effect.
There was a problem hiding this comment.
Can you please put it on the PR description.
There was a problem hiding this comment.
Let a example. If we the command is a string value,
ProcessBuilderwill take it as a whole command likels -al; rm -rf, however, if we split it into a list,-al;,rm,-rfwill be reviewed as the parameter of the first value, thus dangerous operation likerm -rfwill not take effect.
It's really hard to understand the meanings of what you say, can you please rephrase the words.
There was a problem hiding this comment.
Why changing from array to list will solve the problem?
There was a problem hiding this comment.
List<String> subCommandsAsList = new ArrayList<>(Arrays.asList(command));
Why do we need to wrap a list again with ArrayList.
|
I will temporarily close this PR as it could not solve the problem thoroughly as we lack more details about the issue. Once more information is provided, I will continue working on this issue. |
2 participants
What changes were proposed in this pull request?
Fix potential command inject bugs in
ProcessBuilder. Passing a single string as the parameter ofProcessBuilderwill lead to the command inject vulnerability, for example, if the value of the command isls -al ;rm -rf, then it will launch a process that will executels -alfirst and thenrm -rf.However, If we split the command to a list like
['ls', '-al', ';rm', "-rf"], the list will view as a single command and values start from-alwill be viewed the parameter as the commandls, so commandrm -rfwill not take effect.Why are the changes needed?
To fix the bug.
Fix: #3570
Does this PR introduce any user-facing change?
N/A.
How was this patch tested?
Existing IT.