“Finally” Does Not Mean “Immediately”


All that talk a while back about error handling in VBScript got me thinking about some error handling security issues that involve VB.NET specifically, though they apply to managed code written in any language.

Consider this scenario: you provide a fully trusted library of useful functions which can be used by partially trusted callers.  The partially trusted callers might be hostile — that’s why they’re partially trusted, after all.  You have a particular method which needs administration rights.  When it’s called, it prompts the user for the administrator password, impersonates the administrator, does work, and then reverts the impersonation.

Public Sub DoTheThing()
  ‘ Omitted — prompt user to obtain password
  NewIdentity = GetWindowsIdentity(AdminName, Domain, Password)
  NewContext = NewIdentity.Impersonate()
  DoTheAdminThing()
  NewContext.Undo()
End Sub

There’s a potentially huge security hole here.  What if DoTheAdminThing throws an exception?  There’s no Catch or Finally block around the Undo, so control can return to the partially trusted caller with the impersonation still intact!  That seems bad!  Let’s stick a Finally on there:

  NewIdentity = GetWindowsIdentity(UserName, Domain, Password)
  NewContext = NewIdentity.Impersonate()
  Try
    DoTheAdminThing() 
  Finally
    NewContext.Undo()
  End Try

Clearly this an improvement from a code-quality perspective, but does it solve the security problem?

No!  A Finally block guarantees that the code will run eventually, but it does not guarantee that no code will run between the point of the exception and the Finally block!  VB .NET provides a syntax specifically for running code before the Finally block: the exception filter.  Suppose our hostile code looked like this:

Public Sub IAmSoEvil()
  Try
    NiceObject.DoTheThing()
  Catch Ex As System.Exception When ExploitFlaw()
    DoSomethingElse()
  End Try
End Sub
Public Function ExploitFlaw() As Boolean
  DoSomethingEvil() 
  ExploitFlaw = True
End Function

Let’s trace through what happens.

  • First, the hostile partially trusted 

IAmSoEvil calls the fully trusted DoTheThing method.

  • DoTheThing
  • impersonates the administrator and calls DoTheAdminThing.

  • DoTheAdminThing
  • throws an exception (possibly because the partially trusted code has previously eaten up lots of memory or otherwise caused error conditions to become more likely.)

  • The .NET Runtime finds the Catch block in
  • IAmSoEvil and runs ExploitFlaw to see if this block can handle the exception.

  • ExploitFlaw
  • calls DoSomethingEvil, which attempts to take advantage of the fact that it is now impersonating an administrator.

  • DoSomethingEvil
  • finishes its attempt at an exploit and returns.

  • ExploitFlaw
  • returns True. The .NET Runtime stops its search for exception handlers.

  • The .NET Runtime runs the code in the
  • Finally block of DoTheThing. The admin impersonation is reverted.

  • The .NET Runtime runs
  • DoSomethingElse in the Catch block of IAmSoEvil.

    Of course, this doesn’t apply to just thread impersonation.  Any time that important global state can be made inconsistent requires very careful exception handling to ensure that no hostile code runs while the state is vulnerable.  We can do that by catching the error before the hostile code can:

    Dim Reverted As Boolean
    Reverted = False
    NewContext = NewIdentity.Impersonate()
    Try 
      DoTheAdminThing()
    Catch Ex As Exception
      NewContext.Undo()
      Reverted = True
      Throw Ex
    Finally 
      If Not Reverted Then 
        NewContext.Undo()
      End If
    End Try

    Gross, but there’s not much else you can do about it.  Fortunately, these kinds of situations are pretty rare.

    Pop quiz: Is this another example of the flaw pattern?

    FileIOPermission.Assert()
    Try 
      DoSomething()
    Finally 
      CodeAccessPermission.RevertAssert()
    End Try

    Does this do the same thing as before — potentially pass control to a partially-trusted exception filter before the assertion is reverted?  If so, how do you defend against it?  If not, why not, and are there any other potential attacks here? 

    Tune in next time to find out! I’ll be AFK for the Labour Day weekend, so we’ll have the thrilling conclusion next week.  Have a nice weekend, everyone.

    Comments (34)

    1. Peter Torr says:

      I won’t answer the question (it would spoil it for the others 😉 ) but you should really put your Impersonate call *inside* the Try block… theoretically, an exception could occur between the callvirt instruction and the stloc instruction, leaving you impersonating the Administrator.

      The exception could either be from the CLR itself (eg, OutOfMemory) or if you were foolish enough to give the partially-trusted code ControlThread access, maybe it tried to Abort() you. (Similar things for ControlAppDomain and unloading you).

      And so on 😉

    2. Eric Lippert says:

      For safety’s sake, yes, it’s a good habit to put more stuff inside the try. But can an OOM really happen on a store? And if the thread/ADM goes down then the hostile caller just threw away the impersonated thread. Seems like an unlikely attack.

    3. John Cavnar-Johnson says:

      I think your code would be less gross if you changed:

      Dim Reverted As Boolean

      Reverted = False

      NewContext = NewIdentity.Impersonate()

      Try

      DoTheAdminThing()

      Catch Ex As Exception

      NewContext.Undo()

      Reverted = True

      Throw Ex

      Finally

      If Not Reverted Then

      NewContext.Undo()

      End If

      End Try

      To this:

      Dim NewContext As WindowsImpersonationContext

      Try

      NewContext = NewIdentity.Impersonate()

      DoTheAdminThing()

      Catch Ex As Exception When WeHaveCleanedUp(NewContext)

      Throw Ex

      End Try

      NewContext.Undo()

      ———–

      Private Function WeHaveCleanedUp(ByVal myContext As WindowsImpersonationContext) As Boolean

      myContext.Undo()

      Return True

      End Function

      —————-

      Exception filters are really useful. Too bad C# doesn’t have them (yet).

    4. John Cavnar-Johnson says:

      Sorry about the formatting, but you get the idea, I hope.

    5. Eric Lippert says:

      That’s extremely clever!

      That’s why I don’t like it.

      Clever code is badness. As Brian Kernighan famously pointed out, it takes twice as much cleverness to understand code as to write it; clever code is an impediment to maintenance.

      It’s clever because it uses a tool to elegantly achieve a goal, but uses the tool in a manner different from its intended use. The by-design purpose of an exception filter is to MAKE A DECISION as to whether the exception can be handled or not. That it implements it by running the filter code during the search of the catch block chain is an implementation detail of the design that you’re taking advantage of for another purpose.

      Similarly, I also object to using exceptions as a control flow mechanism in non-error cases. Exceptions are for exceptional circumstances, not normal control flow. There’s nothing technically stopping you from writing programs that work by doing expected non-local-gotos all over the place, but doing so is using a tool for the opposite of its intended purpose.

    6. Paul Vick says:

      Ahh… The joys of two-pass exception handling. One comment, though: in the Catch statement, instead of saying "Throw ex", you can just say "Throw". This does a rethrow of the original exception and doesn’t muck about with the exception origin in the exception. This is nice because it doesn’t make it look like your function threw the exception (unless you want it to look that way…).

    7. John Cavnar-Johnson says:

      I agree with your sentiment about clever code, but I disagree with your assessment of this particular technique. I think it’s easier to see with a slightly more complex example. In real code, the function would make a decision about whether or not to handle the exception. It would also make sure that it protected the internal state of the calling routine in all cases. More importantly, I can’t agree with your assessment of the design of the exception filter. It seems to me that "running the filter code during the search of the catch block" is an inherent part of the design contract, not an implementation detail. I would have expected the design of two-pass exception handling to be very different if your assertion were correct.

    8. Nicholas Allen says:

      I’m positive you’ve written almost this exact article before. I can’t find it online so it may have been in the code security book though.

      But to tag onto what Peter was saying, in theory it can get an OOM there although maybe some implementations go beyond the spec to make it safe. If they do an abort/unload on you, they can still save the context by cancelling the abort after it jumps out of your code.

    9. Eric Lippert says:

      I knew someone would spot that. This article is a reworking of part of chapter 5 of my book.

      I got an email the other day from someone who was wondering about the security semantics of exception handlers, so I figured that now would be a good time to get some of this stuff up on the web.

    10. Eric Lippert says:

      Oh, and thanks for the note Paul, I did not realize that VB had that capability.

      I wonder if there’s a way to do that in C#? That would actually potentially fix a problem we’ve got in the VSTO2 exception handling semantics…

    11. Nicholas Allen says:

      C# should let you do

      throw;

      just the same.

    12. Nicholas Allen says:

      In the IL it’s just a special opcode (0xFE1A) rethrow that you can use in a handler block (but not in a filter block to tie back to the original topic). So I’d expect pretty much every NET language to support it if they have exception handling.

    13. Kannan Goundan says:

      Why were exception filters designed to behave like that? It’s clearly counterintuitive.

      Was it just for performance?

    14. Peter Torr says:

      Yes, Nicholas is correct. The attack goes like this (pseudo code):

      // —–

      Thread t

      Hack()

      {

      t = new Thread(new ThreadStart(EvilCode))

      t.Start()

      Thread.Sleep(MAGIC_NUMBER)

      t.Abort()

      }

      void EvilCode

      {

      try

      {

      CallTheImpersonationRoutine()

      }

      catch

      {

      // Now I have admin!

      }

      }

      // —–

      Where MAGIC_NUMBER is clearly the number of milliseconds you need to pause to cause the ThreadAbort at exactly the right time. Or you just keep spinning up new threads and sleeping for increasing numbers of milliseconds until you finally get admin.

    15. Eric Lippert says:

      I learn something new every day in this job.

    16. Nicole Calinoiu says:

      WRT to the CAS question, the same problem does not occur. This is because the assertion on the asserting method’s frame is too low on the call stack to have an effect on a demand for the asserted permission that might be initiated from code run separately from the partially trusted caller.

      As for other potential attacks, of course there are more. Aren’t there always? <g> Without even digging into anything too obscure, there’s the issue of DoSomething() itself which might, for example, invoke a delegate. In addition, the code as written does not make any verification of the caller’s right to perform the target action. While this might be acceptable in some rare cases, it’s opens the code up to a potential luring attack that could be avoided by a demand x then assert y sequence.

    17. Nicholas Allen says:

      I was mulling over this last night, but it seems like your solution is still vulnerable. The pattern you use goes like:

      Try

      DoTheAdminThing()

      Catch Ex As Exception



      Throw Ex

      Finally



      End Try

      But what if we can trick DoTheAdminThing into throwing something that’s not a [mscorlib]System.Exception? Your catch block won’t be picked to handle it so your finally block won’t run until after the exception filter. Now, VB only lets you throw and run filters on Exception, but I wrote my own in IL that doesn’t have this restriction.

    18. Macneil says:

      This seems to be the fault exclusively of the "when" feature.

    19. Eric Lippert says:

      Indeed, the Virtual Execution System specification indicates that any class can be thrown — typically you’d subclass System.Exception, but you don’t have to.

      However, a fully-trusted library that throws exceptions that can’t be caught is asking for trouble. That seems like a pretty rare case.

    20. Eric Lippert says:

      > Why were exception filters designed to behave like that? It’s clearly counterintuitive.

      Why is it counterintuitive? Consider the simplest case:

      try {

      foo()

      } catch when blah() {

      bar()

      } finally {

      baz()

      }

      if foo throws, do you expect the finally to run before the filter or after? Clearly this goes blah, bar, baz. Why should it go in a different order just because the finally is lower on the stack?

      I suppose the logic could have been "check for filter handlers in the current scope, if there are some, run them. Then check for finally blocks, run them. If there was no successful catch, throw the exception up to the next frame" Seems like there would be more state to keep around, but I suppose it could have been implemented that way.

      I don’t know what pros and cons of each were considered by the CLI designers, and the CLI Annotated Specification, page 156, isn’t much help.

    21. Shawn says:

      I just wrote a four part series about Assert … looks like the answer to your pop quiz is Assert Myth #4 (http://blogs.msdn.com/shawnfa/archive/2004/08/25/220458.aspx#myth4)

    22. Nicholas Allen says:

      That’s not quite right. I’m having trouble explaining why in a way that doesn’t spoil the answer for people here. Your Myth #1 is closer to it.

    23. Owen Phelps says:

      >>> if foo throws, do you expect the finally to run before the filter or after? Clearly this goes blah, bar, baz. Why should it go in a different order just because the finally is lower on the stack? <<<

      But but but… that’s not what makes it counterintuitive.

      I expect it to unwind up the stack, going through each catch/finally it comes to in turn. In any *specific* try/catch/finally block, sure, the catch can go first as you describe, but I do *not* expect a catch from further away to fire before a much more local finally.

      If I understand you correctly, the stack is effectively unwound once for catch blocks then again for finally blocks. Is that right? If so, then that is indeed counter-my-intuition.

      I’ve done a fair bit of programming in Delphi, and there the finally is guaranteed to run before you leave its locality. Perhaps I’m blinded by previous experience, but I can’t think why you’d want it to work any other way. Indeed, the potential security issue you’ve highlighted appears to be a great argument against any other approach.

    24. Eric Lippert says:

      The stack is _searched_ for filter blocks which run first, and then _unwound_, running the finally blocks.

      I’m not sure what led to this design decision in the CLI or in VB.NET. You might want to ask Paul Vick.

    25. Shyblog says:

      Eric Lippert: "Finally" Does Not Mean "Immediately" I agree w/ Owen that the difference between how filter blocks and finally blocks are processed seems counter intuitive at first glance. It would be good to know whether this is particular…

    26. Sean Watson says:

      I’ve been asked to explain this blog entry several times. I wanted to add my comments here (a) so I can stop repeating myself, (b) so people I don’t know will be benefit from any clarification I can add, and (c) because I might be wrong and I’d like someone to point it out if I am.

      Q: Why does it do that? Isn’t it wrong to run code out of the try/finally block before the finally is executed?

      A: See the CLI Partition I, Section 12.4.2.5. Clearly someone made a conscious decision that is should work this way. I’m not privy to the real though process behind this, but I can take a stab.

      I think this is Windows structured exception handling (SEH) showing through. Windows has a nice, robust, fully featured exception handling system. When you cause an exception in Windows (either by calling RaiseException or by causing a hardware fault), the OS walks a chain of exception and termination handlers and evaluates a filter for each exception handler. The filter can say (a) keep looking, I’m not the handler you want, (b) stop looking and use this filter, or (c) stop looking and continue executing where you left off. The third one is useful if you want to fault in memory (or a dll), handle some sort of integer overflow, continue after hitting a breakpoint in a debugger, continue after hitting an *exception* in a debugger, etc.

      Because Windows exceptions have this nice feature of being something you can ignore or otherwise recover from, it’s important not to assume you’ll exit the innermost protected block just because you hit an exception. Following this through a little, that means it’s important not to run code in the termination handlers you found along the way unless you find a filter that tells you it’s the codeflow’s really going to exit its protected block. Only after Windows finds an exception filter that will handle the exception is it safe to call the termination handlers for any nested blocks.

      Q: But you’re talking about Windows EH; the .Net Framework is platform independent.

      A: Yes. You can implement this sort of exception handling on pretty much any platform capable of dealing with C’s setjump/longjump. I just so happens that using Windows EH makes the .Net Framework more efficient, able to more easily supported by debuggers, and more able to interoperate with native code written for Windows. The CLI folks (based my reading of I.12.4.2), did a fairly good job of documenting what behavior to expect.

      Q: If I have this code:

      IDisposable disposeMe = new SomeDisposableClass();

      try

      {

      /* do something that might throw */

      }

      catch

      {

      throw;

      }

      finally

      {

      disposeMe.Dispose();

      }

      Can I be sure any unsafe state created in SomeDisposableClass is disposed of before code exits this block?

      A: No. Again looking a I.12.4.2, there are 4 kinds of trys: one with a finally, one with a generic catch, one with a type-filtered catch, and one with a user-filtered catch. The code above really translates to:

      IDisposable disposeMe = new SomeDisposableClass();

      try

      {

      try

      {

      /* do something that might throw */

      }

      catch

      {

      throw;

      }

      }

      finally

      {

      disposeMe.Dispose();

      }

      Q: How did you find out about all this? When I read I.12.4.2, I don’t see half of what you’re talking about.

      A: I.12.4.2 is just a reality check. The only way to be absolutely sure your code is doing what you think it is to step through it in the debugger and run it through interesting cases. Since the language is doing things you might not expect, stepping though the disassembly is probably the way to go (until you again know what to expect from the higher level language).

    27. Eric Lippert says:

      Thanks Sean — spec diving is always interesting.

      For the expert take on SEH and CLR exception handling, see CBrumme’s massive blog entry on the subject:

      http://blogs.msdn.com/cbrumme/archive/2003/10/01/51524.aspx

    28. Richard says:

      Where can exception filters be useful? Surely to do anything you can’t do just by catching the exception and doing a "throw;" if you don’t like it they need internal knowledge of the code where the exception is thrown. That just seems wrong – what about encapsulation?

      Surely the internal details of a method’s exception state shouldn’t be part of its public interface.