What's Wrong With This Code? (#9)

Jill Developer has a new assignment. She needs code that will overwrite data in a sensitive file. Only the local machine administrator has access to the file, but Jill plans to impersonate the admin account to gain access to the file.

Jill first builds a static class to PInvoke LogonUser and start the impersonation. This class (Utility), and it's method (ImpersonateAdministrator) work well. Jill's next step is to write the following code:

public void WriteToSensitiveFile(string path, byte[] data)
{
    
WindowsImpersonationContext impersonationContext = null;
    impersonationContext =
Utility.ImpersonateAdministrator();

    
try
    {
        
using (FileStream fs = File.OpenWrite(path))
        {
            fs.Write(data, 0, data.Length);
        }

    }
    
finally
    {
        
if (impersonationContext != null)
        {
            impersonationContext.Undo();
        }
    }

}

Of course, Jill still has some work ahead to verify the path, the data, and the user who is calling this method. At this early point, however, Jill has one worry she wants to put to rest before moving on - is it possible for a malicious caller to take advantage of the impersonation context and do something other than write to a file?

posted on Monday, December 04, 2006 11:34 PM by scott

Comments

Tuesday, December 05, 2006 2:23 AM by Skup

# re: What's Wrong With This Code? (#9)

It is possible that a ThreadAbortException occures between the return of Utility.ImpersonateAdminitrator and the start of the try section.

A good scheduling of thread.Abort() done by a malicious user from another thread (ok it's not that easy to do that) enables the user to avoid the finally statement and keep the impersonnation context in the remaining of the application. Too bad...
Tuesday, December 05, 2006 2:26 AM by Laurent

# re: What's Wrong With This Code? (#9)

The "public" accessor is a problem. If somebody built an external exe which uses reflection, he might succeed to invoke that method.
I think if "public" cannot be replaced, a specific code attribute should be set up above the method to restrict callers assemblies.
Tuesday, December 05, 2006 3:14 AM by Philip the Duck

# re: What's Wrong With This Code? (#9)

Could the Path parameter refer to something that isn't a physical (or local?) file, but to which the administrator has write-access?
Tuesday, December 05, 2006 7:26 AM by scott

# re: What's Wrong With This Code? (#9)

@Skup: You are thinking along the right lines. I'm thinking of something to do with exception filters that would require an even less severe exception..

@Laurent: Let's assume anyone can invoke the method, but we can verify the caller and the paramerters. Is there anyway for the impersonation to "escape" outside of the method.

@Philip: That's quite possible given all the naming constructs you can use in a path. Let's assume we can validate that the incoming path points to a local file. Is there any danger in the impersonation?
Tuesday, December 05, 2006 7:42 AM by Sahil Malik

# re: What's Wrong With This Code? (#9)

WindowsImpersonationContext ain't threadsafe. Ju got a re-entrant code issue going on.
Tuesday, December 05, 2006 8:15 AM by Laurent

# re: What's Wrong With This Code? (#9)

I have found something there in the line of Skup idea :

http://msdn2.microsoft.com/en-us/library/aa302372.aspx

Putting two level of try / finally should ensure the finally to be executed before any exception to be thrown:

try
{
try
{
using (FileStream fs = File.OpenWrite(path))
{
fs.Write(data, 0, data.Length);
}

}
finally
{
if (impersonationContext != null)
{
impersonationContext.Undo();
}
}
}
catch
{
throw;
}

Eeewww... How tricky :-(
Tuesday, December 05, 2006 10:05 AM by Eber Irigoyen

# re: What's Wrong With This Code? (#9)

- anyone can call Utility.ImpersonateAdministrator (not necesarily true)
- using this function you could override other sensitive files
Tuesday, December 05, 2006 10:11 AM by scott

# re: What's Wrong With This Code? (#9)

@Laurent: Yes, you have it now. I'll post some additional info and resources later. It is very subtle.
Tuesday, December 05, 2006 11:50 AM by Laurent

# re: What's Wrong With This Code? (#9)

Sorry about the horrible code formatting...
Wednesday, December 06, 2006 7:07 PM by K. Scott Allen

# So, What Was Wrong with That Code Anyway?


WWWTC #9 ranks 10 out of 10 on the "difficult and subtle" scale. Let's say we write the following...
Wednesday, December 06, 2006 8:09 PM by Mirror blog entries from the industry

# So, What Was Wrong with That Code Anyway?

WWWTC #9 ranks 10 out of 10 on the "difficult and subtle" scale. Let's say we write the following code
Wednesday, December 06, 2006 9:21 PM by Christopher Steen

# Link Listing - December 6, 2006

ASP.NET AJAX Under the Hood Secrets by PageFlakes.com Creator [Via: ScottGu ] Announcing the release...
Wednesday, December 06, 2006 10:54 PM by John Anderson ( sontek )

# re: What's Wrong With This Code? (#9)

I think in .NET 2.0 the CLR actually checks if an impersonation takes place so it isn't an issue for that (but not positive on it, just read some blogs that say that)
Thursday, December 07, 2006 8:17 AM by James Curran

# re: What's Wrong With This Code? (#9)

Well, you wrote
WindowsImpersonationContext impersonationContext = null;
impersonationContext = Utility.ImpersonateAdministrator();

instead of the more "proper"
WindowsImpersonationContext impersonationContext = Utility.ImpersonateAdministrator();

which suggests that there is a way for the impersonation to be in effect with impersonationContext still null. AN exception thrown in ImpersonateAdministrator would do that (but that conflict with the statement that "class (Utility), and it's method (ImpersonateAdministrator) work well."

Thursday, December 07, 2006 9:38 PM by scott

# re: What's Wrong With This Code? (#9)

@Sahil: No, this is MT safe since the context is a local variable.

@John: Correct, except it places a marker in the stack frame where impersonation starts (in 2.0), and in this case the impersonation starts below the current method doing work.

@James: This was just to make the code pretty for the screen. We'll assume the ImpersonateAdminisrtator method is bulletproof and won't let impersonation start if there is an exception.
Wednesday, December 13, 2006 11:58 AM by Community Blogs

# So, What Was Wrong with That Code Anyway?

WWWTC #9 ranks 10 out of 10 on the "difficult and subtle" scale. Let's say we write the following code