Repository navigation
x/sys: IsWindowsService returns incorrect answer for restricted services #44921
Description
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.
Activity
networkimprov commentedon Mar 11, 2021
cc @alexbrainman @zx2c4
@gopherbot add OS-Windows
zx2c4 commentedon Mar 11, 2021
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 commentedon Mar 11, 2021
@zx2c4 I just tested ignoring the error, and I found that the
windows.OpenProcesscall also returns anAccess is deniederror 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.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 deniederror is encountered, but it doesn't really matter to me whetherIsWindowsServicedoes the fallback for me or if it just returns that error to the caller so that my code can decide to fall back toIsAnInteractiveSession.EDIT: Fixed typo.
toothrot commentedon Mar 11, 2021
/cc @bradfitz
Could you provide an example of the code that produced the error, preferably as a Go playground link?
JeremyRand commentedon Mar 11, 2021
Yes; might take me a day or two.
the-ress commentedon Dec 15, 2021
@zx2c4
The difference is that
IsWindowsServicecalls OpenProcess,and .NET uses NtQuerySystemInformation(SystemProcessInformation).
OpenProcessseems to fail with restricted rights whileNtQuerySystemInformationdoesn't.Additionally
IsWindowsServicechecks 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 commentedon Dec 15, 2021
Fantastic observation! Thank you. I'll see if I can bring us closer to the .NET function.
JeremyRand commentedon Dec 15, 2021
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 commentedon Dec 15, 2021
Change https://golang.org/cl/372554 mentions this issue:
windows/svc: use NtQuerySystemInformation in IsWindowsServicezx2c4 commentedon Dec 15, 2021
@JeremyRand there's a patch. It'd be useful for you to try it out and see if it fixes the problem.
the-ress commentedon Dec 16, 2021
@zx2c4 thanks so much for the patch! I can confirm the patched version fixes the issue for me.
Replace deprecated IsAnInteractiveSession() call
Replace deprecated IsAnInteractiveSession() call (#344)
windows/svc: use NtQuerySystemInformation in IsWindowsService