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

os/exec: on Windows use NeedCurrentDirectoryForExePathW for LookPath behavior #43947

Open
dolmen opened this issue Jan 27, 2021 · 8 comments
Open

Comments

@dolmen
Copy link

@dolmen dolmen commented Jan 27, 2021

In os/exec.LookPath use Windows function NeedCurrentDirectoryForExePathW to check if the environment is hardened to protect against resolution of command path in the current directory.

Note that this proposal is different from just looking at the existence of a NoDefaultCurrentDirectoryInExePath environment variable because the function doesn't just look in the process environment variables but might also look in registry settings.

This is related to:

  • #43724: return error when PATH lookup would use current directory
  • #42950: use LookPathAbs by default

Credit goes to @KalleOlaviNiemitalo for his/her comment in #38736.

@gopherbot gopherbot added this to the Proposal milestone Jan 27, 2021
@dolmen
Copy link
Author

@dolmen dolmen commented Jan 27, 2021

Side note: Windows has multiple ways of resolving application paths. The one that os/exec had implemented before 1.15.7 is the one implemented by cmd.exe (and most CLI tools), not the one recommended for GUI tools which involves more than PATH. Just to say that os/exec has never been the only right way to launch commands on Windows.

Loading

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jan 27, 2021
@rsc rsc moved this from Incoming to Active in Proposals Feb 17, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 17, 2021

Sounds reasonable.
/cc @alexbrainman @zx2c4

Loading

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 17, 2021

Huh, interesting rabbit hole here. One thing I noticed is that CreateProcess itself will handle path resolution if you pass it a non-absolute executable name as argv[0] and NULL as the actual executable path name:

image
from https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa

As an aside from the specific proposal here, this makes me wonder if instead of having exec.Command use LookPath, it should instead use that behavior of CreateProcess, and then we always do what windows does.


With regards to NeedCurrentDirectoryForExePath, here's what kernelbase.dll is doing on Windows 10, translated from the x86 into Go:

if strings.Index(exePath, `\`) != -1 { return true }
_, ok := os.Environ()["NoDefaultCurrentDirectoryInExePath"]
return !ok

So when you write:

Note that this proposal is different from just looking at the existence of a NoDefaultCurrentDirectoryInExePath environment variable because the function doesn't just look in the process environment variables but might also look in registry settings.

I'm not sure that this is actually true. Are there other versions of Windows you think I should look at?

Loading

@antichris
Copy link

@antichris antichris commented Feb 19, 2021

use [the] behavior of CreateProcess and ... always do what windows does

Is it just me or does this sound really reckless? This entire proposal arose exactly from the necessity to mitigate the risk of malicious code execution that may lurk in a current working directory (#38736, #42420, #42950 and #43724). The fact that Microsoft did stupid sh...tuff (and still does, and have been doing for ages), really does not mean we or anyone else should ape them in that, ever (again). Convenience should not take precedence over security.

Loading

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Feb 19, 2021

I hadn't seen #43724. I was thinking that Go wouldn't want a behavior change, and would want to keep os/exec using the defaults of whatever operating system and environment it's using. But it looks like #43724 changes that. So this issue here, I suppose, becomes one of "how can we tell when Windows is going to do something bad?" And I take it that @dolmen's suggestion is to use NeedCurrentDirectoryForExePath() instead of checking the NoDefaultCurrentDirectoryInExePath environment variable. But in reversing the former, I saw that it's basically the same as the latter.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

In summary:

Except for the error in #43724, LookPath aims to match Windows cmd.exe.
If Windows has been configured to not add "." to the PATH implicitly,
then this proposal is for LookPath to respect that configuration setting
(which turns out to be an environment variable).

This seems unobjectionable. There was some discussion above arising from confusion about what exactly is being proposed, but that seems to have been resolved.

Does anyone object to this proposal?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Loading

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 24, 2021
@rsc rsc changed the title proposal: os/exec: on Windows use NeedCurrentDirectoryForExePathW for LookPath behavior os/exec: on Windows use NeedCurrentDirectoryForExePathW for LookPath behavior Mar 24, 2021
@rsc rsc removed this from the Proposal milestone Mar 24, 2021
@rsc rsc added this to the Backlog milestone Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants