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

x/sys/windows/svc/mgr: Allow clients to request read-only access to the Service Control Manager and to individual services #51465

Open
dblohm7 opened this issue Mar 3, 2022 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@dblohm7
Copy link

dblohm7 commented Mar 3, 2022

Mgr.Connect always requests SC_MANAGER_ALL_ACCESS rights, and Mgr.OpenService always requests SERVICE_ALL_ACCESS rights. Since both of those methods are requesting write access, they require the current user to run at a higher permission level than may otherwise be required by the caller.

It would be nice if additional methods were exposed that would allow either for finer-grained control over the access requested, or at least a variant that requests read-only (ie, GENERIC_READ) access.

Of course, this is easily worked around by directly calling the necessary Win32 APIs, but that isn't the cleanest.

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

$ go version
go version go1.17.1 linux/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=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/user/src/tailscale/go.mod"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2930209240=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Compile this program for Windows
  2. On a Windows machine, run it as a normal, non-elevated user.

What did you expect to see?

The program returns without error.

What did you see instead?

The program fails with access denied errors.

@gopherbot gopherbot added this to the Unreleased milestone Mar 3, 2022
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 7, 2022
@cagedmantis
Copy link
Contributor

cc @golang/runtime @alexbrainman

@alexbrainman
Copy link
Member

@dblohm7 thank you for creating the issue.

I am not sure what you are trying to do, but I tried running your program with these changes in golang.org/sys/windows/svc/mgr:

diff --git a/windows/svc/mgr/mgr.go b/windows/svc/mgr/mgr.go
index de75f4a4..e17b1e84 100644
--- a/windows/svc/mgr/mgr.go
+++ b/windows/svc/mgr/mgr.go
@@ -39,7 +39,7 @@ func ConnectRemote(host string) (*Mgr, error) {
        if host != "" {
                s = syscall.StringToUTF16Ptr(host)
        }
-       h, err := windows.OpenSCManager(s, nil, windows.SC_MANAGER_ALL_ACCESS)
+       h, err := windows.OpenSCManager(s, nil, windows.GENERIC_READ)
        if err != nil {
                return nil, err
        }
@@ -162,7 +162,7 @@ func (m *Mgr) CreateService(name, exepath string, c Config, args ...string) (*Se
 // OpenService retrieves access to service name, so it can
 // be interrogated and controlled.
 func (m *Mgr) OpenService(name string) (*Service, error) {
-       h, err := windows.OpenService(m.Handle, syscall.StringToUTF16Ptr(name), windows.SERVICE_ALL_ACCESS)
+       h, err := windows.OpenService(m.Handle, syscall.StringToUTF16Ptr(name), windows.GENERIC_READ)
        if err != nil {
                return nil, err
        }

and your program still fails with

Opening service rpcss: Access is denied.

What you are trying to do exactly?

I made decision to just use windows.SC_MANAGER_ALL_ACCESS and windows.SERVICE_ALL_ACCESS when I developed this package for my own use case. And no one complained about running this code as non-elevated user, until now.

I obviously tried to keep mgr.Connect and mgr.OpenService parameter list short. Otherwise every user would have to pass at least windows.SC_MANAGER_ALL_ACCESS and windows.SERVICE_ALL_ACCESS to these functions.

Then I discovered that I needed access to the service manager on another Windows pc. That is how mgr.ConnectRemote happened.

I suppose we can add new methods similar to mgr.ConnectRemote and mgr.OpenService that also allows passing access rights. I am not sure what to call these. Happy with your suggestions.

Perhaps you have other ideas.

Thank you.

Alex

@conioh
Copy link

conioh commented May 8, 2022

What you are trying to do exactly?

I'm trying to query a service - find out if a service by a specific name exists or not, and if it does query its configuration.

I made decision to just use windows.SC_MANAGER_ALL_ACCESS and windows.SERVICE_ALL_ACCESS when I developed this package for my own use case. And no one complained about running this code as non-elevated user, until now.

I appreciate that, and if it were a personal project I wouldn't be complaining, but since this package is sanctioned and sponsored by Google/Golang we would like to see it working better. Working when run as non-admin/not elevated (i.e. non-root in Linux equivalent terminology) is not a high bar to expect.

I obviously tried to keep mgr.Connect and mgr.OpenService parameter list short. Otherwise every user would have to pass at least windows.SC_MANAGER_ALL_ACCESS and windows.SERVICE_ALL_ACCESS to these functions.

More likely that the actual required permissions would have been passed rather than these two. It's just one more parameter, not dozens. And if you want to assume required permissions, like os.Open in contract with os.OpenFile does, why assume the most extreme?

@conioh
Copy link

conioh commented Nov 15, 2022

@alexbrainman, how's the investigation going? What are the chances that this gets fixed?

@conioh
Copy link

conioh commented Apr 16, 2023

Ping?

@tguenneguez
Copy link

What is the recommended solution ?
influxdata/telegraf#13382

@RafBishopFox
Copy link

@alexbrainman I have been working on some code to return a list of services on a Windows machine, and I needed to list the services without elevated privileges. Instead of passing windows.GENERIC_READ to windows.OpenSCManager, I found I needed to pass the specific access right to the function, SC_MANAGER_ENUMERATE_SERVICE in this case.

For opening a service, I similarly had to pass the specific access right I wanted to windows.OpenService. Since all I wanted was the configuration and status of the service, I passed in windows.SERVICE_QUERY_CONFIG|windows.SERVICE_QUERY_STATUS.

So the calls should look like this:

h, err := windows.OpenSCManager(s, nil, windows.SC_MANAGER_ENUMERATE_SERVICE)

and

h, err := windows.OpenService(m.Handle, syscall.StringToUTF16Ptr(name), windows.SERVICE_QUERY_CONFIG|windows.SERVICE_QUERY_STATUS)

I think making two new functions that accept permissions makes sense. Perhaps we can call them mgr.ConnectWithPermissions and mgr.OpenServiceWithPermissions. I know those are a bit long, but I am a verbose person 😅 .

@alexbrainman
Copy link
Member

@RafBishopFox

I think making two new functions that accept permissions makes sense. Perhaps we can call them mgr.ConnectWithPermissions and mgr.OpenServiceWithPermissions.

Sounds good to me. But I suspect we need to write a proposal now to add these functions.

If you want to write a proposal, here

https://github.com/golang/proposal?tab=readme-ov-file

are some info. You can probably make your proposal short, because it is not complicated.

And others might suggest better names for these functions.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

8 participants