|
|
|
|
 |
Secure ASP.NET LANGUAGES: ALL ASP.NET VERSIONS: ALL
Insecure Exceptions
Shawn Farkas of Microsoft (http://blogs.msdn.com/shawnfa) and Keith Brown of Pluralsight (http://pluralsight.com/blogs/keith) recently pointed out some nasty problems that exceptions can cause if you’re not careful. See if you can spot the vulnerability in this code (assume that userHandle is a valid handle created before this code runs):
public void DoStuff() { // Begin impersonating 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 to do something for which you need another user’s credentials, say write to a protected directory. Being a careful programmer, you wrap the work in a try block and put clean-up code in the finally block that closes the user handle and undoes the impersonation, guaranteeing that the clean-up code executes. All is right with the world, eh?
See it?
Here’s the problem. Say that untrusted, malicious code is somewhere above this code on the stack. In other words, nasty code either calls DoStuff directly or calls code that calls it. The nasty code includes a catch block with an exception filter, something like this:
Sub NastyCode() Try DoStuff() Catch ex As Exception When DoNastyWhileImpersonated() Throw End Try End Sub
[Note: I wrote the above in VB.NET because VB.NET has supported exception filters — the When clause in the Catch statement — since version 1.0. Exception filters are actually implemented in .NET’s intermediate language (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 with a thread and not the stack frame, so the impersonation applies to any code that executes in the thread, even if you jump around the stack as you do with exceptions. So when the DoNastyWhileImpersonated code executes under the security context of the impersonated user, presumably an account with powerful abilities, it is able to do whatever that account can do.
Subtle, eh? This is a great lesson in how you can’t only look at the code in front of you, DoStuff, and decide whether it is secure or not, particularly when you have what amounts to a GoTo lurking in the try..finally block. (Actually, a GoTo is easier to handle because it always branches to a location known at design time, unlike exceptions, which could jump pretty much anywhere up the stack.)
So how to fix it? One solution, suggested by Shawn Farkas, is to put the clean-up code both in a catch block and the finally block, like this:
static void Better() { // Begin impersonating 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 like this in a procedure just makes my skin crawl. With an exception filter in either VB.NET or C# 2.0, you could clean this up by putting the clean-up code in a separate method, and call the method from both the exception filter on the catch block and from the finally block. But either technique is necessary to make your code secure. The reason this works is that now there is a catch block in 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 here so that the user is no longer impersonated when the exception is thrown back up the stack. So malicious code doesn’t have the benefit of the impersonation.
Structured exception handling is a great feature of .NET, but it can open some subtle, yet lethal, security holes in your code.
Don Kiely, MVP, MCSD, is a senior technology consultant, building custom applications 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:donkiely@computer.org and read his blog at http://www.sqljunkies.com/weblog/donkiely/.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
|
||||||
