[release/2.1] Duplicate the access token passed to WindowsIdentity.RunImpersonated (#30346) (#30379)

* Duplicate the access token passed to WindowsIdentity.RunImpersonated (#30346)

So that callbacks for async work initiated while impersonated may continue to impersonate even after the original access token had been disposed.

Port of #30346 and #30377 to release/2.1
Fixes https://github.com/dotnet/corefx/issues/30275

* Small fix

* Address feedback

* Change to use ref counting, remove assert (no null check on access token parameter)

* Manual add/release

* Add test

* Change test

Author: kouvel

src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs

@@ -209,27 +209,68 @@ private WindowsIdentity(IntPtr userToken, string authType, int isAuthenticated)
             _isAuthenticated = isAuthenticated;
         }
 
-
-        private void CreateFromToken(IntPtr userToken)
+        private static SafeAccessTokenHandle DuplicateAccessToken(IntPtr accessToken)
         {
-            if (userToken == IntPtr.Zero)
+            if (accessToken == IntPtr.Zero)
+            {
                 throw new ArgumentException(SR.Argument_TokenZero);
+            }
 
             // Find out if the specified token is a valid.
-            uint dwLength = (uint)sizeof(uint);
-            bool result = Interop.Advapi32.GetTokenInformation(userToken, (uint)TokenInformationClass.TokenType,
-                                                          SafeLocalAllocHandle.InvalidHandle, 0, out dwLength);
-            if (Marshal.GetLastWin32Error() == Interop.Errors.ERROR_INVALID_HANDLE)
+            uint dwLength = sizeof(uint);
+            if (!Interop.Advapi32.GetTokenInformation(
+                    accessToken,
+                    (uint)TokenInformationClass.TokenType,
+                    IntPtr.Zero,
+                    0,
+                    out dwLength) &&
+                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))
+            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)
+        {
+            if (accessToken.IsInvalid)
+            {
+                return accessToken;
+            }
+
+            bool refAdded = false;
+            try
+            {
+                accessToken.DangerousAddRef(ref refAdded);
+                return DuplicateAccessToken(accessToken.DangerousGetHandle());
+            }
+            finally
+            {
+                if (refAdded)
+                {
+                    accessToken.DangerousRelease();
+                }
+            }
+        }
+
+        private void CreateFromToken(IntPtr userToken)
+        {
+            _safeTokenHandle = DuplicateAccessToken(userToken);
         }
 
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2229", Justification = "Public API has already shipped.")]
@@ -640,6 +681,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);

src/System.Security.Principal.Windows/tests/System.Security.Principal.Windows.Tests.csproj

@@ -14,5 +14,10 @@
       <Link>Common\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs</Link>
     </Compile>
   </ItemGroup>
+  <ItemGroup>
+    <Compile Include="$(CommonTestPath)\System\Threading\ThreadTestHelpers.cs">
+      <Link>CommonTest\System\Threading\ThreadTestHelpers.cs</Link>
+    </Compile>
+  </ItemGroup>
   <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
 </Project>

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,70 @@ public static void CheckUserClaims()
         }
     }
 
+    [Fact]
+    public static void RunImpersonatedTest_InvalidHandle()
+    {
+        using (var mutex = new Mutex())
+        {
+            Assert.Throws<ArgumentException>(() =>
+            {
+                WindowsIdentity.RunImpersonated(
+                    new SafeAccessTokenHandle(mutex.SafeWaitHandle.DangerousGetHandle()),
+                    () => { });
+            });
+        }
+    }
+
+    [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(ThreadTestHelpers.UnexpectedTimeoutMilliseconds);
+        if (testData.exception != null)
+        {
+            throw new AggregateException(testData.exception);
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void BeginTask(RunImpersonatedAsyncTestInfo testInfo)
+    {
+        testInfo.continueTask = new SemaphoreSlim(0, 1);
+        using (SafeAccessTokenHandle token = WindowsIdentity.GetCurrent().AccessToken)
+        {
+            WindowsIdentity.RunImpersonated(token, () =>
+            {
+                testInfo.task = Task.Run(async () =>
+                {
+                    try
+                    {
+                        Task<bool> 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);