os/exec: environForSysProcAttr is never called as sysattr.Env is never nil #35314

KatelynHaworth · 2019-11-02T10:03:29Z

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.

The text was updated successfully, but these errors were encountered:

KatelynHaworth · 2019-11-02T15:30:37Z

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.

KatelynHaworth · 2019-11-02T16:53:17Z

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]

KatelynHaworth · 2019-11-02T16:54:17Z

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

networkimprov · 2019-11-02T18:46:56Z

cc @zx2c4 @alexbrainman
@gopherbot add OS-Windows

ianlancetaylor · 2019-11-05T03:40:23Z

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.

KatelynHaworth · 2019-11-05T07:16:28Z

@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 · 2019-11-06T09:14:23Z

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 · 2019-11-06T13:54:32Z

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 · 2019-11-07T08:39:16Z

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

KatelynHaworth · 2019-11-07T09:16:58Z

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 · 2019-11-07T09:49:26Z

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

KatelynHaworth · 2019-11-07T09:57:30Z

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 · 2019-11-07T10:24:09Z

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

petemoore · 2020-02-10T11:49:42Z

@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!

KatelynHaworth · 2020-02-24T13:37:03Z

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.

gopherbot · 2020-02-24T14:32:04Z

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

KatelynHaworth · 2020-02-25T02:10:59Z

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

ianlancetaylor · 2020-02-25T02:17:14Z

I have no idea.

@gopherbot Please open backport issues

This may be security related on Windows systems.

gopherbot · 2020-02-25T02:18:50Z

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.

KatelynHaworth · 2020-02-25T02:20:06Z

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 · 2020-02-25T02:29:15Z

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

KatelynHaworth · 2020-02-26T01:50:19Z

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

toothrot · 2020-02-26T16:01:08Z

@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 · 2020-03-13T14:13:29Z

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.

andybons · 2020-03-26T21:29:04Z

@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!

KatelynHaworth · 2020-03-29T06:00:07Z

@andybons cherrypick CLs have been made for each branch

andybons · 2020-03-29T16:27:32Z

Great. Thank you!

gopherbot · 2020-03-29T17:16:16Z

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

…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

…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 · 2020-03-29T17:17:01Z

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

gopherbot added the OS-Windows label Nov 2, 2019

ianlancetaylor added the NeedsFix label Nov 5, 2019

ianlancetaylor added this to the Go1.14 milestone Nov 5, 2019

ianlancetaylor modified the milestones: Go1.14, Go1.15 Dec 5, 2019

KatelynHaworth mentioned this issue Feb 24, 2020

os/exec: use environment variables for user token when present #37402

Closed

gopherbot closed this as completed in e0c3ded Feb 25, 2020

This was referenced Feb 25, 2020

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil [1.12 backport] #37432

Closed

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil [1.13 backport] #37433

Closed

ianlancetaylor removed this from the Go1.15 milestone Feb 25, 2020

ianlancetaylor added this to the Go1.14 milestone Feb 25, 2020

ianlancetaylor reopened this Feb 25, 2020

toothrot modified the milestones: Go1.14, Go1.15 Feb 25, 2020

toothrot mentioned this issue Feb 26, 2020

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil [1.14 backport] #37471

Closed

dmitshur closed this as completed Mar 13, 2020

golang locked and limited conversation to collaborators Mar 29, 2021

gopherbot added the FrozenDueToAge label Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil #35314

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil #35314

KatelynHaworth commented Nov 2, 2019

KatelynHaworth commented Nov 2, 2019

KatelynHaworth commented Nov 2, 2019

KatelynHaworth commented Nov 2, 2019

networkimprov commented Nov 2, 2019

ianlancetaylor commented Nov 5, 2019

KatelynHaworth commented Nov 5, 2019

alexbrainman commented Nov 6, 2019

ianlancetaylor commented Nov 6, 2019

alexbrainman commented Nov 7, 2019

KatelynHaworth commented Nov 7, 2019 •

edited

Loading

alexbrainman commented Nov 7, 2019

KatelynHaworth commented Nov 7, 2019

alexbrainman commented Nov 7, 2019

petemoore commented Feb 10, 2020

KatelynHaworth commented Feb 24, 2020 •

edited

Loading

gopherbot commented Feb 24, 2020

KatelynHaworth commented Feb 25, 2020

ianlancetaylor commented Feb 25, 2020

gopherbot commented Feb 25, 2020

KatelynHaworth commented Feb 25, 2020

ianlancetaylor commented Feb 25, 2020

KatelynHaworth commented Feb 26, 2020

toothrot commented Feb 26, 2020

dmitshur commented Mar 13, 2020

andybons commented Mar 26, 2020

KatelynHaworth commented Mar 29, 2020

andybons commented Mar 29, 2020

gopherbot commented Mar 29, 2020

gopherbot commented Mar 29, 2020

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil #35314

os/exec: environForSysProcAttr is never called as sysattr.Env is never nil #35314

Comments

KatelynHaworth commented Nov 2, 2019

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

Other comments:

KatelynHaworth commented Nov 2, 2019

KatelynHaworth commented Nov 2, 2019

KatelynHaworth commented Nov 2, 2019

networkimprov commented Nov 2, 2019

ianlancetaylor commented Nov 5, 2019

KatelynHaworth commented Nov 5, 2019

alexbrainman commented Nov 6, 2019

ianlancetaylor commented Nov 6, 2019

alexbrainman commented Nov 7, 2019

KatelynHaworth commented Nov 7, 2019 • edited Loading

alexbrainman commented Nov 7, 2019

KatelynHaworth commented Nov 7, 2019

alexbrainman commented Nov 7, 2019

petemoore commented Feb 10, 2020

KatelynHaworth commented Feb 24, 2020 • edited Loading

gopherbot commented Feb 24, 2020

KatelynHaworth commented Feb 25, 2020

ianlancetaylor commented Feb 25, 2020

gopherbot commented Feb 25, 2020

KatelynHaworth commented Feb 25, 2020

ianlancetaylor commented Feb 25, 2020

KatelynHaworth commented Feb 26, 2020

toothrot commented Feb 26, 2020

dmitshur commented Mar 13, 2020

andybons commented Mar 26, 2020

KatelynHaworth commented Mar 29, 2020

andybons commented Mar 29, 2020

gopherbot commented Mar 29, 2020

gopherbot commented Mar 29, 2020

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

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

KatelynHaworth commented Nov 7, 2019 •

edited

Loading

KatelynHaworth commented Feb 24, 2020 •

edited

Loading