Repository navigation
Reversal of WindowsIdentity.Impersonate does not always work #260
Description
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:
- Create two local users on a machine: usr1 and usr2.
- Start PowerShell.
- Get credentials for usr1 and usr2:
$cred1 = Get-Credential usr1
$cred2 = Get-Credential usr2 - Run the following command until the exception is thrown:
Invoke-WebRequest http://localhost:21433/api/yield -Credential ($cred1 or $cred2)
Activity
avivanoff commentedon Sep 24, 2019
Most likely this issue has the same root problem as #255.
hazz0003 commentedon Sep 24, 2019
I am seeing this too!
hazz0003 commentedon Sep 25, 2019
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 commentedon Sep 26, 2019
Also, running the same code using raw System.Net.HttpListener has no issues. So the problem is definitely with AspNetWebStack.
Tratcher commentedon Oct 18, 2019
Closing as a duplicate of https://developercommunity.visualstudio.com/content/problem/753482/windowsidentityimpersonate-in-some-async-schenario.html
hazz0003 commentedon Oct 18, 2019
It looks like framework bug (full and core)
mattjohnsonpint commentedon Oct 19, 2019
FWIW, if I replace this part (from the console app @Tratcher linked):
With this instead:
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 commentedon Oct 19, 2019
RunImpersonated is a synchronous method, it cannot be used to run asynchronous action/function.
weltkante commentedon Oct 20, 2019
@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 commentedon Oct 20, 2019
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 commentedon Oct 20, 2019
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 commentedon Oct 20, 2019
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 commentedon Oct 20, 2019
@weltkante, This repo is for full framework. There is a different one for AspNetCore.
weltkante commentedon Oct 21, 2019
@avivanoff if you meant to link to an issue that didn't work