From e59284c9d4785146ed5a98c0fd7edd5775a4e174 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Tue, 12 Jun 2018 17:41:25 -0700 Subject: [PATCH 1/5] Duplicate the access token passed to WindowsIdentity.RunImpersonated So that callbacks for async work initiated while impersonated may continue to impersonate even after the original access token had been disposed. Fix for https://github.com/dotnet/corefx/issues/30275 --- .../Security/Principal/WindowsIdentity.cs | 43 ++++++++++++--- ...em.Security.Principal.Windows.Tests.csproj | 5 ++ .../tests/WindowsIdentityTests.cs | 55 +++++++++++++++++++ 3 files changed, 95 insertions(+), 8 deletions(-) diff --git a/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs b/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs index 920cf1214c08..a5ba688002f0 100644 --- a/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs +++ b/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs @@ -209,6 +209,38 @@ private WindowsIdentity(IntPtr userToken, string authType, int isAuthenticated) _isAuthenticated = isAuthenticated; } + private static SafeAccessTokenHandle DuplicateAccessToken(IntPtr accessToken) + { + SafeAccessTokenHandle duplicateAccessToken = SafeAccessTokenHandle.InvalidHandle; + IntPtr currentProcessHandle = Interop.Kernel32.GetCurrentProcess(); + if (!Interop.Kernel32.DuplicateHandle( + currentProcessHandle, + accessToken, + currentProcessHandle, + ref duplicateAccessToken, + 0, + true, + Interop.DuplicateHandleOptions.DUPLICATE_SAME_ACCESS)) + { + throw new SecurityException(new Win32Exception().Message); + } + + return duplicateAccessToken; + } + + private static SafeAccessTokenHandle DuplicateAccessToken(SafeAccessTokenHandle accessToken) + { + Debug.Assert(accessToken != null); + + if (accessToken.IsInvalid) + { + return accessToken; + } + + SafeAccessTokenHandle duplicateAccessToken = DuplicateAccessToken(accessToken.DangerousGetHandle()); + GC.KeepAlive(accessToken); + return duplicateAccessToken; + } private void CreateFromToken(IntPtr userToken) { @@ -222,14 +254,7 @@ private void CreateFromToken(IntPtr userToken) if (Marshal.GetLastWin32Error() == Interop.Errors.ERROR_INVALID_HANDLE) throw new ArgumentException(SR.Argument_InvalidImpersonationToken); - if (!Interop.Kernel32.DuplicateHandle(Interop.Kernel32.GetCurrentProcess(), - userToken, - Interop.Kernel32.GetCurrentProcess(), - ref _safeTokenHandle, - 0, - true, - Interop.DuplicateHandleOptions.DUPLICATE_SAME_ACCESS)) - throw new SecurityException(new Win32Exception().Message); + _safeTokenHandle = DuplicateAccessToken(userToken); } [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2229", Justification = "Public API has already shipped.")] @@ -640,6 +665,8 @@ public void Dispose() private static void RunImpersonatedInternal(SafeAccessTokenHandle token, Action action) { + token = DuplicateAccessToken(token); + bool isImpersonating; int hr; SafeAccessTokenHandle previousToken = GetCurrentToken(TokenAccessLevels.MaximumAllowed, false, out isImpersonating, out hr); diff --git a/src/System.Security.Principal.Windows/tests/System.Security.Principal.Windows.Tests.csproj b/src/System.Security.Principal.Windows/tests/System.Security.Principal.Windows.Tests.csproj index f0daf2c1b900..ab70abfa35fb 100644 --- a/src/System.Security.Principal.Windows/tests/System.Security.Principal.Windows.Tests.csproj +++ b/src/System.Security.Principal.Windows/tests/System.Security.Principal.Windows.Tests.csproj @@ -14,5 +14,10 @@ Common\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs + + + CommonTest\System\Threading\ThreadTestHelpers.cs + + \ No newline at end of file diff --git a/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs b/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs index 5445388cd139..002bbb4ec716 100644 --- a/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs +++ b/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs @@ -5,8 +5,12 @@ using Microsoft.Win32.SafeHandles; using System; using System.Linq; +using System.Runtime.CompilerServices; using System.Security.Claims; using System.Security.Principal; +using System.Threading; +using System.Threading.Tasks; +using System.Threading.Tests; using Xunit; public class WindowsIdentityTests @@ -120,6 +124,57 @@ public static void CheckUserClaims() } } + [Fact] + public static void RunImpersonatedAsyncTest() + { + var testData = new RunImpersonatedAsyncTestInfo(); + BeginTask(testData); + + // Wait for the SafeHandle that was disposed in BeginTask() to actually be closed + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.WaitForPendingFinalizers(); + + testData.continueTask.Release(); + testData.task.Wait(); + if (testData.exception != null) + { + throw new AggregateException(testData.exception); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void BeginTask(RunImpersonatedAsyncTestInfo testInfo) + { + testInfo.task = null; + testInfo.continueTask = new SemaphoreSlim(0, 1); + using (SafeAccessTokenHandle token = WindowsIdentity.GetCurrent().AccessToken) + { + WindowsIdentity.RunImpersonated(token, () => + { + testInfo.task = Task.Run(async () => + { + try + { + Task task = testInfo.continueTask.WaitAsync(ThreadTestHelpers.UnexpectedTimeoutMilliseconds); + Assert.True(await task.ConfigureAwait(false)); + } + catch (Exception ex) + { + testInfo.exception = ex; + } + }); + }); + } + } + + private class RunImpersonatedAsyncTestInfo + { + public Task task; + public SemaphoreSlim continueTask; + public Exception exception; + } + private static void CheckDispose(WindowsIdentity identity, bool anonymous = false) { Assert.False(identity.AccessToken.IsClosed); From bbba5d3f11e356fd0c2204ff945048cc9e6c6115 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Tue, 12 Jun 2018 18:34:15 -0700 Subject: [PATCH 2/5] Small fix --- .../tests/WindowsIdentityTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs b/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs index 002bbb4ec716..30a139953162 100644 --- a/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs +++ b/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs @@ -136,7 +136,7 @@ public static void RunImpersonatedAsyncTest() GC.WaitForPendingFinalizers(); testData.continueTask.Release(); - testData.task.Wait(); + testData.task.CheckedWait(); if (testData.exception != null) { throw new AggregateException(testData.exception); From e2b1cdb8aebbd3b40c0997ef9243730a67900931 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Tue, 12 Jun 2018 18:59:41 -0700 Subject: [PATCH 3/5] Address feedback --- .../tests/WindowsIdentityTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs b/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs index 30a139953162..95859cd8eee7 100644 --- a/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs +++ b/src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs @@ -146,7 +146,6 @@ public static void RunImpersonatedAsyncTest() [MethodImpl(MethodImplOptions.NoInlining)] private static void BeginTask(RunImpersonatedAsyncTestInfo testInfo) { - testInfo.task = null; testInfo.continueTask = new SemaphoreSlim(0, 1); using (SafeAccessTokenHandle token = WindowsIdentity.GetCurrent().AccessToken) { From fcc1358e197ab4aa657e8bd87213c646f2df0f26 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 13 Jun 2018 06:42:16 -0700 Subject: [PATCH 4/5] Change to use ref counting, remove assert (no null check on access token parameter) --- .../kernel32/Interop.DuplicateHandle.cs | 10 ++++++++++ .../Security/Principal/WindowsIdentity.cs | 18 ++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs b/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs index 3fca3c827022..6373c7a11cad 100644 --- a/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs +++ b/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs @@ -19,5 +19,15 @@ internal static extern bool DuplicateHandle( uint dwDesiredAccess, bool bInheritHandle, uint dwOptions); + + [DllImport(Interop.Libraries.Kernel32, SetLastError = true)] + internal static extern bool DuplicateHandle( + IntPtr hSourceProcessHandle, + SafeAccessTokenHandle hSourceHandle, + IntPtr hTargetProcessHandle, + ref SafeAccessTokenHandle lpTargetHandle, + uint dwDesiredAccess, + bool bInheritHandle, + uint dwOptions); } } diff --git a/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs b/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs index a5ba688002f0..18cff7d9651c 100644 --- a/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs +++ b/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs @@ -230,15 +230,25 @@ private static SafeAccessTokenHandle DuplicateAccessToken(IntPtr accessToken) private static SafeAccessTokenHandle DuplicateAccessToken(SafeAccessTokenHandle accessToken) { - Debug.Assert(accessToken != null); - if (accessToken.IsInvalid) { return accessToken; } - SafeAccessTokenHandle duplicateAccessToken = DuplicateAccessToken(accessToken.DangerousGetHandle()); - GC.KeepAlive(accessToken); + SafeAccessTokenHandle duplicateAccessToken = SafeAccessTokenHandle.InvalidHandle; + IntPtr currentProcessHandle = Interop.Kernel32.GetCurrentProcess(); + if (!Interop.Kernel32.DuplicateHandle( + currentProcessHandle, + accessToken, + currentProcessHandle, + ref duplicateAccessToken, + 0, + true, + Interop.DuplicateHandleOptions.DUPLICATE_SAME_ACCESS)) + { + throw new SecurityException(new Win32Exception().Message); + } + return duplicateAccessToken; } From ba6150e691cf5b7cec73e2451113850fbf3c1553 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Wed, 13 Jun 2018 07:06:13 -0700 Subject: [PATCH 5/5] Manual add/release --- .../kernel32/Interop.DuplicateHandle.cs | 10 -------- .../Security/Principal/WindowsIdentity.cs | 24 +++++++++---------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs b/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs index 6373c7a11cad..3fca3c827022 100644 --- a/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs +++ b/src/Common/src/Interop/Windows/kernel32/Interop.DuplicateHandle.cs @@ -19,15 +19,5 @@ internal static extern bool DuplicateHandle( uint dwDesiredAccess, bool bInheritHandle, uint dwOptions); - - [DllImport(Interop.Libraries.Kernel32, SetLastError = true)] - internal static extern bool DuplicateHandle( - IntPtr hSourceProcessHandle, - SafeAccessTokenHandle hSourceHandle, - IntPtr hTargetProcessHandle, - ref SafeAccessTokenHandle lpTargetHandle, - uint dwDesiredAccess, - bool bInheritHandle, - uint dwOptions); } } diff --git a/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs b/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs index 18cff7d9651c..4ceed132dabc 100644 --- a/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs +++ b/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs @@ -235,21 +235,19 @@ private static SafeAccessTokenHandle DuplicateAccessToken(SafeAccessTokenHandle return accessToken; } - SafeAccessTokenHandle duplicateAccessToken = SafeAccessTokenHandle.InvalidHandle; - IntPtr currentProcessHandle = Interop.Kernel32.GetCurrentProcess(); - if (!Interop.Kernel32.DuplicateHandle( - currentProcessHandle, - accessToken, - currentProcessHandle, - ref duplicateAccessToken, - 0, - true, - Interop.DuplicateHandleOptions.DUPLICATE_SAME_ACCESS)) + bool refAdded = false; + try { - throw new SecurityException(new Win32Exception().Message); + accessToken.DangerousAddRef(ref refAdded); + return DuplicateAccessToken(accessToken.DangerousGetHandle()); + } + finally + { + if (refAdded) + { + accessToken.DangerousRelease(); + } } - - return duplicateAccessToken; } private void CreateFromToken(IntPtr userToken)