Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed #26395

safern · 2018-01-17T21:29:56Z

This is the implementation for PipeOptions.CurrentUserOnly.

Server side (Windows): if the flag is on, remove the flag from options variable, and add all the current user as the owner and the only user with rights to access the pipe through ACL attributes. This handles the user's elevation as well.
Client side (windows and unix): if flag is on, remove it, set bool variable to true and then when trying to connect to the server, if the variable is true and the connection was successful, validate that the server we connected was created by the same user with the same elevation level by comparing the Sids. If this is not the case, set State as closed and throw.

Relates to: https://github.com/dotnet/corefx/issues/25427

JeremyKuhne · 2018-01-17T21:45:33Z

src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

@@ -412,6 +412,33 @@ internal static unsafe Interop.Kernel32.SECURITY_ATTRIBUTES GetSecAttrs(HandleIn
            return secAttrs;
        }

+        internal static unsafe Interop.Kernel32.SECURITY_ATTRIBUTES GetSecAttrs(HandleInheritability inheritability, PipeSecurity pipeSecurity, out object pinningHandle)


Can you change this to ref GCHandle to avoid the extra allocation?

JeremyKuhne · 2018-01-17T21:47:51Z

src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

+
+            if (pipeSecurity != null)
+            {
+                // no inherability flag was found so we need to initialize secAttrs structure.


inherability [](start = 22, length = 12)

inheritability

also this comment should go inside the "if" shouldnt it?

JeremyKuhne · 2018-01-17T21:52:06Z

src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Windows.cs

+            }
+            finally
+            {
+                if (pinningHandle != null)


If we pass the GCHandle by ref to GetSecAttrs this can be changed to if (pinningHandle.IsAllocated) pinningHandle.Free();

danmoseley · 2018-01-18T00:26:07Z

src/System.IO.Pipes/src/System/IO/Pipes/PipeOptions.cs

@@ -10,5 +10,6 @@ public enum PipeOptions
        None = 0x0,
        WriteThrough = unchecked((int)0x80000000),
        Asynchronous = unchecked((int)0x40000000),  // corresponds to FILE_FLAG_OVERLAPPED
+        CurrentUserOnly = unchecked((int)0x20000000)


What happens if Windows adds a new option to the CreateNamedPipe flags with this number? I guess that's OK, it's only our convention that the numbers currently match what we send to Windows.

This was the proposed value in the original API proposal, from @stephentoub:

but for extra good measure we should also choose a value that's not currently accepted by CreateNamedPipe

safern · 2018-01-18T00:36:26Z

@weshaggard this needs some stuff in System.IO.Pipes.AccessControl but that is shipped OOB and is not part of the closure, should I move types down and add typeforwards or what do you suggest?

weshaggard · 2018-01-18T00:39:49Z

src/System.IO.Pipes/src/System.IO.Pipes.csproj

@@ -254,6 +254,12 @@
    <Reference Include="System.Threading.Overlapped" />
    <Reference Include="System.Threading.Tasks" />
  </ItemGroup>
+  <ItemGroup Condition="'$(TargetsWindows)' == 'true'">
+    <Reference Include="System.IO.Pipes.AccessControl" />


Lets avoid this reference we intentionally wanted System.IO.Pipes.AccessControl to be a higher level component.

Yes I added it cause I missed it was OOB :) Removing and we can decide how much we want to move down to System.IO.Pipes.

weshaggard · 2018-01-18T00:43:25Z

@weshaggard this needs some stuff in System.IO.Pipes.AccessControl but that is shipped OOB and is not part of the closure, should I move types down and add typeforwards or what do you suggest?

How much is needed? @danmosemsft do we want to merge all of System.IO.Pipes.AccessControl into System.IO.Pipes? If we do that then we will have to do the same thing for AccessControl as well and make it throw on non-windows.

weshaggard · 2018-01-18T00:43:47Z

src/System.IO.Pipes/src/System.IO.Pipes.csproj

+    <Reference Include="System.Security.AccessControl" />
+    <Reference Include="System.Security.Principal.Windows" />
+    <Reference Include="System.Security.Claims" />
+  </ItemGroup>
  <ItemGroup Condition="'$(TargetsUnix)' == 'true'">


What are we going to do on unix?

I'm still reading about Unix security. Not yet what dependencies we're going to need. If any of the ones imported for windows is needed I will move it to the common ItemGroup.

On Unix we will probably want to create a directory that's chmod'd to only be accessible by the current user, and then create the domain socket in that directory.

That is the main thing still missing: right now there is enforcement on the client but nothing on the server side.

Yes, I'm working on it.

I see what you're saying. What if for the UserOnly pipes instead of adding the corefx prefix in the pipe name we use that space that we're already taking, for the directory name?

corefx/src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs

Line 29 in 40cbd56

private static readonly string s_pipePrefix = Path.Combine(Path.GetTempPath(), "CoreFxPipe_");

We could create a directory that contains the userid so that it is unique and related to this user and we don't generate random names, so maybe something like: tempdir/{euid}_fx/{pipename} where an euid is using the 16 bits. As the docs say:

The original Linux getuid() and geteuid() system calls supported only 16-bit user IDs. Subsequently, Linux 2.4 added getuid32() and geteuid32(), supporting 32-bit IDs. The glibc getuid() and geteuid() wrapper functions transparently deal with the variations across kernel versions.

Which at the end will be using less space than what CoreFxPipe_ prefix is already using on a regular named pipe.

@stephentoub thoughts?

@stephentoub noting for posterity that on Roslyn they currently don't use this approach, they connect blindly then check GetPeerID() against GetEUid().

https://github.com/dotnet/roslyn/blob/c7a465fabaf688affbd5897d7348431eeb2059b5/src/Compilers/Shared/BuildServerConnection.cs#L490

@stephentoub any reason to prefer one over the other? It seems like getpeerid() is for this exact purpose. https://linux.die.net/man/3/getpeereid

@danmosemsft I am starting to look back at this to add the tests this week, and I think your point about getpeerid is valid. The only thing that cross my mind is that the existing API on the server side currently has to either throw an exception for each mismatched peer (after the socket connection) or simply ignore them - neither seems good (perhaps a new API in the future?). In this sense the directory is helpful because prevents connections before it reaches the server socket. OTOH it will introduce some small difference between Unix and Windows in at least some (mis)configurations:

Assume same user for all steps;

Create server without current user only flag;

Create client with current user only flag;

On Windows: client will connect fine;

On Unix: client, depending on API, hangs or throws timeout exception (since the path for the named pipes is different between server and client)

All of this to say that let's keep the directory, but also add the getpeerid check also on the server side (extra precaution), it is already done on the client side.

weshaggard · 2018-01-18T00:51:00Z

src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Windows.cs

+            if ((options & PipeOptions.CurrentUserOnly) != 0)
+            {
+                SecurityIdentifier identifier = GetCurrentUser();
+                PipeAccessRule rule = new PipeAccessRule(identifier, PipeAccessRights.ReadWrite, AccessControlType.Allow);


How is this setting up the security for the given Pipe? I don't see the pipe instance getting passed anywhere.

It is being passed through the security attributes, see: https://github.com/dotnet/corefx/pull/26395/files/c50bb469733b32ab61607be7dbe9ae3c41111445#diff-a7e95545fe2b63c35aaee6c4c3de3cffR70

jaredpar · 2018-01-18T00:57:42Z

CC @agocke

danmoseley · 2018-01-18T02:15:27Z

src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.Windows.cs

+            if (remoteOwnerSid != currentUserSid)
+            {
+                State = PipeState.Closed;
+                throw new UnauthorizedAccessException();


You might want a custom message eg "Could not connect to the pipe because it was not owned by the current user."

safern · 2018-01-18T18:38:35Z

How much is needed?

I just need PipeSecurity and whatever PipeSecurity needs from S.IO.Pipes.AccessControl.

weshaggard · 2018-01-18T19:11:38Z

I just need PipeSecurity and whatever PipeSecurity needs from S.IO.Pipes.AccessControl.

Looks like that is most of that library. I originally though this reference would cause a cycle but it doesn't appear to so the dependency would be allowed but it would pull it into the .NET Core App. I don't have a strong opinion about pull in the library or just merging the library. I will leave that question up to @danmosemsft.

safern · 2018-01-18T19:32:57Z

Ok, I'll wait to update this PR until we get a decision on what we want to do regarding this dependency.

pjanotti · 2018-01-19T03:41:48Z

Testing will need to be manual

What is preventing you from adding tests?

pjanotti

Small stuff, but I think it should be feasible to add tests to this feature.

pjanotti · 2018-01-19T03:49:16Z

src/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateServer.cs

@@ -3,6 +3,8 @@
 // See the LICENSE file in the project root for more information.

 using Microsoft.Win32.SafeHandles;
+using System.Security.Principal;


Are these really needed by the new test?

pjanotti · 2018-01-19T03:50:56Z

src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Windows.cs

+                pipeSecurity.AddAccessRule(rule);
+                pipeSecurity.SetOwner(identifier);
+
+                // We need to remove this flag from options because it is not a valid flag for windows PInvoke to create a pipe.


Debug.Assert this invariant after the if block.

pjanotti · 2018-01-19T19:16:54Z

src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

+                if (secAttrs.Equals(default(Interop.Kernel32.SECURITY_ATTRIBUTES)))
+                {
+                    secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES();
+                    secAttrs.nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES);


I see that the old code only sets nLength when inheritability is set but does it cause any problem? I would expect not. Anyway, the logic here can be simply to create the struct and then set the fields, there is no need to used the old GetSecAttrs and compare against the default.

pjanotti · 2018-01-19T19:24:32Z

src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Windows.cs

+            if ((options & PipeOptions.CurrentUserOnly) != 0)
+            {
+                SecurityIdentifier identifier = GetCurrentUser();
+                PipeAccessRule rule = new PipeAccessRule(identifier, PipeAccessRights.ReadWrite, AccessControlType.Allow);


Windows should be creating a default DACL for the named pipe if we don't specify one, can you compare the default one with the one created when the rule is added? Just to be double sure...

The default DACL will give full control to more users than the current one:

The ACLs in the default security descriptor for a named pipe grant full control to the LocalSystem account, administrators, and the creator owner. They also grant read access to members of the Everyone group and the anonymous account.

Also in the docs it says that if we want to prevent any other user to connect to the pipe we should add the Sid to the DACL:

To prevent remote users or users on a different terminal services session from accessing a named pipe, use the logon SID on the DACL for the pipe. The logon SID is used in run-as logons as well; it is the SID used to protect the per-session object namespace.

I just compared the rules with and without my change and that can be seen. Without my change it will have 4 rules involving the administrators, owner and others. With my change the Pipe will have only one rule set specifically to the owner's SID with ReadWrite access.

pjanotti · 2018-01-23T18:20:06Z

src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

+                    secAttrs.bInheritHandle = Interop.BOOL.TRUE;
+                }
+
+                byte[] securityDescriptor = pipeSecurity.GetSecurityDescriptorBinaryForm();


Since it is an "or" on the if statement you can hit this line if pipeSecurity is null.

hrgg forgot to add that if in the refactor. Thanks for catching it.

pjanotti · 2018-01-23T20:30:13Z

src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

-                byte[] securityDescriptor = pipeSecurity.GetSecurityDescriptorBinaryForm();
-                pinningHandle = GCHandle.Alloc(securityDescriptor, GCHandleType.Pinned);
-                fixed (byte* pSecurityDescriptor = securityDescriptor)
+                if (pipeSecurity != null)


Nitpick: it was already on the old code but you don't need the if statement on top, nor the new Interop.Kernel32.SECURITY_ATTRIBUTES(). Just use the one created on the top set its length (always) and let the other ifs do any of the extra work if needed.

safern · 2018-01-24T22:39:14Z

@weshaggard could you please review: a57cbdf which is moving types down to System.IO.Pipes?

safern · 2018-02-02T09:08:25Z

I've added Unix implementation to the client side to validate we're connecting to a pipe created by the same user. Still figuring out what to do in the Unix server side and writing more tests.

weshaggard · 2018-02-13T01:06:37Z

It is odd that msbuild isn't correctly reporting the errors, I suspect it is because the ILLinker task isn't logging them correctly but I do see the following error:

16:04:36 Fatal error in IL Linker
16:04:36 Mono.Cecil.AssemblyResolutionException: Failed to resolve assembly: 'System.Threading.Tasks, Version=4.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
16:04:36    at Mono.Linker.DirectoryAssemblyResolver.Resolve(AssemblyNameReference name, ReaderParameters parameters)
16:04:36    at Mono.Linker.AssemblyResolver.Resolve(AssemblyNameReference name, ReaderParameters parameters)
16:04:36    at Mono.Linker.DirectoryAssemblyResolver.Resolve(AssemblyNameReference name)
16:04:36    at Mono.Cecil.MetadataResolver.Resolve(TypeReference type)
16:04:36    at Mono.Cecil.TypeReference.Resolve()
16:04:36    at Mono.Linker.TypeReferenceExtensions.<GetMethods>d__4.MoveNext()
16:04:36    at Mono.Linker.Steps.TypeMapStep.TryMatchMethod(TypeReference type, MethodDefinition method)
16:04:36    at Mono.Linker.Steps.TypeMapStep.GetBaseMethodInTypeHierarchy(TypeDefinition type, MethodDefinition method)
16:04:36    at Mono.Linker.Steps.TypeMapStep.GetBaseMethodInTypeHierarchy(MethodDefinition method)
16:04:36    at Mono.Linker.Steps.TypeMapStep.MapVirtualBaseMethod(MethodDefinition method)
16:04:36    at Mono.Linker.Steps.TypeMapStep.MapVirtualMethod(MethodDefinition method)
16:04:36    at Mono.Linker.Steps.TypeMapStep.MapVirtualMethods(TypeDefinition type)
16:04:36    at Mono.Linker.Steps.TypeMapStep.MapType(TypeDefinition type)
16:04:36    at Mono.Linker.Steps.TypeMapStep.ProcessAssembly(AssemblyDefinition assembly)
16:04:36    at Mono.Linker.Steps.BaseStep.Process(LinkContext context)
16:04:36    at Mono.Linker.Pipeline.Process(LinkContext context)
16:04:36    at Mono.Linker.Driver.Run()
16:04:36    at Mono.Linker.Driver.Main(String[] args)

I believe there was an offline email about this issue.

pjanotti · 2018-02-13T02:17:38Z

Thanks @weshaggard I forgot about that... for my learning where did you find the error? I guess I am looking at the wrong logs...

weshaggard · 2018-02-13T17:13:45Z

I opened the All configuration build log and searched for error and found it.

pjanotti · 2018-02-13T18:43:25Z

Ah I was looking that the Windows one...

ericstj · 2018-02-13T20:50:54Z

src/System.IO.Pipes.AccessControl/src/System.IO.Pipes.AccessControl.csproj

    <Reference Include="System.Security.AccessControl" />
    <Reference Include="System.Security.Principal.Windows" />
  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp' OR '$(TargetGroup)' == 'uap'">
+    <ReferenceFromRuntime Include="System.Private.Corelib" />


Are you sure you needed to switch to building as CoreLib based assembly? Since Pipes is System.Runtime based I would have expected it to be sufficient to merely add a ProjectReference to Pipes and keep everything else as a simple reference. It is very weird for something this high in the stack to have access to System.Private.CoreLib.

Here's what I mean: 51b4aeb

With that commit I was able to successfully build this without any changes to dependencies. @pjanotti

…n flag is passed

pjanotti · 2018-02-22T05:45:48Z

I will be adding negative tests next week for this feature.

…s passed (dotnet/corefx#26395) * Expose PipeOptions.CurrentUserOnly and add windows implementation when flag is passed * PR Feedback * Add missing if statement * Move PipeSecurity types in implementation to System.IO.Pipes * PR Feedback and move tests to netcoreapp file * Add CurrentUserOnly implementation for NamedPipeClient in Unix * PR Feedback * Refactor to not have duplicated code * Implement server side current option only in named pipes for unix * More PR Feedback * PR Feedback round 3 * Add more tests * Fix build and add using to WindowsIdentity objects * Fix packaging issues * netstandard-Windows_NT needs to be built from sources Commit migrated from dotnet/corefx@29cd6a0

safern added area-System.IO Needs extra testing before merge labels Jan 17, 2018

safern self-assigned this Jan 17, 2018

safern requested review from jaredpar, stephentoub, pjanotti, danmoseley and JeremyKuhne January 17, 2018 21:29

JeremyKuhne reviewed Jan 17, 2018

View reviewed changes

danmoseley reviewed Jan 18, 2018

View reviewed changes

weshaggard reviewed Jan 18, 2018

View reviewed changes

danmoseley reviewed Jan 18, 2018

View reviewed changes

pjanotti reviewed Jan 19, 2018

View reviewed changes

pjanotti reviewed Jan 23, 2018

View reviewed changes

safern force-pushed the NamedPipesAcl branch from fd34eae to a57cbdf Compare January 24, 2018 22:38

safern changed the title ~~WIP: Expose PipeOptions.CurrentUserOnly and add windows implementation when flag is passed~~ WIP: Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed Feb 2, 2018

ericstj reviewed Feb 13, 2018

View reviewed changes

safern and others added 15 commits February 21, 2018 11:29

Expose PipeOptions.CurrentUserOnly and add windows implementation whe…

20e7102

…n flag is passed

PR Feedback

47bf74e

Add missing if statement

7d9ab1a

Move PipeSecurity types in implementation to System.IO.Pipes

c827c7f

PR Feedback and move tests to netcoreapp file

b53a112

Add CurrentUserOnly implementation for NamedPipeClient in Unix

7410628

PR Feedback

528b493

Refactor to not have duplicated code

18d6521

Implement server side current option only in named pipes for unix

b3f14ca

More PR Feedback

2b537e6

PR Feedback round 3

eeb1e39

Add more tests

cea4c1e

Fix build and add using to WindowsIdentity objects

57f674d

Fix packaging issues

71d475e

netstandard-Windows_NT needs to be built from sources

c282a2b

pjanotti force-pushed the NamedPipesAcl branch from 49e610b to c282a2b Compare February 21, 2018 23:50

pjanotti changed the title ~~WIP: Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed~~ Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed Feb 22, 2018

pjanotti removed the Needs extra testing before merge label Feb 22, 2018

pjanotti merged commit 29cd6a0 into dotnet:master Feb 22, 2018

pjanotti mentioned this pull request Feb 26, 2018

NamedPipe: CurrentUserOnly, quick fixes for Unix #27463

Merged

karelz added this to the 2.1.0 milestone Mar 10, 2018

safern deleted the NamedPipesAcl branch May 8, 2018 20:26

carlossanlop mentioned this pull request Oct 25, 2019

Document System.IO.Pipes.PipeOptions.CurrentUserOnly dotnet/dotnet-api-docs#3411

Merged

Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed #26395

Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed #26395

Conversation

safern commented Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weshaggard commented Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jan 18, 2018

Choose a reason for hiding this comment

safern commented Jan 18, 2018

weshaggard commented Jan 18, 2018

safern commented Jan 18, 2018

pjanotti commented Jan 19, 2018

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern commented Jan 24, 2018

safern commented Feb 2, 2018

weshaggard commented Feb 13, 2018

pjanotti commented Feb 13, 2018

weshaggard commented Feb 13, 2018

pjanotti commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti commented Feb 22, 2018

safern commented Jan 17, 2018 •

edited

Loading

weshaggard commented Jan 18, 2018 •

edited

Loading