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: environForSysProcAttr is never called as sysattr.Env is never nil #35314

Closed
LiamHaworth opened this issue Nov 2, 2019 · 29 comments
Closed
Milestone

Comments

@LiamHaworth
Copy link
Contributor

@LiamHaworth LiamHaworth commented Nov 2, 2019

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

$ go version
go version go1.13.4 darwin/amd64

Note: Even though I am developing on darwin, the code is targeted to windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/liam/git/go_path/bin"
GOCACHE="/Users/liam/Library/Caches/go-build"
GOENV="/Users/liam/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/liam/git/go_path"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.4/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/jt/myl07nr11fb6_27x6k61xlzm0000gn/T/go-build237806154=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Launched a process with SysProcAttr set that include a Token for the current active user.

What did you expect to see?

The process should have access to environment variables appropriate to the user it was launched for.

What did you see instead?

The launched process had environment variables originating from the service that started it, rather than those for the user it is running as.

Other comments:

When launching a process on Windows with a SysProcAttr that includes a token, Go doesn't load the environment variables for that token, instead it uses the environment variables for the current process.

I can see in os/exec_posix.go (on line 40) it does a check to see if the Env is nil, if it is it will call environForSysProcAttr which would load the appropriate environment variables if a token is supplied in the process attributes.

But in the calling method (os/exec/exec.go:416) the Env variable it set by addCriticalEnv(dedupEnv(c.envv())), of which c.envv() will always return a value based on it's logic:

func (c *Cmd) envv() []string {
	if c.Env != nil {
		return c.Env
	}
	return os.Environ()
}

Ideally, this can probably be removed as environForSysProcAttr for windows and other platforms always fallback to os.Environ() if there is nothing to be done based on the process attributes.

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Nov 2, 2019

The changes that introduced environForSysProcAttr were in #32000 but it doesn't look like there was any testing. I will put together some example code that replicates the issue.

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Nov 2, 2019

Made some code to reproduce the issue along with a hacky solution to it plus a README on how to run it: https://gist.github.com/LiamHaworth/57bd4bc16d9f82f7cecba945095aa83f

But the results of it are:

Without Env Set: Environment variables:  [ALLUSERSPROFILE=C:\ProgramData APPDATA=C:\Windows\system32\config\systemprofile\AppData\Roaming CommonProgramFiles=C:\Program Files\Common Files CommonProgramFiles(x86)=C:\Program Files (x86)\Common Files CommonProgramW6432=C:\Program Files\Common Files COMPUTERNAME=DESKTOP-6DAJDGQ ComSpec=C:\Windows\system32\cmd.exe DriverData=C:\Windows\System32\Drivers\DriverData LOCALAPPDATA=C:\Windows\system32\config\systemprofile\AppData\Local NUMBER_OF_PROCESSORS=4 OS=Windows_NT Path=C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Windows\system32\config\systemprofile\AppData\Local\Microsoft\WindowsApps PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC PROCESSOR_ARCHITECTURE=AMD64 PROCESSOR_IDENTIFIER=Intel64 Family 6 Model 158 Stepping 9, GenuineIntel PROCESSOR_LEVEL=6 PROCESSOR_REVISION=9e09 ProgramData=C:\ProgramData ProgramFiles=C:\Program Files ProgramFiles(x86)=C:\Program Files (x86) ProgramW6432=C:\Program Files PSModulePath=C:\Program Files\WindowsPowerShell\Modules;C:\Windows\system32\WindowsPowerShell\v1.0\Modules PUBLIC=C:\Users\Public SystemDrive=C: SystemRoot=C:\Windows TEMP=C:\Windows\TEMP TMP=C:\Windows\TEMP USERDOMAIN=WORKGROUP USERNAME=DESKTOP-6DAJDGQ$ USERPROFILE=C:\Windows\system32\config\systemprofile windir=C:\Windows]

With Env Set: Environment variables:  [ALLUSERSPROFILE=C:\ProgramData APPDATA=C:\Users\QA\AppData\Roaming CommonProgramFiles=C:\Program Files\Common Files CommonProgramFiles(x86)=C:\Program Files (x86)\Common Files CommonProgramW6432=C:\Program Files\Common Files COMPUTERNAME=DESKTOP-6DAJDGQ ComSpec=C:\Windows\system32\cmd.exe DriverData=C:\Windows\System32\Drivers\DriverData HOMEDRIVE=C: HOMEPATH=\Users\QA LOCALAPPDATA=C:\Users\QA\AppData\Local LOGONSERVER=\\DESKTOP-6DAJDGQ NUMBER_OF_PROCESSORS=4 OS=Windows_NT Path=C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\QA\AppData\Local\Microsoft\WindowsApps PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC PROCESSOR_ARCHITECTURE=AMD64 PROCESSOR_IDENTIFIER=Intel64 Family 6 Model 158 Stepping 9, GenuineIntel PROCESSOR_LEVEL=6 PROCESSOR_REVISION=9e09 ProgramData=C:\ProgramData ProgramFiles=C:\Program Files ProgramFiles(x86)=C:\Program Files (x86) ProgramW6432=C:\Program Files PSModulePath=%ProgramFiles%\WindowsPowerShell\Modules;C:\Windows\system32\WindowsPowerShell\v1.0\Modules PUBLIC=C:\Users\Public SystemDrive=C: SystemRoot=C:\Windows TEMP=C:\Users\QA\AppData\Local\Temp TMP=C:\Users\QA\AppData\Local\Temp USERDOMAIN=DESKTOP-6DAJDGQ USERDOMAIN_ROAMINGPROFILE=DESKTOP-6DAJDGQ USERNAME=QA USERPROFILE=C:\Users\QA windir=C:\Windows]
@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Nov 2, 2019

Also happy to take on responsibility to submit a fix, just want suggestions on the best way to do so

@networkimprov
Copy link

@networkimprov networkimprov commented Nov 2, 2019

cc @zx2c4 @alexbrainman
@gopherbot add OS-Windows

@gopherbot gopherbot added the OS-Windows label Nov 2, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Nov 5, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 5, 2019

My first thought is that we should move the Windows version of environForSysProcAttr to internal/syscall/windows and call it from both os and os/exec to initialize the environment.

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Nov 5, 2019

@ianlancetaylor Based on your suggestion, Cmd.envv() would be updated to look like this (for example):

func (c *Cmd) envv() []string {
	if c.Env != nil {
		return c.Env
	}
	return windows.EnvironForSysProcAttr(c.SysProcAttr)
}
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 6, 2019

But in the calling method (os/exec/exec.go:416) the Env variable it set by

Yes. We have forgotten about os/exec package.

it doesn't look like there was any testing.

Do you have suggestion for a test?

Also happy to take on responsibility to submit a fix, just want suggestions on the best way to do so

Sure, that would be nice. Here https://golang.org/doc/contribute.html is how to contribute.

we should move the Windows version of environForSysProcAttr to internal/syscall/windows

@ianlancetaylor internal/syscall/windows package only contains windows specific code at this moment. As I understand your proposal, there will be all OSes version of environForSysProcAttr there. Should we add this function to internal/syscall instead?

Thank you.

Alex

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

I was thinking that only windows specific code would call environForSysProcAttr. We can add to internal/syscall if that turns out to be cleaner.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 7, 2019

I was thinking that only windows specific code would call environForSysProcAttr. We can add to internal/syscall if that turns out to be cleaner.

Sure that is fine with me too. Except @LiamHaworth code fragment from #35314 (comment) cannot refer to internal/syscall/windows package directly. But I am fine either way, whatever is simpler.

@LiamHaworth let me know, if you are still want to send a fix for this.

Thank you.

Alex

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Nov 7, 2019

Do you have suggestion for a test?

Right now, at least from my perspective, there isn't really an easy way to automate testing of this code but I did provide a gist in a previous comment with code that replicates the issue.

Sure that is fine with me too. Except @LiamHaworth code fragment from #35314 (comment) cannot refer to internal/syscall/windows package directly

What is the reasoning behind this? From what I understood of the code base, the internal package can be accessed by only packages in the standard library (e.g. os/exec)

@LiamHaworth let me know, if you are still want to send a fix for this.

I'll give it a shot, been wanting to help contribute so this is my chance I suppose

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 7, 2019

Right now, at least from my perspective, there isn't really an easy way to automate testing of this code but I did provide a gist in a previous comment with code that replicates the issue.

Your Gist uses Windows service. And not a real service. Don't worry about the test, if you cannot come up with something simple.

What is the reasoning behind this? From what I understood of the code base, the internal package can be accessed by only packages in the standard library (e.g. os/exec)

The problem with your code fragment is that it uses internal/syscall/windows package, and the package contains no non-windows code. So, if you add EnvironForSysProcAttr there, you would have to implement 2 different versions: windows and non-windows. And that would be unusual to have non-windows code in this package.

The windows.EnvironForSysProcAttr name should give you a clue. This name starts with windows, but the code is for non-windows. I think it would be confusing for future users.

I think the choice is either to implement windows specific internal/syscall/windows.EnvironForSysProcAttr and use it in both os and os/exec. Or create all platform supported version in internal/syscall.EnvironForSysProcAttr and use it in os and os/exec.

Alex

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Nov 7, 2019

Ah, yup, I understand what you are getting at now. Yeah, so the original code for environForSysProcAttr in os (from #32000) has an implementation for windows and then a default that simply wraps Environ().

So with that, both versions of environForSysProcAttr could be moved to internal/syscall, exposed and then used in *Cmd.envv().

I'll start writing a patch, but I'll have to wait till tomorrow (East Aus Time) for my employer to sign the CLA.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Nov 7, 2019

I'll start writing a patch, but I'll have to wait till tomorrow (East Aus Time) for my employer to sign the CLA.

No worries. When you are ready.

Alex

@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Go1.15 Dec 5, 2019
@petemoore
Copy link

@petemoore petemoore commented Feb 10, 2020

@LiamHaworth Thanks for all the work you have done on this issue so far. Are you still intending to provide a fix?

If I understand correctly, go is still vulnerable to https://nvd.nist.gov/vuln/detail/CVE-2019-11888 while a fix for this issue is outstanding, so I am happy to assist with a fix if you are busy with other things.

Thanks!

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Feb 24, 2020

Hey @petemoore , thank you for poking me on this. Got so stuck in work and holidays and forgot about this.

If I understand correctly, go is still vulnerable to https://nvd.nist.gov/vuln/detail/CVE-2019-11888 while a fix for this issue is outstanding

You are correct. I've begun work on the changes and am currently testing the changes to confirm they work and then will submit an initial PR for review as I expected there will be a few inputs on styling/layout.

LiamHaworth added a commit to LiamHaworth/go that referenced this issue Feb 24, 2020
Builds upon the changes from golang#32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.

But due to the logic of `os/exec`, the `Env` field of a process was
always non-nil when it reached that change.

This change moves the logic up to `os/exec`, specifically when
`os.ProcAttr` is being built for the `os.StartProcess` call, this
ensures that if a user token has been supplied and no `Env` slice has
been provided on the command it will be sourced from the user's
environment.

If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
`syscall.Environ()`.

Fixes golang#35314
LiamHaworth added a commit to LiamHaworth/go that referenced this issue Feb 24, 2020
Builds upon the changes from golang#32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.

But due to the logic of `os/exec`, the `Env` field of a process was
always non-nil when it reached that change.

This change moves the logic up to `os/exec`, specifically when
`os.ProcAttr` is being built for the `os.StartProcess` call, this
ensures that if a user token has been supplied and no `Env` slice has
been provided on the command it will be sourced from the user's
environment.

If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
`syscall.Environ()`.

Fixes golang#35314
LiamHaworth added a commit to LiamHaworth/go that referenced this issue Feb 24, 2020
Builds upon the changes from golang#32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.

But due to the logic of os/exec, the Env field of a process was
always non-nil when it reached that change.

This change moves the logic up to os/exec, specifically when
os.ProcAttr is being built for the os.StartProcess call, this
ensures that if a user token has been supplied and no Env slice has
been provided on the command it will be sourced from the user's
environment.

If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
syscall.Environ().

Fixes golang#35314
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 24, 2020

Change https://golang.org/cl/220587 mentions this issue: os/exec: use environment variables for user token when present

LiamHaworth added a commit to LiamHaworth/go that referenced this issue Feb 25, 2020
Builds upon the changes from golang#32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.

But due to the logic of os/exec, the Env field of a process was
always non-nil when it reached that change.

This change moves the logic up to os/exec, specifically when
os.ProcAttr is being built for the os.StartProcess call, this
ensures that if a user token has been supplied and no Env slice has
been provided on the command it will be sourced from the user's
environment.

If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
syscall.Environ().

Fixes golang#35314
@gopherbot gopherbot closed this in e0c3ded Feb 25, 2020
@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Feb 25, 2020

@ianlancetaylor Should these changes be backported to 1.13 and/or 1.14 since it a security related change?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 25, 2020

I have no idea.

@gopherbot Please open backport issues

This may be security related on Windows systems.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 25, 2020

Backport issue(s) opened: #37432 (for 1.12), #37433 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Feb 25, 2020

The original change set (#32000) was a fix for CVE-2019-11888 as would be these changes as the original change set didn't cover a process started with exec.*Cmd.Start() thus without these changes 1.13/1.14 are still open to CVE-2019-11888

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Go1.14 Feb 25, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 25, 2020

Gopherbot opened for 1.12 and 1.13, reopening this issue for 1.14.

@toothrot toothrot modified the milestones: Go1.14, Go1.15 Feb 25, 2020
@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Feb 26, 2020

@toothrot Would the milestone 1.14.1 not be more appropriate as this is a security fix?

@toothrot
Copy link
Contributor

@toothrot toothrot commented Feb 26, 2020

@LiamHaworth I believe this issue should track this fix as being in 1.15.

Now that 1.14 is released, a new backport issue will track 1.14.x: #37471.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 13, 2020

Gopherbot opened for 1.12 and 1.13, reopening this issue for 1.14.

@toothrot has opened a new backport issue for 1.14 (#37471), and this issue is already fixed on master via CL 220587. So in order to reflect the current state more accurately, I'll close this issue.

@dmitshur dmitshur closed this Mar 13, 2020
@andybons
Copy link
Member

@andybons andybons commented Mar 26, 2020

@LiamHaworth I approved cherry picks for 1.13 and 1.14 (#37433 and #37471 respectively).

Can you create cherrypick CLs for each branch, please? You can use this guide if you’re not sure how to do so.

Thanks!

@LiamHaworth
Copy link
Contributor Author

@LiamHaworth LiamHaworth commented Mar 29, 2020

@andybons cherrypick CLs have been made for each branch

@andybons
Copy link
Member

@andybons andybons commented Mar 29, 2020

Great. Thank you!

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 29, 2020

Change https://golang.org/cl/226280 mentions this issue: [release-branch.go1.13] os/exec: use environment variables for user token when present

gopherbot pushed a commit that referenced this issue Mar 29, 2020
…oken when present

Builds upon the changes from #32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.

But due to the logic of os/exec, the Env field of a process was
always non-nil when it reached that change.

This change moves the logic up to os/exec, specifically when
os.ProcAttr is being built for the os.StartProcess call, this
ensures that if a user token has been supplied and no Env slice has
been provided on the command it will be sourced from the user's
environment.

If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
syscall.Environ().

For #35314
Fixes #37471

Change-Id: I4c1722e90b91945eb6980d5c5928183269b50487
GitHub-Last-Rev: 32216b7
GitHub-Pull-Request: #37402
Reviewed-on: https://go-review.googlesource.com/c/go/+/220587
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit e0c3ded)
Reviewed-on: https://go-review.googlesource.com/c/go/+/226281
gopherbot pushed a commit that referenced this issue Mar 29, 2020
…oken when present

Builds upon the changes from #32000 which supported sourcing environment
variables for a new process from the environment of a Windows user token
when supplied.

But due to the logic of os/exec, the Env field of a process was
always non-nil when it reached that change.

This change moves the logic up to os/exec, specifically when
os.ProcAttr is being built for the os.StartProcess call, this
ensures that if a user token has been supplied and no Env slice has
been provided on the command it will be sourced from the user's
environment.

If no token is provided, or the program is compiled for any other
platform than Windows, the default environment will be sourced from
syscall.Environ().

For #35314
Fixes #37433

Change-Id: I4c1722e90b91945eb6980d5c5928183269b50487
GitHub-Last-Rev: 32216b7
GitHub-Pull-Request: #37402
Reviewed-on: https://go-review.googlesource.com/c/go/+/220587
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/226280
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 29, 2020

Change https://golang.org/cl/226281 mentions this issue: [release-branch.go1.14] os/exec: use environment variables for user token when present

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

9 participants
You can’t perform that action at this time.