Reversal of WindowsIdentity.Impersonate does not always work

[avivanoff]

Lets say you have the following controller:

[Authorize]
[RoutePrefix("yield")]
public sealed class YieldController : ApiController
{
    [HttpGet]
    [Route]
    public async Task<IHttpActionResult> FindAsync()
    {
        try
        {

            using (var threadIdentity = WindowsIdentity.GetCurrent(true))
            {
                if (threadIdentity != null)
                {
                    throw new InvalidOperationException($"Must not be impersonating {threadIdentity.Name} at the beginning of the request");
                }
            }

            var currentWindowsIdentity = (WindowsIdentity)this.User.Identity;
            Trace.WriteLine($"{Environment.UserName} ManagedThreadId({Thread.CurrentThread.ManagedThreadId})");

            await Task.Yield();

            using (currentWindowsIdentity.Impersonate())
            {
                Trace.WriteLine($"{Environment.UserName} ManagedThreadId({Thread.CurrentThread.ManagedThreadId})");
                await Task.Yield();
            }

            using (var threadIdentity = WindowsIdentity.GetCurrent(true))
            {
                if (threadIdentity != null)
                {
                    throw new InvalidOperationException($"Must not be impersonating {threadIdentity.Name} after disposing impersonation context");
                }
            }

            Trace.WriteLine($"{Environment.UserName} ManagedThreadId({Thread.CurrentThread.ManagedThreadId})");
        }
        catch (Exception ex)
        {
            Trace.TraceError(ex.ToString());

            throw;
        }

        return this.Ok();
    }

Why does this code periodically hits throw new InvalidOperationException($"Must not be impersonating {threadIdentity.Name} after disposing impersonation context")? This looks like a serious security hole, because the code in my test after impersonation context is disposed sometimes was running as some user that made the request before.

The sample code is attached. And this is how I reproduce the issue:

1. Create two local users on a machine: usr1 and usr2.
2. Start PowerShell.
3. Get credentials for usr1 and usr2: $cred1 = Get-Credential usr1 $cred2 = Get-Credential usr2
4. Run the following command until the exception is thrown: Invoke-WebRequest http://localhost:21433/api/yield -Credential ($cred1 or $cred2)

AsyncKatana.zip

[avivanoff]

Most likely this issue has the same root problem as #255.

[hazz0003]

I am seeing this too!

[hazz0003]

It has now been reproduced outside of Katana. In fact, in a stand alone console desktop app.

In case it isn't obvious from the example, this is an escalation of privileged issue. User B might randomly get to act as User A.

[avivanoff]

Also, running the same code using raw System.Net.HttpListener has no issues. So the problem is definitely with AspNetWebStack.

[Tratcher]

Closing as a duplicate of https://developercommunity.visualstudio.com/content/problem/753482/windowsidentityimpersonate-in-some-async-schenario.html

[hazz0003]

It looks like framework bug (full and core)

[mattjohnsonpint]

FWIW, if I replace this part (from the console app @Tratcher linked):

using (var identity = WindowsIdentity.GetCurrent())
using (identity.Impersonate())
{
    await Task.Yield();
}

With this instead:

using (var identity = WindowsIdentity.GetCurrent())
{
    await WindowsIdentity.RunImpersonated(identity.AccessToken, async () =>
    {
        await Task.Yield();
    });
}

Then it no longer throws the exception. I still think there's an issue, but perhaps there are two separate issues going on here.

[avivanoff]

RunImpersonated is a synchronous method, it cannot be used to run asynchronous action/function.

[weltkante]

@avivanoff it can, by installing an AsyncLocal with a callback to be notified whenever the asynchronous method suspends/resumes. In addition calling the delegate through ExecutionContext.Run can be used to prevent leaking the AsyncLocal to the caller, allowing for isolation to just the control flow within the delegate.

In Desktop Framework something similar happens, but the implementation is entirely different since everything is hardcoded in the framework and not going through the AsyncLocal machinery (didn't exist when it was written).

[avivanoff]

How am I supposed to know that RunImpersonated uses AsyncLocal internally and it is ok to pass in asynchronous delegate? Does documentation say it? Or is it OK now to pass in async delegates anywhere Func is expected, assuming the method internally uses AsyncLocal or whatever else?

[weltkante]

When something says it "flows execution context" then it doesn't matter whether you pass synchronous or asynchronous delegates, the async delegate will correctly propagate the execution context. Whether its doing that via AsyncLocal (.NET Core) or something else (Desktop Framework) is just an implementation detail.

The fact that the documentation doesn't specify that impersonation is flown with execution context I would consider to be a documentation problem, at this point you can only rely on it by looking at the implementation. Considering that both Desktop and Core flow execution context I don't see any issue with documenting it instead of treating it as an implementation detail.

[weltkante]

Took me a while to figure out why this was closed, so as reference for future readers trying to follow up (and in response to @hazz0003): the "Impersonate" call from this issue doesn't exist in .NET Core so there is nothing to fix in this repo, so this was closed with reference to the equivalent bug on Desktop Framework (which at this point is still under investigation). Any similar bugs using API available on Core Framework should probably be reported separately from this issue.

[avivanoff]

@weltkante, This repo is for full framework. There is a different one for AspNetCore.

[weltkante]

@avivanoff if you meant to link to an issue that didn't work