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.

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

Allow arbitrary arguments to be bassed through the pip module #55492

Merged
merged 2 commits into from Dec 4, 2019

Conversation

Akm0d
Copy link
Contributor

@Akm0d Akm0d commented Dec 2, 2019

What does this PR do?

Port #52327 to master

@Akm0d Akm0d requested a review from as a code owner Dec 2, 2019
@ghost ghost requested a review from xeacott Dec 2, 2019
@Akm0d Akm0d self-assigned this Dec 2, 2019
@Akm0d Akm0d added this to PR needs merge to master in PRs to port to master via automation Dec 2, 2019
@Akm0d Akm0d added this to the Approved milestone Dec 2, 2019
Copy link
Contributor

@dwoz dwoz left a comment

Seems like we need to validate that we are not introducing the potential for a shell escape here

@Akm0d
Copy link
Contributor Author

@Akm0d Akm0d commented Dec 3, 2019

Seems like we need to validate that we are not introducing the potential for a shell escape here

The python documentation on subprocess has this to say:

Unlike some other popen functions, this implementation will never implicitly call a system shell. This means that all characters, including shell metacharacters, can safely be passed to child processes. If the shell is invoked explicitly, via shell=True, it is the application’s responsibility to ensure that all whitespace and metacharacters are quoted appropriately to avoid shell injection vulnerabilities.

When using shell=True, the shlex.quote() function can be used to properly escape whitespace and shell metacharacters in strings that are going to be used to construct shell commands.

Perhaps it would be good to wrap all options in shlex.quote() before appending them to the pip command

@Akm0d
Copy link
Contributor Author

@Akm0d Akm0d commented Dec 3, 2019

Actually, this is already handled in cmdmod.py which is where the pip command is ultimately handed to:

if python_shell is not True and not salt.utils.platform.is_windows() and not isinstance(cmd, list):
        cmd = salt.utils.args.shlex_split(cmd)

xeacott
xeacott approved these changes Dec 4, 2019
@dwoz dwoz merged commit e96c0fa into saltstack:master Dec 4, 2019
49 checks passed
PRs to port to master automation moved this from PR needs merge to master to PR merged Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants