"Finally" Does Not Mean "Immediately"

"Finally" Does Not Mean "Immediately"

Rate This
  • Comments 34

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.

  • 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.
  • 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.
  • This seems to be the fault exclusively of the "when" feature.
  • 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.
  • > 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.
  • 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)
  • 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.
  • >>> 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.
  • 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.
  • 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...
Page 2 of 3 (34 items) 123