x/sys: IsWindowsService returns incorrect answer for restricted services

[JeremyRand]

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

$ go version
go version go1.16.1 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
set GO111MODULE=off
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\user\AppData\Local\go-build
set GOENV=C:\Users\user\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\user\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\user\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.16.1
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\user\AppData\Local\Temp\go-build2100961858=/tmp/go-build -gno-record-gcc-switches

What did you do?

I ran a Go program as a restricted Windows service (created like this, with the intent of limiting attack surface), and had the Go program check whether it was running as a service via IsWindowsService().

What did you expect to see?

IsWindowsService() should return true, or at least return a non-nil error if it can't determine whether it's running as a service.

What did you see instead?

IsWindowsService() returned false, and a nil error. Some debugging indicates that the bug is here: https://github.com/golang/sys/blob/7844c3c200c348863f4ff2a6efe6c016b5ba8b57/windows/svc/security.go#L83-L86

When running as a restricted service, err contains an Access is denied error. This code treats any non-nil error at this spot as indicating that the program is not running as a service, i.e. it returns false and a nil error.

At the very least, the Access is denied error should be returned to the caller rather than returning a wrong result with a nil error. However, I also notice that the deprecated IsAnInteractiveSession() handles this case properly, because it does not need any permissions that restricted services do not have. It might be desirable to automatically fall back to the IsAnInteractiveSession() implementation in this case, rather than returning an error.

[networkimprov]

cc @alexbrainman @zx2c4
@gopherbot add OS-Windows

[zx2c4]

This follows more or less the same algorithm as .NET does. Perhaps, though, it's enough to just look at the process parent? Any suggestions on a more robust algorithm? Falling back to a less robust one doesn't seem like a good idea.

[JeremyRand]

Perhaps, though, it's enough to just look at the process parent?

@zx2c4 I just tested ignoring the error, and I found that the windows.OpenProcess call also returns an Access is denied error when running as a restricted service. This is less bad since at least that error is returned to the caller, so the caller is aware that we don't actually know if we're running as a service. But it doesn't help us actually determine whether we're running as a service.

Any suggestions on a more robust algorithm? Falling back to a less robust one doesn't seem like a good idea.

I don't know. One question to ponder is whether it's actually possible for a non-service process to get this error. I've never encountered it outside of the scenario of restricted services, but this is not exactly my area of expertise. If this error does imply that a restricted service is being used, then returning true with a nil error would be sufficient (but someone smarter than me would need to figure out whether this is actually the case). I'm 90% confident that my own use cases would be solved by falling back to the old algorithm when this Access is denied error is encountered, but it doesn't really matter to me whether IsWindowsService does the fallback for me or if it just returns that error to the caller so that my code can decide to fall back to IsAnInteractiveSession.

EDIT: Fixed typo.

[toothrot]

/cc @bradfitz

Could you provide an example of the code that produced the error, preferably as a Go playground link?

[JeremyRand]

Could you provide an example of the code that produced the error, preferably as a Go playground link?

Yes; might take me a day or two.

[the-ress]

@zx2c4

This follows more or less the same algorithm as .NET does. Perhaps, though, it's enough to just look at the process parent? Any suggestions on a more robust algorithm? Falling back to a less robust one doesn't seem like a good idea.

The difference is that IsWindowsService calls OpenProcess,
and .NET uses NtQuerySystemInformation(SystemProcessInformation).

OpenProcess seems to fail with restricted rights while NtQuerySystemInformation doesn't.

Additionally IsWindowsService checks both process name and path, and .NET only checks the process name. (Probably because it can't access the full path with restricted rights.)

Here's the relevant part of .NET source:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs#L279

And here's some Go code I've come across that uses NtQuerySystemInformation(SystemProcessInformation) too (as an example of Go implementation):
https://github.com/microsoft/hcsshim/blob/master/internal/jobcontainers/jobcontainer.go#L573

[zx2c4]

Fantastic observation! Thank you. I'll see if I can bring us closer to the .NET function.

[JeremyRand]

Apologies for failing to post example test case code promptly; things have been super busy here and this bug got back-burnered for me. I'm happy to test a proposed patch to see if it works here; I'll try to post example test case code as soon as I have a few minutes, assuming the bug doesn't get fixed first. Thanks @the-ress for moving this diagnosis forward!

[gopherbot]

Change https://golang.org/cl/372554 mentions this issue: windows/svc: use NtQuerySystemInformation in IsWindowsService

[zx2c4]

@JeremyRand there's a patch. It'd be useful for you to try it out and see if it fixes the problem.

[the-ress]

@zx2c4 thanks so much for the patch! I can confirm the patched version fixes the issue for me.