Use full path of cmd.exe

[xiaoyinl]

This patch prevents running cmd.exe from the working directory, which may be attacker-controlled. It fixes CVE-2019-17664.

Related discussion on Ghidra: NationalSecurityAgency/ghidra#107

[xiaoyinl]

Hi @jeff5, does this PR look good to you? Let me know if there’s anything I need to do to get this merged. Thank you.

[jeff5]

This looks right to me, but I shall download and verify. I think resolving the symbol SystemRoot was a good choice in the second commit.

[jeff5]

@xiaoyinl : Was there a reason for not using COMSPEC as in the discussion on NationalSecurityAgency/ghidra#107 ?

On my machine ComSpec is set from the expression %SystemRoot%\system32\cmd.exe so it's the same thing really. However, on a sufficiently old machine it might be C:\Windows\SysWOW64\cmd.exe (or C:\DOS\COMMAND.COM, in ancient history). I don't know whether we would run on such a machine, but in principle the location of cmd.exe can change.

[xiaoyinl]

@jeff5 I didn't use ComSpec because I don't know if this environment variable always exists, and I mistakenly believed ComSpec is always the same as %SystemRoot%\system32\cmd.exe. Now I agree it's better to use ComSpec, since the location of cmd.exe can change.

Do you want to fall back to %SystemRoot%\system32\cmd.exe if ComSpec doesn't exist? Or trust ComSpec always exists, like this: Matcher m = p.matcher(Py.getCommandResult(System.getenv("ComSpec"), "/c", "ver"));

[jeff5]

I think I would assume ComSpec exists.

I have a couple of changes to push onto this PR, so don't change just yet.

[jeff5]

@xiaoyinl : If you now pull from your PR branch you will have the the merge I made from the development tip on which you can try the ComSpec change. If it is not defined, the os version strings just end up empty strings (existing behaviour). I might reconsider that in a separate change.

When you're done, I'll squash and merge.

[xiaoyinl]

@jeff5 I have made the change, and tested it: with this PR applied, sys.getwindowsversion() launches cmd.exe from the System32 directory correctly.

[jeff5]

@xiaoyinl : thanks for your collaboration on this.

As it is relatively minor and follows the proposals on NationalSecurityAgency/ghidra#107, we can manage without the PSF contributor agreement this time.