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 : IsWindowsService doesn't work properly inside a windows container #56335

Open
gillg opened this issue Oct 19, 2022 · 25 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@gillg
Copy link

gillg commented Oct 19, 2022

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

$ go version go1.19.2 linux/amd64
x/sys v0.1.0

Does this issue reproduce with the latest release?

Yes

What did you do?

If I use IsWindowsService() inside a container servercore:ltsc2019 it always returns false, nevermind if I launch it as CLI or as service.
In windows containers world the service stack is native. It's not necessarly a "container pattern" but it's widely used for multiple reasons. So that method should work.

After some tests, the fail reason seems pretty simple.
On the host the "Session ID" of the process svchost.exe (for example) is 0, but inside the container, the "Session ID" of a process svchost.exe is 1.
The only process with a session ID 0 inside a container are :

Get-Process | where {$_.SessionId -eq 0}

Handles  NPM(K)    PM(K)      WS(K)     CPU(s)     Id  SI ProcessName
-------  ------    -----      -----     ------     --  -- -----------
      0       0       56          8                 0   0 Idle
     50       3      456       1212       5.20    964   0 smss
   2117       0      156        136      23.61      4   0 System

But good news, the parent process is still "services.exe".

WmiApSrv is a random service (it's the native monitoring stack of windows)

Get-Process -Id (Get-CimInstance Win32_Process -Filter "Name = 'WmiApSrv.exe'" | select ParentProcessId).ParentProcessId

Handles  NPM(K)    PM(K)      WS(K)     CPU(s)     Id  SI ProcessName
-------  ------    -----      -----     ------     --  -- -----------
    218      10     3628       7780     175.66    524   1 services

I think, removing the condition parentProcess.SessionID == 0 could be safe and fine... Any windows guru there ?

In the meantime, the older deprecated funtion IsAnInteractiveSession is also buggy in a container, and never detects an interactive session even if you are in a CLI.

@bcmills
Copy link
Contributor

bcmills commented Oct 20, 2022

(CC @golang/windows)

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted labels Oct 20, 2022
@alexbrainman
Copy link
Member

Thank you for creating this issue.

I think, removing the condition parentProcess.SessionID == 0 could be safe and fine...

If you look at the code, you can see that Go code is copied from Microsoft .Net code:

https://github.com/dotnet/extensions/blob/f4066026ca06984b07e90e61a6390ac38152ba93/src/Hosting/WindowsServices/src/WindowsServiceHelpers.cs#L31

And that code still checks for sessionID of 0.

Do you have any references that it is safe and fine to remove parentProcess.SessionID == 0 check?

Thank you.

Alex

@gillg
Copy link
Author

gillg commented Oct 23, 2022

@alexbrainman Now I asked an official question to Microsoft. But their condition is probably just a safety guard to avoid a wrong interpretation if a process is spawned from a custom process named services.exe.
So adding a condition on session name seems safer than nothing,

@gillg
Copy link
Author

gillg commented Oct 23, 2022

@alexbrainman I have a very good news. The answer is already done.
They decided to just remove the session Id condition (with the probable risk of a wrong result of you name your own exe "services").

So x/sys should follow the same logic I think.

Original code https://github.com/dotnet/runtime/blob/36bf84fc4a89209f4fdbc1fc201e81afd8be49b0/src/libraries/Microsoft.Extensions.Hosting.WindowsServices/src/WindowsServiceHelpers.cs#L32

@bcmills
Copy link
Contributor

bcmills commented Oct 24, 2022

(That was dotnet/runtime#52416.)

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 24, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 24, 2022
@alexbrainman
Copy link
Member

They decided to just remove the session Id condition (with the probable risk of a wrong result of you name your own exe "services").

So x/sys should follow the same logic I think.

@gillg sounds good to me.

Do you want to send a CL? See https://go.dev/doc/contribute on how to contribute.

Thank you.

Alex

@gillg
Copy link
Author

gillg commented Oct 30, 2022

@alexbrainman it's the weekend, so maybe I can succeed to follow the contribution guide 😆

I let you know if I don't have time or succeed to open a correct PR.

By the way do you have an internal mechanic to observe changes to their runtime and see if changes in Golang are revelant ?

@alexbrainman
Copy link
Member

@alexbrainman it's the weekend, so maybe I can succeed to follow the contribution guide 😆

I let you know if I don't have time or succeed to open a correct PR.

Sounds good to me.

By the way do you have an internal mechanic to observe changes to their runtime and see if changes in Golang are revelant ?

You need to run make.bash or make.bat (depending on the OS you are on) once do build Go. After that go command will automatically rebuild any changes you make in runtime package.

But the fix for this issue will not require changing runtime package, but golang.org/x/sys/windows/svc package.

Alex

@gillg
Copy link
Author

gillg commented Oct 30, 2022

You need to run make.bash or make.bat (depending on the OS you are on) once do build Go.

Sorry it was not clear, I was asking if the x/sys/windows Golang team was monitoring changes in the official Microsoft DotNet runtime. Just to be aware of this kind of things and report back potential security issues too.

@alexbrainman
Copy link
Member

I was asking if the x/sys/windows Golang team was monitoring changes in the official Microsoft DotNet runtime.

I don't monitor Microsoft DotNet runtime changes. I doubt anyone on Go Team monitors Microsoft DotNet runtime changes either.

Alex

@gillg
Copy link
Author

gillg commented Oct 30, 2022

@alexbrainman done golang/sys#141

@alexbrainman
Copy link
Member

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 1, 2022

FWIW, this seems like a pretty bad change. I don't mind following .NET, but doesn't this seem awfully brittle? You can just make your parent process named 'services.exe' and trick the test?

Probably I should reverse engineer a bit and find something better.

@gillg
Copy link
Author

gillg commented Nov 1, 2022

Probably I should reverse engineer a bit and find something better.

Definitely agree ! That's why I proposed to eventually use the session name as PR comment.
I made the test and nevermind the context, the process services is launched as a session named services.
But now the challenge is to find how get the session name of a process in Golang...

gillg pushed a commit to gillg/golang-sys that referenced this issue Nov 1, 2022
The current IsWindowsService() function was inherited from the legacy .NET
framework and not compatible inside windows containers. We need to update it
so it works inside containers.

References to: dotnet/runtime#52416
Fixes golang/go#56335
@dims
Copy link

dims commented Nov 1, 2022

OpenSSL and httpd seems to be using GetProcessWindowStation to get this info for a VERY long time!

@gillg
Copy link
Author

gillg commented Nov 17, 2022

Thanks @dims for your archeology !
I'm not super familiar to transpose it correctly to Golang, does anyone can add a commit on my PR, or do you prefer follow the .NET strategy in a first round ?
I don't see any new comment in the PR, so I don't know if womeone waits for something before merging.

@guilhermocc
Copy link

guilhermocc commented Nov 18, 2022

+1 for this issue to be resolved, we are implementing native windows support for https://github.com/spiffe/spire using the svc lib, and we would appreciate the windows container compatibility for the IsWindowsService method. For now, we will be using a homemade version of this method with the same aforementioned dotnet/runtime fix.

@alexbrainman
Copy link
Member

I'm not super familiar to transpose it correctly to Golang, does anyone can add a commit on my PR, or do you prefer follow the .NET strategy in a first round ?
I don't see any new comment in the PR, so I don't know if womeone waits for something before merging.

Hello @gillg

You are not expected to do anything more. You did everything correctly at https://go-review.googlesource.com/c/sys/+/446535 . It is my turn to move this change forward.

I did not comment here or on CL 446535 because I share Jason's concern (from #56335 (comment))

this seems like a pretty bad change. I don't mind following .NET, but doesn't this seem awfully brittle? You can just make your parent process named 'services.exe' and trick the test?

I was hopping Jason will make some suggestion on how to improve. But he did not comment more.

Thanks to @dims for posting how other tools implement similar code at #56335 (comment) . But these approaches are very different from what current IsWindowsService does. How can we be sure that their code is better than what we already have? We don't want to break our existing users. How do we know their code actually works inside containers? Someone would have to try it before it can even be considered.

I also had this idea. If we can find a way for a Windows process to determine if it is running inside of a container, then we can change IsWindowsService to run existing code if not inside of a container, and new .NET code if inside of a container. This will let our existing users run the same code as before, while container users will test new .NET code.

Unfortunately I could not find a reliable way for a Windows process to determine if it is running in a container or not.

I found many many ways to do it in Linux, like /proc/... or /.dockerenv files - https://www.thorsten-hans.com/check-if-application-is-running-in-docker-container . But none of them works in Windows according to https://www.thorsten-hans.com/check-if-application-is-running-in-docker-container/#what-about-windows-containers .

The only thing we can try are described in

https://stackoverflow.com/a/50748300/1390507

and

https://stackoverflow.com/a/46810471/1390507 (I don't know how to implement second solution)

I don't actually have Windows computer with containers. I cannot even try these things. So someone else needs to try these suggestions.

I also opened to other ideas.

Thank you.

Alex

@gillg
Copy link
Author

gillg commented Nov 19, 2022

Thanks @alexbrainman !

In my opinion working with the session name / windows station name is the more safe.
There is no reliable way to test if you are running in a container. Even on Linux it's not a good idea because all the tricks depends on on container runtime... You will not have the same behavior on Docker, Podman, containerd, etc. So never do that !
The goal of containers is to isolate the processes so the behavior of the process should be the same inside or outside the container without tricks.
On windows the isolation is not based on cgroups but part of the host processes are injected to the container as "system" process. That's probably a reason why with "isolation" process level (against hyperv) you can only run a container built on the same windows major version as the host.
See https://learn.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/version-compatibility?tabs=windows-server-2022%2Cwindows-11#matching-container-host-version-with-container-image-versions

Today windows only support docker and containerd is in a good position to be really supported soon.

So regarding all that things, using the concept of session name seems really the best option and it seems not possible to spawn a process with this session name if you are not the "real" process services.exe. So for me the risk for existing users is probably null.

@alexbrainman
Copy link
Member

@gillg

In my opinion working with the session name / windows station name is the more safe.

I assume you are proposing not to go ahead with your https://go-review.googlesource.com/c/sys/+/446535 and instead rewrite IsWindowsService using either of those two approaches described at #56335 (comment) . If so, someone else would have to do it. And I suggest you also create a proposal about changing an implementation. Because I will not review the code unless others will agree with that change.

Even on Linux it's not a good idea because all the tricks depends on on container runtime...

I am happy to accept whatever tricks we come up with, because IsWindowsService is broken on container now. So any improvement is good. What I don't want to happen is to break existing users. And using files under /proc seems safe enough in that regard. So if you can come up with a similar solution for Windows, I think that would be reasonable.

Alex

@gillg
Copy link
Author

gillg commented Nov 21, 2022

In fact I agree with you, just not on the form about trying to identify if we are inside a container or not.

Merging the current PR seems pretty safe even if the solution from Microsoft is really discutable, and as you said, a next more open discussion to add the condition on the session name (or something better ?) could be a good option.
But just as I can say for now after some tests, using the session name seems the more reliable.

@alexbrainman
Copy link
Member

Merging the current PR seems pretty safe ...

I disagree.

If you are talking about https://go-review.googlesource.com/c/sys/+/446535 , then I share Jason's opinion (from #56335 (comment)) that the CL makes it easy to trick IsWindowsService function.

Alex

@clarkb7
Copy link

clarkb7 commented Aug 18, 2023

In my opinion working with the session name / windows station name is the more safe.

This won't work by itself. In Windows Containers the only session is the "services" session. So apps running in containers that aren't Windows Services will get a false positive.

> docker run --rm --isolation=process mcr.microsoft.com/windows/servercore:ltsc2022 qwinsta
 SESSIONNAME       USERNAME                 ID  STATE   TYPE        DEVICE
>services                                    3  Disc

> docker run -it --rm --isolation=hyperv mcr.microsoft.com/windows/servercore:ltsc2022 qwinsta
 SESSIONNAME       USERNAME                 ID  STATE   TYPE        DEVICE
>services                                    1  Disc

@pjanotti
Copy link

pjanotti commented Dec 5, 2023

I hit this same issue and initially was thinking about asking for the fix proposed above. However, per recommendation of service API documentation programs should instead call svc.Run and in case of error check for windows.ERROR_FAILED_SERVICE_CONTROLLER_CONNECT. If this is the error the application can safely assume that it is not running as a service. The duration of a call to svc.Run failing with this error was below 3 microseconds in the current GH runner and below 5 microseconds on my box.

It will be nice to add something like the above to the package documentation so others follow the pattern of checking the return of svc.Run instead of trying to use either svc.IsAnInteractiveSession or svc.IsWindowsService since both have different pitfalls.

@torsm
Copy link

torsm commented Aug 26, 2024

However, per recommendation of service API documentation programs should instead call svc.Run and in case of error check for windows.ERROR_FAILED_SERVICE_CONTROLLER_CONNECT. If this is the error the application can safely assume that it is not running as a service.

+1 to this. We use an MSI installer to create our windows service, and if it's the MSI installer that launched the service post-install, the session ID of services.exe is also != 0. So this is another instance apart from containers where the current implementation is not behaving accurately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Development

No branches or pull requests

10 participants