Skip to content

use safe local envvars for subprocess stuff #170

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

Open
guykisel wants to merge 1 commit into
base: master
Choose a base branch
from
Open

use safe local envvars for subprocess stuff #170

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion inlineplz/linters/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,13 +277,20 @@ def run_command(command, log_on_fail=False, log_all=False):
shell = False
if os.name == 'nt':
shell = True
local_env = {}
safe_envvars = [
b'PATH', b'PYTHONPATH', b'GOPATH', b'SystemRoot',
Copy link
Collaborator

@efokschaner efokschaner Jul 31, 2016

Choose a reason for hiding this comment

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

Looks like this could balloon into many language + tool aware exceptions. Can I get the context on what this is trying to solve?

Copy link
Owner Author

@guykisel guykisel Jul 31, 2016

Choose a reason for hiding this comment

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

the idea here is to reduce risk when running potentially untrusted subprocesses by providing the absolute minimum necessary environment variables. i'm also going to refactor the way the token is being passed in the bot to make this safer.

Copy link
Collaborator

@efokschaner efokschaner Jul 31, 2016

Choose a reason for hiding this comment

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

Ah, if this is about security then the gains from doing this are barely worth it. Processes can just look at the environment variables on their parent if they're feeling malicious. If you want to run code without trusting it you must sandbox it / jail it using OS functionality or machine separation.

Copy link
Contributor

@theTechnoWeenie theTechnoWeenie Aug 1, 2016

Choose a reason for hiding this comment

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

Yeah, that's a really good point about the per language ballooning. Running the comment portion in a separate container would be an option, but then you'd need to think about authorization to be able to use the comment service... how deep would that rabbit hole go?

b'HOME', b'USER', b'APPDATA'
]
for envvar in safe_envvars:
local_env[envvar] = os.environ.get(envvar, b'')
proc = subprocess.Popen(
command,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
shell=shell,
env=os.environ
env=local_env
)
stdout, stderr = proc.communicate()
stdout = stdout.decode('utf-8', errors='replace')
Expand Down