distinguish between recoverable PE dumping errors and non-recoverable ones

[GoogleCodeExporter]

This is copied from an email and should describe the problem:

* regarding the dumping plugins (moddump, dlldump, procexedump), the
--unsafe option allows users to ignore failed sanity checks on
sections. a failed section sanity check raises ValueError. but we also
have some *non-recoverable* conditions that also raise ValueError
(like this one:
http://code.google.com/p/volatility/source/browse/trunk/volatility/plugins/overl
ays/windows/windows.py#745).
the problem is we don't distinguish between the recoverable ValueError
and non-recoverable ValueError, so when either of them occurs, the
try/except block in plugins (for example:
http://code.google.com/p/volatility/source/browse/trunk/volatility/plugins/dlldu
mp.py#105)
will print a message like:

Unable to dump executable; sanity check failed:
You can use -u to disable this check

This is mis-leading because if its a non-recoverable error, then -u
won't do anything. So I'd suggest we either check if --unsafe is
already set before saying "You can use -u to disable this check" OR
have exceptions like RecoverablePEValueError and
NonRecoverablePEValueError so they can be dealt with differently when
raised.

Original issue reported on code.google.com by michael.hale@gmail.com on 11 Apr 2012 at 3:34

[GoogleCodeExporter]

Ok, here's a patch that adds a new exception type called SanityCheckException, 
and then catches just that.  Whoever applies it might want to catch the value 
errors as well and leave a message that they're unrecoverable.  Again, I don't 
know that I have an image that exhibits this, so I'm not the best person to 
test it out...

Original comment by mike.auty@gmail.com on 11 Apr 2012 at 8:41

Attachments:

• volatility-issue-243.patch

[GoogleCodeExporter]

Hey Mike, this seems good. When I looked for other places we'd want to catch 
SanityCheckException and ValueError, I came up with procdump.py, dlldump.py, 
and moddump.py, but there may be others like verinfo and the imports/exports 
generators. So I'm gonna wait until there's time to find all locations and 
apply the patch at once. We also might want to consider moving the big block of 
code in render_text of dlldump, procexedump, and moddump that does the dumping 
(since most of it can be shared). Not big priorities though. 

Original comment by michael.hale@gmail.com on 12 Apr 2012 at 1:59

[GoogleCodeExporter]

Cool, not big priorities, but if they seem pretty simple to get fixed up, we 
might as well try to do that before 2.1 goes out.  Have you managed to find any 
time for this one yet?  If not, can you suggest which kinds of things you think 
should be throwing/catching this and then I can take another crack at a patch?

Original comment by mike.auty@gmail.com on 25 Jun 2012 at 6:08

• Added labels: Milestone-2.1.x

[GoogleCodeExporter]

Alright, here's a patch that distinguishes between recoverable and 
non-recoverable PE errors (based on the one provided by Ikelos above). So in 
short, if an exception is not recoverable (such as PE header is paged, DOS 
signature doesn't check out, MZ signature not in the right place, etc) then 
users will not be advised to try again with the -u/--unsafe option. However, if 
PE sections sizes or virtual addresses are problematic, it'll raise 
SanityCheckException and users will be advised to try -u/--unsafe since those 
are the checks that can sometimes safely be ignored and still acquire the 
dll/pe/driver. 

I think we can probably work on PE parsing much more, and I know scudette has 
done a good amount of refactoring (especially with the PE address space), but 
for the time being we just want to prevent misleading messages (telling the 
user to try -u/--unsafe when in fact that would have no effect on the 
outcome)...so that's what this patch tries to accomplish. 

Original comment by michael.hale@gmail.com on 6 Jul 2012 at 5:18

Attachments:

• pe_sanity_exceptions.patch

[GoogleCodeExporter]

I was actually wondering why we have this flag at all? I mean the user asks to 
dump a pe file knowing its not going to be a perfect runnable file of course 
(it never is because we dont patch the IAT). So when would the user not want us 
to make the best efforts in dumping out the file? I mean is there a reasonable 
scenario in which a user is asking us to dump a file, and we tell them: "this 
dump does not pass sanity checks, use the --unsafe flag" and then the user 
decides - oh ok i wont dump it then?

I just dont see the point of the unsafe flag myself. I totally agree with 
logging errors about some anomalies but dont see why we should just abort 
dumping the file because its not perfect, and then ask the user if they really 
want us to try harder.

Original comment by scude...@gmail.com on 6 Jul 2012 at 8:55

[GoogleCodeExporter]

This issue was closed by revision r1984.

Original comment by mike.auty@gmail.com on 7 Jul 2012 at 7:20

• Changed state: Fixed

[GoogleCodeExporter]

As per Ikelos' request I have reposted this here:

Im a little confused about this new SanityCheckException exception. Its 
inconsistent with the NoneObject design pattern. In most other cases we return 
a NoneObject in case of errors, but here we raise an exception.

The original purpose of the NoneObject design was to avoid having to catch 
exceptions all the time just like the code above:

try:
    dos_header = obj.Object("_IMAGE_DOS_HEADER", offset = self.DllBase,
                            vm = self.obj_native_vm)
    return dos_header.get_nt_header()
...
except exceptions.SanityCheckException:
   return obj.NoneObject("Failed initial sanity checks. Try -u or --unsafe")

Are we changing the NoneObject design in favour of exceptions now?

Original comment by scude...@gmail.com on 8 Jul 2012 at 9:40