Insecure Exceptions

SecureASP.NET

LANGUAGES: ALL

ASP.NET VERSIONS: ALL

Insecure Exceptions

By Don Kiely

Shawn Farkas of Microsoft (http://blogs.msdn.com/shawnfa) andKeith Brown of Pluralsight (http://pluralsight.com/blogs/keith)recently pointed out some nasty problems that exceptions can cause if you renot careful. See if you can spot the vulnerability in this code (assume thatuserHandle is a valid handle created before this code runs):

public void DoStuff()

{

     // Beginimpersonating the user

     WindowsImpersonationContext impersonationContext

      =WindowsIdentity.Impersonate(userHandle.Token);

     try

     {

           DoSomeWorkWhileImpersonating();

     }

     finally

     {

           if(userHandle!= IntPtr.Zero)

                 CloseHandle(userHandle);

           if(impersonationContext != null)

                 impersonationContext.Undo();

     }

}

Seems pretty innocuous, right? You impersonate a user todo something for which you need another user s credentials, say write to aprotected directory. Being a careful programmer, you wrap the work in a tryblock and put clean-up code in the finally block that closes the user handleand undoes the impersonation, guaranteeing that the clean-up code executes. Allis right with the world, eh?

See it?

Here s the problem. Say that untrusted, malicious code issomewhere above this code on the stack. In other words, nasty code either callsDoStuff directly or calls code that calls it. The nasty code includes a catchblock with an exception filter, something like this:

Sub NastyCode()

     Try

           DoStuff()

     Catch ex As ExceptionWhen DoNastyWhileImpersonated()

           Throw

     End Try

End Sub

[Note: I wrote the above in VB.NET because VB.NET hassupported exception filters the When clause in the Catch statement sinceversion 1.0. Exception filters are actually implemented in .NET s intermediatelanguage (IL) and exposed by VB.NET, but C# through 1.1 doesn t expose them. C#2.0 will support them, however.]

The real problem is that impersonation is associated witha thread and not the stack frame, so the impersonation applies to any code thatexecutes in the thread, even if you jump around the stack as you do withexceptions. So when the DoNastyWhileImpersonated code executes under thesecurity context of the impersonated user, presumably an account with powerfulabilities, it is able to do whatever that account can do.

Subtle, eh? This is a greatlesson in how you can t only look at the code in front of you, DoStuff, anddecide whether it is secure or not, particularly when you have what amounts toa GoTo lurking in the try..finally block. (Actually, aGoTo is easier to handle because it always branches to a location known atdesign time, unlike exceptions, which could jump pretty much anywhere up thestack.)

So how to fix it? One solution,suggested by Shawn Farkas, is to put the clean-up code both in a catch blockand the finally block, like this:

static void Better()

{

     // Beginimpersonating the user

     WindowsImpersonationContext impersonationContext

      =WindowsIdentity.Impersonate(userHandle.Token);

     try

     {

           DoSomeWorkWhileImpersonating();

     }

     catch

     {

           if(userHandle!= IntPtr.Zero)

           {

                 CloseHandle(userHandle);

                 userHandle = IntPtr.Zero;

           }

           if(impersonationContext != null)

           {

                 impersonationContext.Undo();

                 impersonationContext = null;

           }

           throw;

     }

     finally

     {

           if(userHandle!= IntPtr.Zero)

                 CloseHandle(userHandle);

           if(impersonationContext!= null)

                 impersonationContext.Undo();

     } 

}

I don t know about you, but putting duplicate code likethis in a procedure just makes my skin crawl. With an exception filter ineither VB.NET or C# 2.0, you could clean this up by putting the clean-up codein a separate method, and call the method from both the exception filter on thecatch block and from the finally block. But either technique is necessary tomake your code secure. The reason this works is that now there is a catch blockin the local method, so the CLR doesn t have to crawl the stack to find one,which just might be located in malicious code. The undo is handled right hereso that the user is no longer impersonated when the exception is thrown back upthe stack. So malicious code doesn t have the benefit of theimpersonation.

Structured exception handling is a great feature of .NET,but it can open some subtle, yet lethal, security holes in your code.

DonKiely, MVP, MCSD, is a senior technology consultant, building customapplications as well as providing business and technology consulting services.His development work involves tools such as SQL Server, Visual Basic, C#,ASP.NET, and Microsoft Office. He writes regularly for several trade journals,and trains developers in database and .NET technologies. You can reach Don at mailto:[email protected] and readhis blog at http://www.sqljunkies.com/weblog/donkiely/.