bpo-34489: subprocess / fixed vulnerability by execution of batch-files (.cmd/.bat) in python for windows / insufficient escape

[sebres]

bpo-34489

Closes bpo-34489 vulnerability "insufficient escape of special chars for quoting of arguments by exec process" for python-language, if executing windows batch-files (bat/cmd).

Fixed for 2.7, but can be easy merged (or cherry-picked) for 3.x up to master.

What version of python is affected?

All

Does this issue reproduce with the latest master?

Yes

What did you do?

Execution of batch-file using subprocess module with arguments containing some special meta-characters.

A recipe for reproducing the error as well as more extensive PoC with additional info (and more lang's affected also) - github/sebres/PoC/SB-0D-001-win-exec
A complete runnable program - test-dump-inv.py.

A simple example:

 # invoke exe-file:
 >>> import subprocess
 >>> subprocess.call(['test-dump.exe', 'test&whoami'])
+    `test-dump.exe´ `test&whoami´
 # invoke cmd-file:
 >>> subprocess.call(['test-dump.CMD', 'test&whoami'])
-    `test-dump.exe´ `test´my_domain\sebres

For more "broken" cases, see the result of my test-suite - python.diff

What did you expect to see?

Arguments are escaped/quoted properly.

What did you see instead?

Arguments are insufficient escaped/quoted, so it is vulnerable currently.

Solution:

• the algorithm description
• how it was fixed in TCL as C-code (see the function BuildCommandLine)

Possible similar issues:

[[bpo-33515]](https://bugs.python.org/issue33515)

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

[sebres]

Regression tests extended, to invoke it with debug-output (as well as to execute also slow particular tests) use:

python Lib\test\test_subprocess.py --debug

[eryksun]

The CMD shell does not use VC++ rules to parse the command line. Backslash is not an escape character in CMD, and CMD does not remove double quotes that are added to escape special characters and white space. For example, given a test.bat script that executes echo %1:

>>> cmd = r'.\test.bat "\"eggs spam"'
>>> subprocess.call(cmd)
"\"eggs
0

Also, there is no way to really escape "%" in a cmd.exe /c command. At best you can obfuscate name matching by adding ^ or " special characters, which are unlikely to be used in existing variable names. This is apparent with a contrived counterexample, such as the following:

>>> cmd = r'.\test.bat %"eggs"%'
>>> environ = os.environ.copy()
>>> environ['"eggs"'] = 'EGGS'
>>> subprocess.call(cmd, env=environ)
EGGS

[sebres]

Backslash is not an escape character in CMD ... For example, given a test.bat script that executes echo %1.

I know.
But the batch is "normally" written quasi as interim interface to invoke one or more exe-files (using some.exe param1 ... paramN %3 %4 %5 or even do something...; some.exe %*) to work with an ascii arguments, set environment, etc and to bypass other (even non-ascii) arguments to the executable from the batch.

In this case it does no matter what echo %1 will produce as an output, more interesting what does some.exe %1 here.
But also working with some batch-internals like for-cycle etc, across some files is safe in this context, because in this case it does no matter also, just because " character is not allowed as file name (or path segment) in windows.

Additionally this does not matter at al, because otherwise it is simply unusable because vulnerable by supplying of args like this to the batch-files - see python.diff.
It just invokes additionally another executable corresponding not proper escaped &, | etc!

So in my opinion a bit "wrong" echo (if it expected at all) is many times better as "wrong" execution flow (let alone a vulnerability).

Also, there is no way to really escape "%"

Nope, you are wrong, there is a way (and it is already implemented), just checkout and try, so for example compare:

-origin\python -Wd -3 -E -tt  -R ./Lib/test/regrtest.py -v -m "*inject*" test_subprocess.py
+pr8906\python -Wd -3 -E -tt  -R ./Lib/test/regrtest.py -v -m "*inject*" test_subprocess.py
 ...
 Ran 156 tests in ...
-FAILED (failures=3, skipped=84)
+OK (skipped=2)

or simply try it using a simple example above

subprocess.call(['test-dump.exe', r'%USERDOMAIN%\&\%USERNAME%'])
subprocess.call(['test-dump.CMD', r'%USERDOMAIN%\&\%USERNAME%'])

results in:

 >>> subprocess.call(['test-dump.exe', r'%USERDOMAIN%\&\%USERNAME%'])
     `test-dump.exe´ `%USERDOMAIN%\&\%USERNAME%´
 >>> subprocess.call(['test-dump.CMD', r'%USERDOMAIN%\&\%USERNAME%'])
-    # inside origin\python
-    `test-dump.exe´ `MY_DOMAIN\´'\sebres' is not recognized as an internal or external command, operable program or batch file.
+    # inside pr8906\python
+    `test-dump.exe´ `%USERDOMAIN%\&\%USERNAME%´

In my opinion both should produce the same result. An interpolation of variables (much less interim invocations of some executables) is a nonsense!

[eryksun]

In this case it does no matter what echo %1 will produce as an output, more interesting what does some.exe %1 here.

But in the example I provided %1 is parsed incorrectly. The first argument is supposed to be "\"eggs spam", not "\"eggs. This will be an error for any batch script that depends on the parsed value in %1.

Also, there is no way to really escape "%"

Nope, you are wrong

Look at the output from your version of list2cmdline:

>>> print(list2cmdline(['test-dump.CMD', r'%USERDOMAIN%\&\%USERNAME%']))
test-dump.CMD ""%"USERDOMAIN"%"\&\\"%"USERNAME"%""

The argument begins with "", which in CMD is an empty quoted string. Then it has %"USERDOMAIN"%, which is just like what I showed in my example. This is an evironment variable named "USERDOMAIN", including the double quote characters. If there is no such variable, CMD will include the literal string, but this isn't actually escaping the percent characters.

[sebres]

But in the example I provided %1 is parsed incorrectly.

My proposal does nothing is this direction if args supplied as string, it works only if you try to use called as "safe" variant, so using cmd as tuple or array. Please provide better (more speaking) example, so something like subprocess.call(['1st', '2nd', '3rd', ...])

This will be an error for any batch script that depends on the parsed value in %1

Wrong. See above.
Also note, that normally script will expect ascii args to handle and all non-ascii args going then to the exe, file, mail etc.
So in example like subprocess.call(['send-mail.bat', '-subject', unsafeTitle, '-body', unsafeBody]) the batch-script can take (e. g. compare) 1st and 3rd arguments (so '-subject' and '-body') and pass 2nd and 4th args to sending mail process.

So again, if I'm wrong, please provide an example used array (or tuple).

Look at the output from your version of list2cmdline:
The argument begins with "", which in CMD is an empty quoted string...

Hmm... You are right here, indeed.
I assumed under windows it is impossible to create environment-variables with names like this.
But I was wrong:

>>> os.environ['X'] = 'simply-X'; os.environ['"X"'] = 'quoted-X'; os.environ['""X""'] = 'doublq-X'
>>> subprocess.call(['test-dump.CMD', r'%X%', r'%"X"%', r'"%X%"'])
    `test-dump.exe´ `quoted-X´ `%"X"%´ `"quoted-X"´

But, it looks like it is still possible (note the 2nd arg), but indeed more complex now (should possibly switch to "unpaired-quote"-similar logic/escape).
I'll try to find a solution and provide a modification for this.

WiP.

[eryksun]

My proposal does nothing is this direction if args supplied as string, it works only if you try to use called as "safe" variant, so using cmd as tuple or array.

For simplicity I started with a command line instead of using your version of list2cmdline, but the problem is the same:

>>> print(list2cmdline(['"eggs spam']))
"\"eggs spam"

We can't use VC++ rules to create a command line for a program such as cmd.exe that does not itself use VC++ rules. In this case a VC++ escaped double quote is not seen as escaped by CMD, so the quote matching is wrong.

My recommendation in bpo-33515 was to document that running a batch script (.bat or .cmd) always acts as if shell=True is used. The recommendation with shell=True is to use a command-line string instead of a list. This is almost always required in Unix and always a good idea in Windows given list2cmdline is intended for VC++ applications.

If I have time I'll check your code against some cases using command-line utilities such as reg.exe, setx.exe, sc.exe, schtasks.exe, and icacls.exe. If Python takes this burden on, it has to be robust. A solution that works only in simple cases is just asking for trouble. Again, personally I'd rather put the onus on the code that needs the shell, rather than pack all of the complexity into subprocess. Most people's use cases will actually be simple, and if starts to get mind-bendingly complex that's a sign that maybe you need a different approach.

[sebres]

@eryksun I assume, I understand now what you meant with "eggs spam argument...
So for example:

subprocess.call(['test-dump.cmd', r'"eggs spam'])

will indeed result INSIDE the batch "\"eggs as %1 and spam" as %2.

But this is an INITIAL state, it was ever "escaped" this way, also in original (unmodified) python (as well as in many other languages for windows).
I'm not sure it can be handled properly for both cases (%* as well as %1 %2 %3 ...), but will check this also.
Anyway, thank you for your review!

[sebres]

Just for the better understanding, so still again:

• I'm aware here to don't use shell=True (all what this PR handles, is safe array args execution with shell=False handling), so the case like subprocess.call("cmd /c ...", shell=True) is not affected by the PR in any way;
• in my opinion the safe way to execute an .exe should not be different with the execution of .bat/.cmd (with few exceptions as possible) at least not so vulnerable at is currently the case is.
• this PR made this not more worse as it already was, as regards the matter the weird windows unescape/unquote behavior in case of command-processor (missing function like ArgvToCommandLine, etc).
• unmodified original python-version has above-mentioned unpaired quote split-issue also (r'"eggs spam' results in batch gets "\"eggs as %1 and spam" as %2).
• if the "proper" escape of % will get state as really impossible (so will be defined as not a bug but "expected" feature), so let it be, but at least it belongs definitely into the documentation.
• possibly, one can not completely "fix" all the issues, that MS had introduced at that time (so "broken" by design), but at least so many as possible is desirable.

WiP.

[eryksun]

I'm aware here to don't use shell=True (all what this PR handles, is safe array args execution with shell=False handling), so the case like subprocess.call("cmd /c ...", shell=True) is not affected by the PR in any way;

CreateProcess rewrites the command line to start with "%ComSpec% /c". So running a batch script with shell=False is fundamentally the same as running it with shell=True. As things stand, subprocess doesn't prevent using an args list with shell=True.

The new behavior could be implemented in a private function that's only called if the first item of the list is a .bat or .cmd file. (Note that CreateProcess only tries appending .EXE if no extension is provided, so there's no ambiguity in that case.) I would prefer this, since it decouples the regular implementation from one that's special-cased for CMD.

unmodified original python-version has above-mentioned unpaired quote split-issue also (r'"eggs spam' results in batch gets ""eggs as %1 and spam" as %2).

Do we currently recommended using an args list to run a batch script? We shouldn't. Assuming this PR is accepted for 3.8, it should be documented for older versions that using an args list in Windows is only for programs that follow the VC++ convention, which excludes CMD and batch scripts.

If we support the creation of CMD-compatible command lines, the algorithm should at least guarantee that args[n] in Python maps to %n in CMD. Beyond that, one might consider that it's ultimately the responsibility of a batch script to process its command-line arguments for use by other programs. Since CMD provides no way to escape literal double quote and percent characters in quoted strings, we should document the convention that we follow and document ways to transform strings in CMD to get the proper value (e.g. how to strip outer quotes and do simple string substitutions).

We need to use a pair of quotes for each literal double quote, as noted in this Stack Overflow answer. We can't use VC++ backslash escaping since that convention results in mismatched quotes in CMD. That said, it should be noted that VC++ and CommandLineToArgvW are inconsistent when it comes to parsing such an argument. For example:

>>> s = r'python test.py "eggs ""spam"" eggs"'
>>> subprocess.call(s)
['test.py', 'eggs "spam" eggs']
0
>>> shell32.CommandLineToArgvW(s, ctypes.byref(n))[1:4]
['test.py', 'eggs "spam', 'eggs']

IMO, it's more important that it works correctly in the VC++ runtime. CommandLineToArgvW is used far less frequently. GUI C/C++ apps can use the runtime's __argc and __wargv variables.

if the "proper" escape of % will get state as really impossible (so will be defined as not a bug but "expected" feature), so let it be, but at least it belongs definitely into the documentation.

Yes, it would need to be documented that text between percent characters gets quoted, under the assumption that no existing environment-variable names contain double quotes. For internal consumption, batch scripts would have to handle replacing "%" with % (e.g. for an argument such as "spam"%"eggs"%""). There's no problem if the argument is merely relayed to an application that follows the standard convention.

[sebres]

1. The proper way to escape a %-char (for command processor) is found, WiP (working on the test-cases now).
2. There is no way for proper escape of unpaired quote as regards the interpolations %1..%9 in the batch-file, see [SB-0D-001-win-exec] escape single/unpaired quote in argument sebres/PoC#1
   Therefore in my opinion:
   • either it can be "escaped" as safe as possible (to avoid first level vulnerability) but can be wrong as regards the %1..%9 (so like undefined behavior);
   • or the occurrence of unpaired quote in any argument (batch-file only) should cause an exception, like Invalid argument or similar;

[willingc]

The proper way to escape a %-char (for command processor) is found, WiP (working on the test-cases now).

Labeling as "Do not merge" based on WIP comment.

[sebres]

Because MS refuses the fix of the issue (see microsoft/terminal#250), I would like to know any opinions to above mentioned options:

1. try to "escape" as safe as possible
2. raise exception Unpaired quote found ...

[brettcannon]

Thanks for the PR, but closing as the CLA has not been signed within the last month. If you do decide to sign the CLA we can re-open this PR.

[brettcannon]

@sebres you can email contributors@python.org as outlined at https://www.python.org/psf/contrib/contrib-form/ and see if the form was received.

[brettcannon]

@sebres reopened!

[zware]

Hi @seberg! Unfortunately this did not reach a conclusion before Python 2.7 reached EOL on January 1st. As this does not appear to be a critical security issue, there is almost no chance that it will be accepted in this brief limbo window between end-of-support and the final 2.7.18 release, so I'm going to go ahead and close it.

Do feel free to open a new PR against the master branch if you would still like to see a change in Python 3.

Thanks for your contribution anyway, and I hope your next one is more fruitful!