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: os.Environ() is not appropriate default for passing to syscall.StartProcess if attr.Sys.Token != 0 on Windows #29534

Closed
petemoore opened this issue Jan 3, 2019 · 5 comments

Comments

@petemoore
Copy link

@petemoore petemoore commented Jan 3, 2019

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

I use a Mac, but cross-compile to Windows. This is my darwin env, but it isn't really relevant.

go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

Again, this isn't really relevant for this issue - but here is my env in any case.

go envOutput
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/pmoore/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/pmoore/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
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/v9/mll6p_rj5h94dt_m5m8j0f9c0000gn/T/go-build797508603=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I looked at https://github.com/golang/go/blob/go1.11.4/src/os/exec_posix.go#L39-L41

What did you expect to see?

I expected that on Windows os.Environ() would not be used as default value if env == nil and attr.Sys.Token != 0.

If attr.Sys.Token is nonzero, it indicates that the process is to be executed by a different user than the parent process was executed by. It is dangerous to use the environment of one user for a process running as a different user, since many environment variables are required by Windows subsystems, and are user-specific. For example HOMEPATH, LOCALAPPDATA, USERNAME are all user-specific, and often required by programs. A much better behaviour if no env is specified but an auth token is, would be to call CreateEnvironmentBlock with the given auth token, to create a new environment block that is appropriate for that user.

What did you see instead?

os.Environ() is used, even though the env vars (such as HOMEPATH, LOCALAPPDATA, USERNAME, ....) will undoubtedly cause problems for the process being created.

Proposed solution

I believe the best approach here is to introduce a behaviour change, and update both the docs and the implementation.

Doc update

State that if nil is passed for the environment, that the parent process environment will be used if no authentication token is specified ((exec.Command).SysProcAttr.Token == 0) otherwise the default environment for the given authentication token will be used.

Code change

If running on Windows, and (exec.Command).SysProcAttr.Token != 0 and env == nil, a call is made to CreateEnvironmentBlock, passing in attr.Sys.Token for HANDLE hToken, to retrieve a usable environment block. Note, a cleanup call to DestroyEnvironmentBlock is also needed after copying the environment block.

I'm happy to make a PR for this, if the proposed approach is deemed acceptable. The syscalls look something like this. When implementing the syscalls, I would of course plug into the existing code generation in the standard library to do this.

@katiehockman katiehockman changed the title os.startProcess: os.Environ() is not appropriate default for passing to syscall.StartProcess if attr.Sys.Token != 0 on Windows os/exec: os.Environ() is not appropriate default for passing to syscall.StartProcess if attr.Sys.Token != 0 on Windows Jan 3, 2019
@katiehockman katiehockman added this to the Unplanned milestone Jan 4, 2019
@katiehockman

This comment has been minimized.

Copy link
Member

@katiehockman katiehockman commented Jan 4, 2019

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 4, 2019

I don't think we want to complicate the common case or the docs.

For the few users doing something low-level, I think it's okay to write a few more lines and explicitly specify the environment.

I'll let @alexbrainman and @ianlancetaylor chime in though.

@bradfitz bradfitz added the OS-Windows label Jan 4, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 4, 2019

I'm inclined to agree. I think we should just mention this on the docs for SysProcAttr.Token.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 5, 2019

For the few users doing something low-level, I think it's okay to write a few more lines and explicitly specify the environment.

I agree. SysProcAttr.Token was introduced to allow people shoot themselves in the foot. They should know what they are doing.

Alex

@petemoore

This comment has been minimized.

Copy link
Author

@petemoore petemoore commented Feb 10, 2020

For the few users doing something low-level, I think it's okay to write a few more lines and explicitly specify the environment.

I agree. SysProcAttr.Token was introduced to allow people shoot themselves in the foot. They should know what they are doing.

Alex

It looks like this issue was reported as CVE-2019-11888 four months later, duped as #32000 and fixed in https://go-review.googlesource.com/c/go/+/176619 (as per the fix suggested above, minus the doc update), so closing here.

@petemoore petemoore closed this Feb 10, 2020
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.

None yet
5 participants
You can’t perform that action at this time.