Duplicate the access token passed to WindowsIdentity.RunImpersonated

[kouvel]

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

[stephentoub]

Why SecurityException? Is that what netfx throws?

[stephentoub]

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

[kouvel]

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

[kouvel]

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

[stephentoub]

Should we do the whole DangerousAddRef/DangerousRelease dance instead?

[kouvel]

Could do that too, this is what is done elsewhere and I believe it suffices and is cheaper

[stephentoub]

this is what is done elsewhere

From what I can see, netfx passes the SafeHandle to the P/Invoke:
https://referencesource.microsoft.com/#mscorlib/system/security/principal/windowsimpersonationcontext.cs,47
and it will do the addref/release as part of that marshaling.

I believe it suffices and is cheaper

I'm not convinced it's sufficient. The KeepAlive prevents the access token from being finalized while it's being used in the P/Invoke, but it doesn't prevent Dispose from being called on the SafeHandle. This SafeHandle was passed in by the developer, so concurrent with this operation, they could be calling Dispose on it. If that Dispose happened after the IsInvalid checks, it could be closing the handle prior to or during the DuplicateHandle operation, which then also makes it susceptible to handle recycling.

[kouvel]

From the API perspective, a concurrent dispose is something the caller is responsible to prevent. That can also happen with the solution you suggest.

[stephentoub]

That can also happen with the solution you suggest.

If the SafeHandle is disposed concurrently, with the solution I suggest it is guaranteed that either the dispose will happen prior to the P/Invoke and result in an exception being thrown then, or that the disposal will be delayed until after the P/Invoke returns. It would prevent handle recycling and other such concurrency bugs.

[kouvel]

If the SafeHandle is disposed concurrently, with the solution I suggest it is guaranteed that either the dispose will happen prior to the P/Invoke and result in an exception being thrown then

The handle may also be recycled before the P/Invoke, in which case the DuplicateHandle P/Invoke may not result in an exception.

[stephentoub]

I think we're talking past each other.

SafeHandle completely protects the underlying handle against recycling. If it's disposed of, any subsequent attempts to use it will result in exception, e.g. passing a disposed SafeHandle to a P/Invoke will throw because the P/Invoke will call DangerousAddRef, which will use an interlocked to try to get a ref on the SafeHandle and will fail because it was already disposed. And if it's currently being used in a P/Invoke, any disposals will be delayed until the P/Invoke completes, because the disposal will only take effect the moment the ref count drops to 0 when the P/Invoke marshaling layer invokes DangerousRelease.

I think this is potentially quite impactful here. We're talking about security and impersonation. Imagine this is in an ASP.NET web site, where every request is impersonating the caller, and that impersonation is then used to determine, for example, whether a particular file can be opened. And let's say the code has a bug where the AccessToken is disposed of concurrently with this operation. If a SafeHandle is used appropriately, either passed to a P/Invoke so that AddRef/Release are used or with the code using AddRef/Release directly, then there is no way the DuplicateHandle call could end up with a recycled handle: either disposal of the SafeHandle would happen prior to the AddRef, in which case AddRef will throw, or the disposal won't happen until after DuplicateHandle has completed. But with an IntPtr and not using AddRef/Release, the disposal could free the handle, another unrelated request could create an AccessToken for a completely different user with different privileges and with the same handle value, and then this DuplicateHandle call could end up duplicating that other handle with greater privileges... then this request proceeds happily thinking it has a handle that represents one user when it reality it represents a completely different user.

[kouvel]

Responding to myself:

Ref counting does not solve that problem, as the dispose may happen after the SubRef (or equivalent), at which point the handle is invalid and any attempt to do anything meaningful on it later would fail in the same way.

That's not quite right, if the dispose happens after the duplicate then it's fine because only the (still valid) duplicate would be used later. The issue is when the dispose happens before (either before the AddRef or before the actual duplication, between which there is no difference).

[stephentoub]

The issue is when the dispose happens before (either before the AddRef or before the actual duplication, between which there is no difference).

There is a difference. If dispose happens before the AddRef, then AddRef will throw an exception. If it happens after the AddRef, then the disposal will be delayed until the Release after the P/Invoke. Either way, it's guaranteed that if the P/Invoke is called, it'll be on the right and open handle.

[kouvel]

I see, I missed that the implied AddRef would throw. That's a fair point, I'll change it.

[stephentoub]

Nit: This could just be a TaskCompletionSource.

[kouvel]

I tried this:

    [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.continueTaskSource.SetResult(true);
        testData.task.CheckedWait();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void BeginTask(RunImpersonatedAsyncTestInfo testInfo)
    {
        using (SafeAccessTokenHandle token = WindowsIdentity.GetCurrent().AccessToken)
        {
            WindowsIdentity.RunImpersonated(token, () =>
                testInfo.task = Task.Run(async () => await testInfo.continueTaskSource.Task.ConfigureAwait(false)));
        }
    }

    private class RunImpersonatedAsyncTestInfo
    {
        public Task task;
        public TaskCompletionSource<bool> continueTaskSource;

        public RunImpersonatedAsyncTestInfo()
        {
            continueTaskSource = new TaskCompletionSource<bool>();
        }
    }

But it doesn't fail in the baseline, don't know why. Adding a Thread.Sleep(1000) before the wait makes it fail. I'm going to leave it as is for now, the current code also allows for unexpected timeouts to be observed and fail the test instead of hanging (though there may be another way to do that).

[stephentoub]

In what situation is it not already null?

[kouvel]

Leftover from another version of the code, will remove

[stephentoub]

Does the new handle get disposed of correctly eventually?

[kouvel]

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

[stephentoub]

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

[kouvel]

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

[stephentoub]

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;

[kouvel]

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

[stephentoub]

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

[kouvel]

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

[kouvel]

Ah I see

[stephentoub]

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.