From b330391ab366fe3574c770cbc65ed30866dccce9 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Thu, 1 Mar 2018 15:51:47 -0800 Subject: [PATCH] PipeOptions.CurrentUserOnly: remove dir on Unix and add negative tests for Windows (dotnet/corefx#27573) * Initial unpolished change Cleanup impersonation implementation Remove extra dir added for CurrentUserOnly on Unix * PR feedback plus NETFX fix Commit migrated from https://github.com/dotnet/corefx/commit/df5bf81649c0bc3ed64dd0354add2c97b5944a2c --- .../src/System.IO.Pipes.csproj | 3 - .../System/IO/Pipes/NamedPipeClientStream.cs | 2 +- .../IO/Pipes/NamedPipeServerStream.Unix.cs | 2 +- .../src/System/IO/Pipes/PipeStream.Unix.cs | 14 +- .../src/System/IO/Pipes/PipeStream.Windows.cs | 3 +- ...Test.CurrentUserOnly.netcoreapp.Windows.cs | 161 ++++++++++++++++++ ...amedPipeTest.CurrentUserOnly.netcoreapp.cs | 23 ++- .../tests/System.IO.Pipes.Tests.csproj | 9 +- 8 files changed, 185 insertions(+), 32 deletions(-) create mode 100644 src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.Windows.cs diff --git a/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj b/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj index 890853949c7ac..a9e859a5bde57 100644 --- a/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj +++ b/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj @@ -182,9 +182,6 @@ Common\Interop\Unix\Interop.IOErrors.cs - - Common\Interop\Unix\Interop.ChMod.cs - Common\Interop\Unix\Interop.Close.cs diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs index 430f4000ea4aa..80767b60bb35e 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs @@ -90,7 +90,7 @@ public NamedPipeClientStream(String serverName, String pipeName, PipeDirection d IsCurrentUserOnly = true; } - _normalizedPipePath = GetPipePath(serverName, pipeName, IsCurrentUserOnly); + _normalizedPipePath = GetPipePath(serverName, pipeName); _direction = direction; _inheritability = inheritability; _impersonationLevel = impersonationLevel; diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs index 25c8d8a5d6848..05e7b843644da 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs @@ -41,7 +41,7 @@ private void Create(string pipeName, PipeDirection direction, int maxNumberOfSer // We don't have a good way to enforce maxNumberOfServerInstances across processes; we only factor it in // for streams created in this process. Between processes, we behave similarly to maxNumberOfServerInstances == 1, // in that the second process to come along and create a stream will find the pipe already in existence and will fail. - _instance = SharedServer.Get(GetPipePath(".", pipeName, IsCurrentUserOnly), maxNumberOfServerInstances); + _instance = SharedServer.Get(GetPipePath(".", pipeName), maxNumberOfServerInstances); _direction = direction; _options = options; diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs index 786d3a0bf60ef..c11d8b699cbca 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs @@ -29,7 +29,7 @@ public abstract partial class PipeStream : Stream /// Prefix to prepend to all pipe names. private static readonly string s_pipePrefix = Path.Combine(Path.GetTempPath(), "CoreFxPipe_"); - internal static string GetPipePath(string serverName, string pipeName, bool isCurrentUserOnly) + internal static string GetPipePath(string serverName, string pipeName) { if (serverName != "." && serverName != Interop.Sys.GetHostName()) { @@ -59,18 +59,6 @@ internal static string GetPipePath(string serverName, string pipeName, bool isCu // naming scheme used as that breaks the ability for code running on an older // runtime to connect to code running on the newer runtime. That means we're stuck // with a tmp file for the lifetime of the server stream. - if (isCurrentUserOnly) - { - string directory = Path.Combine(Path.GetTempPath(), ".NET" + Interop.Sys.GetEUid()); - Directory.CreateDirectory(directory); - if (Interop.Sys.ChMod(directory, (int)Interop.Sys.Permissions.S_IRWXU) == -1) - { - throw CreateExceptionForLastError(); - } - - return Path.Combine(directory, pipeName); - } - return s_pipePrefix + pipeName; } diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs index 9eb63d78f5517..fdcaa423ff35a 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs @@ -17,8 +17,7 @@ public abstract partial class PipeStream : Stream internal const bool CheckOperationsRequiresSetHandle = true; internal ThreadPoolBoundHandle _threadPoolBinding; - // In Windows we don't use isCurrentUserOnly flag for the path because we set an ACL to the pipe. - internal static string GetPipePath(string serverName, string pipeName, bool isCurrentUserOnly) + internal static string GetPipePath(string serverName, string pipeName) { string normalizedPipePath = Path.GetFullPath(@"\\" + serverName + @"\pipe\" + pipeName); if (String.Equals(normalizedPipePath, @"\\.\pipe\" + AnonymousPipeName, StringComparison.OrdinalIgnoreCase)) diff --git a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.Windows.cs b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.Windows.cs new file mode 100644 index 0000000000000..96d9d67897ca6 --- /dev/null +++ b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.Windows.cs @@ -0,0 +1,161 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Microsoft.Win32.SafeHandles; +using System.ComponentModel; +using System.DirectoryServices.AccountManagement; +using System.Runtime.InteropServices; +using System.Security.Cryptography; +using System.Security.Principal; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace System.IO.Pipes.Tests +{ + // Class to be used as xUnit fixture to avoid creating the user, an relatively slow operation (couple of seconds), multiple times. + public class TestAccountImpersonator : IDisposable + { + private const string TestAccountName = "CorFxTst0uZa"; // Random suffix to avoid matching any other account by accident, but const to avoid leaking it. + private SafeAccessTokenHandle _testAccountTokenHandle; + + public TestAccountImpersonator() + { + string testAccountPassword; + using (RandomNumberGenerator rng = new RNGCryptoServiceProvider()) + { + var randomBytes = new byte[33]; + rng.GetBytes(randomBytes); + + // Add special chars to ensure it satisfies password requirements. + testAccountPassword = Convert.ToBase64String(randomBytes) + "_-As@!%*(1)4#2"; + } + + DateTime accountExpirationDate = DateTime.UtcNow + TimeSpan.FromMinutes(2); + using (var principalCtx = new PrincipalContext(ContextType.Machine)) + { + bool needToCreate = false; + using (var foundUserPrincipal = UserPrincipal.FindByIdentity(principalCtx, TestAccountName)) + { + if (foundUserPrincipal == null) + { + needToCreate = true; + } + else + { + // Somehow the account leaked from previous runs, however, password is lost, reset it. + foundUserPrincipal.SetPassword(testAccountPassword); + foundUserPrincipal.AccountExpirationDate = accountExpirationDate; + foundUserPrincipal.Save(); + } + } + + if (needToCreate) + { + using (var userPrincipal = new UserPrincipal(principalCtx)) + { + userPrincipal.SetPassword(testAccountPassword); + userPrincipal.AccountExpirationDate = accountExpirationDate; + userPrincipal.Name = TestAccountName; + userPrincipal.DisplayName = TestAccountName; + userPrincipal.Description = TestAccountName; + userPrincipal.Save(); + } + } + } + + const int LOGON32_PROVIDER_DEFAULT = 0; + const int LOGON32_LOGON_INTERACTIVE = 2; + + if (!LogonUser(TestAccountName, ".", testAccountPassword, LOGON32_LOGON_INTERACTIVE, LOGON32_PROVIDER_DEFAULT, out _testAccountTokenHandle)) + { + _testAccountTokenHandle = null; + throw new Exception($"Failed to get SafeAccessTokenHandle for test account {TestAccountName}", new Win32Exception()); + } + } + + public void Dispose() + { + if (_testAccountTokenHandle == null) + return; + + _testAccountTokenHandle.Dispose(); + _testAccountTokenHandle = null; + + using (var principalCtx = new PrincipalContext(ContextType.Machine)) + using (var userPrincipal = UserPrincipal.FindByIdentity(principalCtx, TestAccountName)) + { + if (userPrincipal == null) + throw new Exception($"Failed to get user principal to delete test account {TestAccountName}"); + + try + { + userPrincipal.Delete(); + } + catch (InvalidOperationException) + { + // TODO: Investigate, it always throw this exception with "Can't delete object already deleted", but it actually deletes it. + } + } + } + + // This method asserts if it impersonates the current identity, i.e.: it ensures that an actual impersonation happens + public void RunImpersonated(Action action) + { + using (WindowsIdentity serverIdentity = WindowsIdentity.GetCurrent()) + { + WindowsIdentity.RunImpersonated(_testAccountTokenHandle, () => + { + using (WindowsIdentity clientIdentity = WindowsIdentity.GetCurrent()) + Assert.NotEqual(serverIdentity.Name, clientIdentity.Name); + + action(); + }); + } + } + + [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)] + private static extern bool LogonUser(string userName, string domain, string password, int logonType, int logonProvider, out SafeAccessTokenHandle safeAccessTokenHandle); + } + + /// + /// Tests for the constructors for NamedPipeClientStream + /// + public class NamedPipeTest_CurrentUserOnly_Windows : NamedPipeTestBase, IClassFixture + { + private TestAccountImpersonator _testAccountImpersonator; + + public NamedPipeTest_CurrentUserOnly_Windows(TestAccountImpersonator testAccountImpersonator) + { + _testAccountImpersonator = testAccountImpersonator; + } + + [OuterLoop] + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsWindowsAndElevated))] + [InlineData(PipeOptions.None, PipeOptions.CurrentUserOnly)] + [InlineData(PipeOptions.CurrentUserOnly, PipeOptions.None)] + public void Connection_UnderDifferentUser_SingleSide_CurrentUserOnly_Fails(PipeOptions serverPipeOptions, PipeOptions clientPipeOptions) + { + string name = GetUniquePipeName(); + using (var cts = new CancellationTokenSource()) + using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, serverPipeOptions | PipeOptions.Asynchronous)) + { + Task serverTask = server.WaitForConnectionAsync(cts.Token); + + _testAccountImpersonator.RunImpersonated(() => + { + using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, clientPipeOptions)) + { + Assert.Throws(() => client.Connect()); + } + }); + + // Server is expected to not have received any request. + cts.Cancel(); + var e = Assert.Throws(() => serverTask.Wait(10_000)); + Assert.IsType(typeof(TaskCanceledException), e.InnerException); + } + } + } +} diff --git a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.cs b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.cs index f02976511e4ba..a267629715f6c 100644 --- a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.cs +++ b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.cs @@ -41,17 +41,22 @@ public static void CreateServer_ConnectClient() } } - [Fact] - [PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix domain socket should have different location in this case. - public static void CreateServerNotCurrentUserOnly_ClientCurrentUserOnly_ThrowsTimeout_OnUnix() + [Theory] + [InlineData(PipeOptions.None, PipeOptions.CurrentUserOnly)] + [InlineData(PipeOptions.CurrentUserOnly, PipeOptions.None)] + public static void Connection_UnderSameUser_SingleSide_CurrentUserOnly_Works(PipeOptions serverPipeOptions, PipeOptions clientPipeOptions) { string name = GetUniquePipeName(); - using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte)) + using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, serverPipeOptions)) + using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, clientPipeOptions)) { - using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, PipeOptions.CurrentUserOnly)) - { - Assert.Throws(() => client.Connect(1)); - } + Task[] tasks = new[] + { + Task.Run(() => server.WaitForConnection()), + Task.Run(() => client.Connect()) + }; + + Assert.True(Task.WaitAll(tasks, 20_000)); } } @@ -99,4 +104,4 @@ public static void CreateMultipleServers_ConnectMultipleClients_MultipleThreads( Task.WaitAll(tasks.ToArray()); } } -} \ No newline at end of file +} diff --git a/src/libraries/System.IO.Pipes/tests/System.IO.Pipes.Tests.csproj b/src/libraries/System.IO.Pipes/tests/System.IO.Pipes.Tests.csproj index ebe1e38a1c5bd..b0f18d7e12e05 100644 --- a/src/libraries/System.IO.Pipes/tests/System.IO.Pipes.Tests.csproj +++ b/src/libraries/System.IO.Pipes/tests/System.IO.Pipes.Tests.csproj @@ -15,8 +15,6 @@ - - @@ -24,6 +22,7 @@ + @@ -32,6 +31,7 @@ + @@ -42,6 +42,9 @@ + + + @@ -58,4 +61,4 @@ - \ No newline at end of file +