(CC @golang/windows)

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

@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,

@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

(That was dotnet/runtime#52416.)

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

@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 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

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.

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

@alexbrainman done golang/sys#141

@alexbrainman done golang/sys#141

Replied at https://go-review.googlesource.com/c/sys/+/446535

Alex

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.

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...

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

• https://github.com/apache/httpd/blame/trunk/support/win32/wintty.c#L155-L166
• https://github.com/openssl/openssl/blame/master/crypto/cryptlib.c#L65-L89

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.

+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.

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

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.

@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

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.

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

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

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.