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

Tuesday, December 5, 2006

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?


Comments
Skup Tuesday, December 5, 2006
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...
Laurent Tuesday, December 5, 2006
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.
Philip the Duck Tuesday, December 5, 2006
Could the Path parameter refer to something that isn't a physical (or local?) file, but to which the administrator has write-access?
scott Tuesday, December 5, 2006
@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?
Sahil Malik Tuesday, December 5, 2006
WindowsImpersonationContext ain't threadsafe. Ju got a re-entrant code issue going on.
Laurent Tuesday, December 5, 2006
I have found something there in the line of Skup idea :

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 :-(
Eber Irigoyen Tuesday, December 5, 2006
- anyone can call Utility.ImpersonateAdministrator (not necesarily true)
- using this function you could override other sensitive files
scott Tuesday, December 5, 2006
@Laurent: Yes, you have it now. I'll post some additional info and resources later. It is very subtle.
Laurent Tuesday, December 5, 2006
Sorry about the horrible code formatting...
John Anderson ( sontek ) Thursday, December 7, 2006
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)
James Curran Thursday, December 7, 2006
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."

scott Friday, December 8, 2006
@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.
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!