"Finally" Does Not Mean "Immediately"

Rate This

Eric Lippert

1 Sep 2004 9:51 AM

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.

Security

Peter Torr

1 Sep 2004 10:25 AM

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 ;-)
Eric Lippert

1 Sep 2004 11:06 AM

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.
John Cavnar-Johnson

1 Sep 2004 11:22 AM

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).
John Cavnar-Johnson

1 Sep 2004 11:23 AM

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

1 Sep 2004 11:39 AM

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.
Paul Vick

1 Sep 2004 11:50 AM

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...).
John Cavnar-Johnson

1 Sep 2004 12:29 PM

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.
Nicholas Allen

1 Sep 2004 12:34 PM

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.
Eric Lippert

1 Sep 2004 12:41 PM

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.
Eric Lippert

1 Sep 2004 1:49 PM

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...
Nicholas Allen

1 Sep 2004 2:17 PM

C# should let you do

throw;

just the same.
Nicholas Allen

1 Sep 2004 2:28 PM

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.
Kannan Goundan

1 Sep 2004 2:50 PM

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

Was it just for performance?
Peter Torr

1 Sep 2004 5:57 PM

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.
Eric Lippert

1 Sep 2004 10:28 PM

I learn something new every day in this job.

"Finally" Does Not Mean "Immediately"

Eric Lippert’s Blog

A new fabulous adventure

Why is deriving a public class from an internal class illegal?

It's still essential!

Dynamic contagion, part two

"Finally" Does Not Mean "Immediately"