dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.45k stars 4.76k forks source link

NamedPipeServerStream RunAsClient changes other thread's euid #24654

Open eerhardt opened 6 years ago

eerhardt commented 6 years ago

When calling NamedPipeServerStream RunAsClient on Unix, it calls Interop.Sys.SetEUid(peerID), which calls the POSIX command seteuid.

According to the man pages for setreuid and nptl, whenever seteuid is called, it changes the user for all threads in the process in order to be POSIX compliant.

NPTL and process credential changes At the Linux kernel level, credentials (user and group IDs) are a per-thread attribute. However, POSIX requires that all of the POSIX threads in a process have the same credentials. To accommodate this requirement, the NPTL implementation wraps all of the system calls that change process credentials with functions that, in addition to invoking the underlying system call, arrange for all other threads in the process to also change their credentials.

Wrapper functions employing this technique are provided for setgid(2), setuid(2), setegid(2), seteuid(2), setregid(2), setreuid(2), setresgid(2), setresuid(2), and setgroups(2).

I have a repro of this issue here: https://github.com/eerhardt/NamedPipeRunAsClient/

@ianhays @stephentoub

stephentoub commented 6 years ago

Yes, this is why the comments talk about process rather than thread: https://github.com/dotnet/corefx/blob/0eb5e7451028e9374b8bb03972aa945c128193e1/src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs#L177 It's a known difference from Windows. If there's a way to implement it per thread, it should be changed, but as your comment points out, POSIX does not support that.

eerhardt commented 6 years ago

If there's a way to implement it per thread, it should be changed

From everything I've read (and from a little prototype testing on my Ubuntu box), this can be achieved using:

syscall(SYS_setreuid, getuid(), euid);

I'm not sure of the full ramifications of doing this. I know that on macOS, syscall is deprecated as of 10.12, so we'd have to find a different approach on macOS. We may have issues on other platforms as well.

JeremyKuhne commented 4 years ago

Triage: @eerhardt Is this something we should still be tracking? How does this impact customers?

eerhardt commented 4 years ago

Is this something we should still be tracking?

If we are not going to change the current behavior, I would expect an update to the docs to describe any differences between Windows and Unix.

https://docs.microsoft.com/en-us/dotnet/api/system.io.pipes.namedpipeserverstream.runasclient?view=netcore-3.1

Calls a delegate while impersonating the client.

This makes it sound like only the delegate gets executed while impersonating the client, not any other threads running in the process.

How does this impact customers?

This is a behavior change between Windows and Unix. Customers coming from .NET Framework and Windows using this method could expect it work like it does on Windows, and could have security issues with their product if they are moving it to Unix.

https://github.com/dotnet/corefx/issues/24746#issuecomment-338016478 is an example where I assume someone coming from Windows would expect this API to behave the same on Unix.

ghost commented 3 years ago

This issue has been automatically marked no recent activity because it has been marked as needs more info but has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity.

Please refer to our contribution guidelines for tips on what information might be required.

fubar-coder commented 3 years ago

The current code still hasn't changed and is still using seteuid. What kind of information is needed to at least make this behavior consistent across Linux and Windows?

EDIT: The reason I ask about this, is that I'm planning to use some kind of impersonation for a service that's accessible for multiple different users. It would be confusing if multiple calls to NamedPipeServerStream.RunAsClient would result in globally changing the EUID.

ghost commented 3 years ago

This issue has been automatically marked no recent activity because it has been marked as needs more info but has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity.

Please refer to our contribution guidelines for tips on what information might be required.

eerhardt commented 3 years ago

I'm removing the "needs more info" label - as I think all the info is available here. We just need to make a decision on whether we update the docs, or try to fix the behavior (if possible).

ghost commented 3 years ago

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

ghost commented 3 years ago

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

fubar-coder commented 3 years ago

@eerhardt Did the msftbot run wild?

eerhardt commented 3 years ago

Yes, and I made the mistake of trying to adjust the labels while it was running, making it even more active.