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

x/build/cmd/release: Go installer appends insecure path 'C:\Go\bin' to Windows system PATH #42070

Closed
menocu opened this issue Oct 19, 2020 · 24 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker Security
Milestone

Comments

@menocu
Copy link

menocu commented Oct 19, 2020

What version of Go are you using (go version)?

Tested against go1.15.2.windows-amd64.msi and go1.14.4.windows-amd64.msi.

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Windows 10, amd64

What did you do?

Installed Go on Windows 10 using an installer from https://golang.org/dl/

What did you expect to see?

The installer appends 'C:\Go\bin' to the system path; This path should not be writable by low privileged users.

What did you see instead?

The permissions on 'C:\Go\bin' are overly permissive, and allow writes from the 'Authenticated Users' group. This means that any authenticated low privilege user on this system can plant a .dll or executable at this location which might later be loaded by an application or user running in a more privileged context, resulting in privilege escalation.

@dmitshur dmitshur changed the title Go installer appends insecure path 'C:\Go\bin' to Windows system PATH x/build/release: Go installer appends insecure path 'C:\Go\bin' to Windows system PATH Oct 19, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Oct 19, 2020
@gopherbot gopherbot added this to the Unreleased milestone Oct 19, 2020
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Security labels Oct 19, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Oct 19, 2020

CC @golang/security, @golang/release.

@FiloSottile
Copy link
Contributor

It would be good to fix this before the next release, checking that it leads to secure permissions both on a fresh install and on an upgrade.

@toothrot
Copy link
Contributor

toothrot commented Nov 5, 2020

@menocu What does removing that permission mean for a multi-user environment? Will that break Go for other users on Windows? Do we know what other languages do on Windows?

@rsc
Copy link
Contributor

rsc commented Nov 5, 2020

It shouldn't break any other users to remove writability. People install Go in a non-world-writable /usr/local/go or similar directories on Unix all the time.

@menocu
Copy link
Author

menocu commented Nov 5, 2020

I agree that removing write permissions for 'Authenticated Users' from C:\Go\bin shouldn't break anything. For what its worth, I looked at a few other languages to see what they do, and it looks like python 2.7 and Ruby both add folders writable by this group to the system path. Python 3 and Haskell do not, by virtue of installing into subdirectories of C:\Program Files\ when installed for all users, from which they inherit secure permissions.

@toothrot
Copy link
Contributor

toothrot commented Nov 5, 2020

@rsc Yep! I am familiar with Unix, just not the common practices for Windows.

@menocu Awesome, thank you. I will make the appropriate change to our MSI configuration, and test it on my Windows PC.

@DemiMarie
Copy link

@menocu I consider this to be a bug in Windows, which allowed C:\ to be writable by authenticated users (!).

@dmitshur dmitshur added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Nov 19, 2020
@zx2c4
Copy link
Contributor

zx2c4 commented Nov 19, 2020

I suppose you could play with the PermissionEx tag in the Wix on the CreateDirectory call with a fairly restrictive SDDL that's inherited by its containers.

https://wixtoolset.org/documentation/manual/v3/xsd/wix/permissionex.html

Alternatively, install Go to Program Files like normal programs? If you're already adding it to PATH, that shouldn't be less convenient. Unless you're concerned about filenames with spaces or something?

@lkarlslund
Copy link

Not to sidetrack this issue, but defaulting to C:\Go for installs does not make sense on the Windows platform IMHO, and I'd not recommend the permission fix for anything but upgrades.

Two different approaches make sense:

  1. System wide install
    Install to C:\Program Files\Go. Required admin rights (elevation). Add Go binary path to system wide PATH variable.

  2. User mode install
    Install to C:\Users<username>\AppData\Local\Go (or elsewhere in the user folder, there is no strict standard). Add Go binary path to users PATH variable.

But binaries in folders directly under C:? No thanks.

One major complaint about the installer is that the previous install location isn't remembered for upgrades. For instance I always opt for approach 1 under Program Files. But every upgrade I have to change the installer path manually, as it defaults to C:\Go. A huge improvement would be to reflect this in a registry key in the Uninstall area.

Also I'm asked if I want to uninstall the previous version, which hmm ... does it make sense ... wouldn't you always answer yes for MSI installs? If I want multiple Go versions on a Windows box, I download the ZIP version and just unzip the versions I want in a folder dedicated to that.

@toothrot
Copy link
Contributor

Based on everyone's recommendations in this thread, I think changing this to ProgramFilesFolder seems like the best decision. Please let me know if you disagree. I'll prepare a CL with that change.

It's my understanding that the MSI should remove the old C:\Go installation path, but I will verify on my Windows test machine.

I am sorry this has taken a while to get to, but I'm glad we're finally fixing it.

@toothrot
Copy link
Contributor

@lkarlslund I think you're totally right, and it looks like we do set HKCU\Software\GoProgrammingLanguage\installLocation, but I don't believe we read it. I will file a separate issue for that.

@DemiMarie
Copy link

@toothrot do you mean C:\Program Files\?

@lkarlslund
Copy link

Well, to play nice an upgrade should use the last installation folder. So if Go was installed to c:\go the last time, and upgrade should just uninstall it from there, and install the new one there. Then there will be no path problems.

Fresh installations should probably go to C:\Program Files\ as suggested.

@lkarlslund
Copy link

@lkarlslund I think you're totally right, and it looks like we do set HKCU\Software\GoProgrammingLanguage\installLocation, but I don't believe we read it. I will file a separate issue for that.

HKCU? Doesn't the installer always elevate to admin? If so you should use HKLM as this is a system wide install.

@toothrot
Copy link
Contributor

@DemiMarie @lkarlslund Sorry for the confusion, the WIX ID for C:\Program Files is called ProgramFilesFolder. I will of course be testing this and ensure it's where we expect.

@toothrot
Copy link
Contributor

toothrot commented Jan 12, 2021

@lkarlslund I agree, but I feel like we should do that separately from this change.

For everyone who is following along, the configuration lives here:
https://github.com/golang/build/blob/789b9298d832791dabdbac856c1d2b52c668c0d9/cmd/release/releaselet.go#L355

This code hasn't been touched in a few years, and I am not a Windows developer. I very much appreciate your advice and feedback.

I'll also ensure it's the correct folder based on the architecture being installed.

@lkarlslund
Copy link

@toothrot awesome, I've been looking for the installer source. Also really cool that you're taking this upon yourself even though you're not usually working with Windows.

I'm not sure what you agree with, and want to handle in a different change?

The installer is meant to be system wide. This implicates that information about this installation is recorded under HKLM, but I guess we agree on this.

To change the install folder, I found something like this:

<!-- Sets the TARGETDIR property to a path based on ProductName property-->
<CustomAction Id="SetTARGETDIR" Property="TARGETDIR" 
Value="[ProgramFilesFolder][Manufacturer]\[ProductName]"
   Execute="firstSequence" />

So I suggest that upon starup the installer checks the registry key to see where Go is installed now, and sets the TARGETDIR to this. If the registry key does not exist, it defaults to "[ProgramFilesFolder]\Go". User can override the path like now while clicking through the wizard.

MSI files can typically be invoked with a parameter to specify where to install:

msiexec /i "msi path" TARGETDIR="C:\myfolder" /qb

Whatever cleverness gets built in should of course respect that.

@toothrot
Copy link
Contributor

I've managed to get a configuration working, and successfully tested the install for windows-amd64 and windows-386 on my windows machine. I'll post a CL shortly.

@lkarlslund Thanks again, your feedback has been very helpful.

My comment about the installer currently using HKCU is based on this XML element:
https://github.com/golang/build/blob/789b9298d832791dabdbac856c1d2b52c668c0d9/cmd/release/releaselet.go#L454-L465

I've filed #43681 to address remembering the path across installs. I'm not sure it will be trivial, given how we always prompt to uninstall first.

@toothrot toothrot changed the title x/build/release: Go installer appends insecure path 'C:\Go\bin' to Windows system PATH x/build/cmd/release: Go installer appends insecure path 'C:\Go\bin' to Windows system PATH Jan 13, 2021
@gopherbot
Copy link

Change https://golang.org/cl/283600 mentions this issue: cmd/release: install Go to appropriate Program Files directory

@gopherbot
Copy link

Change https://golang.org/cl/290309 mentions this issue: content/static/doc: update windows install location doc

gopherbot pushed a commit to golang/website that referenced this issue Feb 8, 2021
Document the new install location changed in CL 283600.

Updates golang/go#42070
Updates golang/go#44134

Change-Id: I987b5219b5877068708344d5809cd5bdd09e1807
Reviewed-on: https://go-review.googlesource.com/c/website/+/290309
Reviewed-by: Alexander Rakoczy <alex@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
@toothrot
Copy link
Contributor

@zx2c4 Nice find! Did you find this by searching/reading, or is this causing an issue? This shouldn't impact installed versions of Go.

@zx2c4
Copy link
Contributor

zx2c4 commented Feb 24, 2021

I was just reading x/build/cmd/release to figure out how to make my own, and came across this.

@gopherbot
Copy link

Change https://golang.org/cl/297131 mentions this issue: utils/pathUtils: add the new installation paths for windows

gopherbot pushed a commit to golang/vscode-go that referenced this issue Mar 1, 2021
For golang/go#42070
Fixes #1252

Change-Id: I70e60a90075c275241100156ea392276da4fc042
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/297131
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
@golang golang locked and limited conversation to collaborators Feb 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows release-blocker Security
Projects
None yet
Development

No branches or pull requests

10 participants