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

Closed
dolmen opened this issue Jan 27, 2021 · 14 comments
Closed
Assignees
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Milestone

Comments

@dolmen
Copy link
Contributor

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:

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

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

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.

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

rsc commented Feb 17, 2021

Sounds reasonable.
/cc @alexbrainman @zx2c4

@zx2c4
Copy link
Contributor

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?

@antichris
Copy link

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.

@zx2c4
Copy link
Contributor

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.

@rsc
Copy link
Contributor

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?

@rsc
Copy link
Contributor

rsc commented Mar 10, 2021

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

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

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

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) 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 modified the milestones: Proposal, Backlog Mar 24, 2021
@rsc rsc self-assigned this Jan 27, 2022
@rsc rsc modified the milestones: Backlog, Go1.19 Jan 27, 2022
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 27, 2022
@gopherbot
Copy link

Change https://golang.org/cl/381374 mentions this issue: os/exec: return error when PATH lookup would use current directory

@dolmen
Copy link
Contributor Author

dolmen commented Feb 2, 2022

@rsc wrote:

then this proposal is for LookPath to respect that configuration setting (which turns out to be an environment variable).

This is not what this proposal is about because the case where the configuration setting is in the registry and not in the environment is not handled.

This proposal is about using NeedCurrentDirectoryForExePathW to ensure that however the system setting is defined it is respected.

So CL381374 is not a correct implementation of this proposal because it only uses the environment variable.

@zikaeroh
Copy link
Contributor

zikaeroh commented Apr 29, 2022

The above CL broke x/sys/execabs (and its users in the wild, who may or may not ever update), since it expects to find a lookPathErr field, but the CL removes it in favor of an exported Err.

@bcmills bcmills reopened this Apr 29, 2022
@gopherbot
Copy link

Change https://go.dev/cl/403058 mentions this issue: Revert "os/exec: return error when PATH lookup would use current directory"

gopherbot pushed a commit that referenced this issue Apr 29, 2022
…ctory"

This reverts CL 381374.

Reason for revert: broke tests for x/sys/execabs.

Updates #43724.
Updates #43947.

Change-Id: I9eb3adb5728dead66dbd20f6afe1e7a77e2a26f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/403058
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/403694 mentions this issue: os/exec: in Command, update cmd.Path even if LookPath returns an error

gopherbot pushed a commit that referenced this issue May 3, 2022
Fixes #52666.
Updates #43724.
Updates #43947.

Change-Id: I72cb585036b7e93cd7adbff318b400586ea97bd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/403694
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
@ianlancetaylor
Copy link
Contributor

This appears to be done. Please comment if you disagree.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
Development

No branches or pull requests

8 participants