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

Fix shell=True security issues #73

Closed
jasikpark opened this issue Oct 15, 2018 · 4 comments
Closed

Fix shell=True security issues #73

jasikpark opened this issue Oct 15, 2018 · 4 comments
Labels
bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@jasikpark
Copy link
Contributor

Because the main repo contains functions with shell=True as parameters, the Codacy PR automated review fails on every pull request. Is there a way to turn this off or to fix the code so that Codacy is happy with it?

@alichtman
Copy link
Owner

alichtman commented Oct 15, 2018

We should remove all instances of shell=True and use shlex.split() for processing args, as shown in this link.

Do you want to open a PR, @jasikpark?

@alichtman alichtman added the help wanted Extra attention is needed label Oct 15, 2018
@alichtman
Copy link
Owner

I played around with this tonight and couldn't get any files to copy without the shell=True argument. I'm not sure how to solve this bug, but I remember running into it when I was working on this like 6 months ago and this was the best solution I had.

@alichtman alichtman added the bug Something isn't working label Oct 15, 2018
@jasikpark
Copy link
Contributor Author

jasikpark commented Oct 15, 2018

This stackoverflow explanation seems to imply that the calls to the shell should be broken up into individual programs to prevent needing to use shell=True, replicating the lost functionality natively in python.

So instead of

sp.run("ls {0}/.cargo/bin/ > {1}/cargo_list.txt".format(os.path.expanduser('~'), backup_path), shell=True, stdout=sp.PIPE)

we have

result = sp.run(["ls","{0}/.cargo/bin/"],stdout=sp.PIPE)

and result.stdout will store a byte string of the result of that shell call, which we can use python to store in the correct file.

getting stdout from running a program

@alichtman
Copy link
Owner

alichtman commented Oct 15, 2018

That sounds reasonable to me.

We should extract that to a method called run_shell_cmd(command).

@alichtman alichtman added this to the v1.4 milestone Oct 18, 2018
@alichtman alichtman changed the title The Codacy PR Check fails on every pull request Fix shell=True security issues Oct 19, 2018
This was referenced Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants