-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
bpo-30730: Prevent environment variables injection in subprocess on Windows. #2325
Merged
serhiy-storchaka
merged 3 commits into
python:master
from
serhiy-storchaka:env-var-injection
Jun 23, 2017
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@serhiy-storchaka I am curious about this. Is there a particular version of Windows this works on? We copied this code in PyPy, but it is failing tests for
putenv("=hidden", "foo")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattip, the system tracks the working directory on each drive using environment variable names that start with "=", such as "=C:". Python's
os.chdir()
implements this. Such names are accessible via WinAPIGetEnvironmentStringsW
,GetEnvironmentVariableW
, andSetEnvironmentVariableW
. In most contexts such variables are hidden or filtered out, and using them at the application level isn't officially supported. In particular, CRT environment functions such asputenv
andgetenv
do not support them, and they're filtered out of C[_w]environ
, from which Python initializesos.environ
.Here's a manual example:
Or set the drive working directory in the environment of a child process:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick reply. So if I understand correctly while the code (in three places: this one and twice in
posix_module
) may permit the use of=hidden
, it is not guaranteed that the subsequent system call will succeed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Names that begin with "=" always work with the Windows functions
GetEnvironmentVariableW
andSetEnvironmentVariableW
, and they're inherited fine by child processes. For example:However, such names are not officially supported. IMO, this is primarily because the C runtime filters them out of its
[_w]environ
global variable and doesn't support them in environment functions such asputenv()
. This in turn is because C and POSIX are tightly linked, and POSIX disallows "=" in environment variable names. Python'sos.environ
,os.getenv()
, andos.putenv()
are based on the Windows C runtime (i.e. ucrt), so they have the same limits, and probably should since these are C/POSIX constructs. Maybe one day Python will provide native Windows functionality in the os module that directly accesses the environment block of the process and directly exposes the WinAPIGetEnvironmentVariableW
andSetEnvironmentVariableW
functions. For now, that's only possible with extensions such as PyWin32 or ctypes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. So I will keep PyPy's
putenv
consistent with CPython: python allows=hidden
as a name, but then theputenv
syscall will fail.