Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Duplicate the access token passed to WindowsIdentity.RunImpersonated #30346

Merged
stephentoub merged 5 commits into from
Jun 13, 2018
Merged

Duplicate the access token passed to WindowsIdentity.RunImpersonated #30346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e59284c
Duplicate the access token passed to WindowsIdentity.RunImpersonated
Jun 13, 2018
bbba5d3
Small fix
Jun 13, 2018
e2b1cdb
Address feedback
Jun 13, 2018
fcc1358
Change to use ref counting, remove assert (no null check on access to…
Jun 13, 2018
ba6150e
Manual add/release
Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,46 @@ 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);
Copy link
Member

@stephentoub stephentoub Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why SecurityException? Is that what netfx throws?

Copy link
Member

@stephentoub stephentoub Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, you just moved the code from below.

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Netfx actually creates/saves the current WindowsIdentity after impersonation, but yes any failure to duplicate the access token results in a SecurityException in Netfx.

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in Core in any of the WindowsIdentity constructors that take an IntPtr access token

}

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();
}
}
Copy link
Member

@stephentoub stephentoub Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than declaring the DllImport twice, you could also just do the AddRef/Release yourself:

SafeAccessTokenHandle duplicateAccessToken;
bool success = false;
try
{
    accessToken.DangerousAddRef(ref success);
    duplicateAccessToken = DuplicateAccessToken(accessToken.DangerousGetHandle());
}
finally
{
    if (success) accessToken.DangerousRelease();
}
return duplicateAccessToken;

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of the bool return? It seems like it always either returns true or throws.

Copy link
Member

@stephentoub stephentoub Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the ref bool argument to Monitor.Enter: reliability on platforms with thread aborts.

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if success is false after DangerousAddRef? Should it throw ObjectDisposedException? Maybe I can just completely ignore the return value.

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see

Copy link
Member

@stephentoub stephentoub Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if success is false after DangerousAddRef?

It won't be: as you said, it'll either be true or DangerousAddRef will throw. The ref bool is there to support thread aborts. On systems with thread aborts, you put the DangerousAddRef call inside the try, and then if there's an exception, the finally can look at the bool to determine whether it needs to DangerousRelease. If the DangerousAddRef were outside the try, a thread abort could occur after a successful AddRef but before entering the try, and the SafeHandle would be leaked.

Maybe I can just completely ignore the return value.

On .NET Core, you can. Technically aborts are still possible from the debugger, but that's not a key reliability scenario.

}

private void CreateFromToken(IntPtr userToken)
{
Expand All @@ -222,14 +262,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);
Copy link
Member

@stephentoub stephentoub Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the new handle get disposed of correctly eventually?

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handle will be closed once the GC determines there are no more references and the SafeHandle is finalized

Copy link
Member

@stephentoub stephentoub Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're depending on finalization. Is that how things work in netfx, too?

Copy link
Author

@kouvel kouvel Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, there's no other clear way of identifying the proper lifetime for the handle from the API side

}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2229", Justification = "Public API has already shipped.")]
Expand Down Expand Up @@ -640,6 +673,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);
Expand Down
5 changes: 5 additions & 0 deletions src/System.Security.Principal.Windows/tests/System.Security.Principal.Windows.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
54 changes: 54 additions & 0 deletions src/System.Security.Principal.Windows/tests/WindowsIdentityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -120,6 +124,56 @@ 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.CheckedWait();
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);
Expand Down