Spot the Bug!

Privilege left high if ReadSecretFile() throws an exception?

Should be :

finally
{
LowerPrivelege();
}

If exception is raised in ReadSecretFile() method then LowerPrivilege() wont be called and they will be left in Elevated Priviledge state.

Even putting LowerPrivilege in a finally block after the catch is not going to be enough in the event of an exception filter higher up the call stack...

See, for example: http://pluralsight.com/blogs/keith/archive/2005/03/31/7149.aspx

Lower privilege outside of the try-catch. All you are doing is reading a file, forcing yourself into a lower privilege won't effect much because you would naturally lower elevation if the try succeeded.

So:

try {
ElevatePrivilege();
try {
ReadSecretFile();
}
finally {
LowerPrivilege();
}
}
catch(FileException fe) {
ReportException();
}

ok i m not an expert but try this and post the reson.

consider a timer whose interval is set to 60000 ms.(1 minute)
see this 2 example and try to figure out which one is better :)
1)
Private Sub tmrMain_Elapsed(ByVal sender As System.Object, ByVal e As System.Timers.ElapsedEventArgs) Handles tmrMain.Elapsed
Dim dt As DateTime
dt = dt.Parse("02:10:7 pm")
If dt.Hour = Now.Hour Then
If dt.Minute = Now.Minute Then
msgbox "hello"
....
....
else
....
....
end if
else
....
....
end if
end sub
2)
Private Sub tmrMain_Elapsed(ByVal sender As System.Object, ByVal e As System.Timers.ElapsedEventArgs) Handles tmrMain.Elapsed
Dim dt As DateTime
dt = dt.Parse("02:10:7 pm")
If dt.Minute = Now.Minute Then
If dt.Hour = Now.Hour Then
msgbox "hello"
....
else
....
....
end if
else
....
....
end if
end sub

if there are hundreads of line of code between if..else which on u preffer and why?
regards,

Besides the obvious, I would also consider it a bug to report an exception (via ReportException) but not actually pass the exception...

I only looked at it for a few seconds on my way though the MSDN site, but at first glance you are left with an elevation of privilege vulnerability if the call to ReadSecretFile fails and an exception is raised.

The caller will be left in a higher privileged state rather than the system failing to a secure stat.

The permissions will still be elevated int he catch block if an excpetion is thrown.

You need a finally block to lower the permissions if the call to ReadSecretFile should throw an exception
try
{
ElevatePrivilege();
ReadSecretFile();
}
catch(FileException fe)
{
ReportException();
}
finally
{
LowerPrivilege();

}

You do not need to run at elevated privilege to read the file. This is distinguishing between Access Check and Privilege level as outlined in the Code Secure book

ElevatePrivilege(), ReadSecretFile(), LowerPrivilege() & ReportException can throw exceptions that can't be handled by caught by FileException catch block.

Then the result would be...

So it'd be better if there was "catch (Exception ex) as well

This is minor but I believe:

catch(FileException fe)

will cause a compiler warning.

It should be:

catch(FileException)

since "fe" is not being used.

Can we talk about how to quantify how this particular bug's exploitability? Basically, anywhere else in our code where we do a permissions check before doing something is suspect. We risk that the current privileges state is incorrectly set. Doesn't this really reflect a larger design flaw than just this particular try/catch/finally statement?