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

CVE-2023-40590 fix capitalized all environment variables on Windows #1646

Closed
irwand opened this issue Sep 6, 2023 · 1 comment · Fixed by #1650
Closed

CVE-2023-40590 fix capitalized all environment variables on Windows #1646

irwand opened this issue Sep 6, 2023 · 1 comment · Fixed by #1650

Comments

@irwand
Copy link

irwand commented Sep 6, 2023

This fix:
6029211
capitalized all environment variables on Windows. It can be illustrated by this short program:

import subprocess

print(subprocess.check_output(
    "set | findstr /c:SystemRoot /i", shell=True, universal_newlines=True
))

import git

print(subprocess.check_output(
    "set | findstr /c:SystemRoot /i", shell=True, universal_newlines=True
))

The output is:

SystemRoot=C:\Windows

SYSTEMROOT=C:\Windows

This side effect breaks our use case currently. We use gnu make in cygwin for our build, in which all environment variables are case sensitive.

The core problem was unittest.mock.patch.dict(os.environ, {"NoDefaultCurrentDirectoryInExePath": "1"}) -- in which it will try to treat os.environ as a dictionary, but os.environ is not just a simple dictionary. It actually remembers the original casing of the environment variable. Unfortunately when reading it as dictionary it capitalize all letters.

We can also observe the same side effect with this code below:

import os
import subprocess
import unittest.mock

print(subprocess.check_output(
    "set | findstr /c:SystemRoot /i", shell=True, universal_newlines=True
))

with unittest.mock.patch.dict(os.environ, {"NoDefaultCurrentDirectoryInExePath": "1"}):
    pass

print(subprocess.check_output(
    "set | findstr /c:SystemRoot /i", shell=True, universal_newlines=True
))

The side effect is the same as above.

@Byron
Copy link
Member

Byron commented Sep 7, 2023

Thanks for reporting!

It's an unexpected side-effect for sure :/. Maybe there are other ways to solve the original problem instead of affecting the callers os.environ like that.

CC @EliahKagan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants