Safely Impersonating Another User


Yesterday I posted a bit of code that shows how to impersonate another user in managed code.  However, that code had a subtle security hole waiting to bite you if you used it directly.  Both Dean and Eric found the problem.  In fact Eric reminded me of a blog entry he wrote on the same subject last fall.

The basic problem is that unlike most CLR security settings, impersonation is tied to the executing thread and not to the stack frame.  This means that if malicious code sees that your code is going to impersonate a privileged user, and then call into a method that it can cause to throw an exception, that malicious code can catch the exception and be running as the privileged user in its exception handler.

Steve suggested wrapping the entire impersonate / Undo operation in a simple utility object that implements IDisposable.  That way you could use the C# using construct and have the Dispose method undo impersonation if the stack frame was popped of during an exception unwind.  At first brush this seems like a really good idea, however some deeper analysis shows that even this won’t stop a determined malicious assembly.

How’s that?  CLR exception handling takes place in two passes.  On the first pass, the CLR walks down the call stack checking for a frame willing to handle the exception.  In C# this means that the type specified in your catch block matches the type of the exception.  However, in VB or IL you can write an exception filter, which is a block of arbitrary code that executes to determine if the exception handler should run.

Once the stack frame containing the exception handler is identified, the CLR returns back to the top of the stack to begin its second pass of exception handling.  On this step, finally and fault blocks are executed and then the frame is popped until the handling frame is reached.

So if we put our Undo call in the finally block, it won’t get called until the second pass of exception handling.  That means any malicious code that can execute on the first pass, by implementing an exception filter, will still run in the impersonated context, regardless of the best intentions of our finally block.

What’s the solution?  If you’re coding in C#, the only way to be called on the first pass of exception handling is to actually catch the exception.  That means that there is unfortunately nothing much more elegant than:

// Begin impersonating the user
WindowsImpersonationContext impersonationContext = WindowsIdentity.Impersonate(userHandle.Token);

try
{
    DoSomeWorkWhileImpersonating();
}
catch
{
    // Clean up
    if(userHandle != IntPtr.Zero)
    {
      CloseHandle(userHandle);
      userHandle = IntPtr.Zero;
    }

    if(impersonationContext != null)
    {
      impersonationContext.Undo();
      impersonationContext = null;
    }

    // allow the exception to continue on its way
    throw;
}
finally
{
    // Clean up
    if(userHandle != IntPtr.Zero)
      CloseHandle(userHandle);

    if(impersonationContext != null)
      impersonationContext.Undo();
}

Notice that we use a plain catch, since ECMA allows for objects not derived from Exception to be thrown.  We also use a throw operation to cause the IL rethrow opcode to be emitted, which limits the amount of interfering we did with the exception handling process.

In VB you can be a little more slick and use an exception filter:

‘ Begin impersonating the user
WindowsImpersonationContext impersonationContext = WindowsIdentity.Impersonate(userHandle.Token)

Try

    DoSomeWorkWhileImpersonating()

Catch When UndoImpersonation(userHandle, impersonationContext) = False

    Debug.Assert(False, “CleanUp returned False”)
Finally

    UndoImpersonation(userHandle, impersonationContext)
End Try

‘ …

Private Function UndoImpersonation(ByRef userHandle As IntPtr, ByRef impersonationContext As WindowsImpersonationContext) As Boolean
  If Not IntPtr.Zero.Equals(userHandle) Then
    CloseHandle(userHandle)
    userHandle = IntPtr.Zero
  End If
  If impersonationContext <> Nothing Then
    impersonationContext.Undo()
    impersonationContext = Nothing
  End If
  Return True
End Function

Here, we undo the impersonation in the exception filter, but never actually catch the exception, since the filter will never evalutate to True.  It’s a nicer approach than the C# method, since by not catching the exception we make debugging a lot easier.

Both approaches are ugly, but in order to be safe you’ll need to implement at least one.  The lesson here is that coding when you don’t necessarily trust your callers is difficult.  Throw in the fact that exception safe coding is also difficult to do properly, and you’ve got yourself into a very tricky situation.  Tomorrow, we’ll look at using some new Whidbey features to make the whole process a lot nicer and safer.

Updated 3/23: Fixed double-free problem

Comments (17)

  1. I may be missing something here…

    Wont the finally be called even when there was an exception and so the

    impersonationContext.Undo(); will be called twice in case of an exception!

  2. Nicole Calinoiu says:

    Isn’t it a wee bit late in the game to be considering this sort of thing to be a _subtle_ security hole? Allowing any caller code to run in the impersonation context strikes me as a severe security hole, with potential subtlety being a matter of discoverability only. For example, if DoSomeWorkWhileImpersonating() were to use a previously assigned delegate, that might be considered subtle, but only in the sense of "not immediately obvious to a reviewer of the code". Failure to implement a reversion pattern that accounts for potential exceptions is not particularly difficult to discover by either well-intentioned or malign examiners of one’s code, and it’s often relatively easy to exploit as well…

  3. Shawn says:

    Nicole,

    This is a very severe hole. You’re right, the subtlety I was referring to was the ability for a code reviewer to find the hole, or the initial programmer to know it was there in the first place. Obviously its a problem if you’re using managed code or not, but I think it becomes a bit more of an issue in managed langauges since we get so used to security state being tied to the stack frame, and thus being safe from situations like this.

    The double-subtlety here is that you can get bitten by first-chance exception handling, which is something a lot of people who are C# only developers would tend to not think about either.

    -Shawn

  4. Shawn says:

    Good catch Maheshwar — that could would indeed double undo, and even more importantly double CloseHandle. I’ve updated it to solve the problem.

    -Shawn

  5. WWWTC #9 ranks 10 out of 10 on the "difficult and subtle" scale. Let’s say we write the following…

  6. WWWTC #9 ranks 10 out of 10 on the "difficult and subtle" scale. Let’s say we write the following code

  7. Hafthor says:

    Regarding IDisposable wrapping WindowsImpersonationContext — Dispose method on WindowsImpersonationContext already calls Undo().

    // Declaring Type: System.Security.Principal.WindowsImpersonationContext

    // Assembly: mscorlib, Version=2.0.0.0

    [ComVisible(false)]

    public void Dispose()

    {

     this.Dispose(true);

    }

    [ComVisible(false)]

    protected virtual void Dispose(bool disposing)

    {

     if ((disposing && (this.m_safeTokenHandle != null)) && !this.m_safeTokenHandle.IsClosed)

     {

       this.Undo();

       this.m_safeTokenHandle.Dispose();

     }

    }

    so couldn’t you do

    try

    {

     // Begin impersonating the user, relying on Dispose to call Undo

     using(WindowsImpersonationContext impersonationContext = WindowsIdentity.Impersonate(userHandle.Token))

     {

       DoSomeWorkWhileImpersonating();

     }

    }

    catch // end search for qualifying exception handler to prevent exception filter exploit

    {

     throw;

    }

  8. shawnfa says:

    You’re right, it does, but the second call isn’t hurting anything either and it makes the intention of the code more clear to the reader.

    -Shawn