vadinfo's Protection field can be misleading

[GoogleCodeExporter]

I just updated the wiki to describe how vadinfo's Protection field can be 
misleading. In particular:

Note there is a difference between the original protection and current 
protection. The original protection is derived from the flProtect parameter to 
VirtualAlloc. For example you can reserve memory (MEM_RESERVE) with protection 
PAGE_NOACCESS (original protection). Later, you can call VirtualAlloc again to 
commit (MEM_COMMIT) and specify PAGE_READWRITE (becomes current protection). 
The vadinfo command shows the original protection only. Thus, just because you 
see PAGE_NOACCESS here, it doesn't mean code in the region cannot be read, 
written, or executed. 

So I'd like to create this issue to remind us that two enhancements could be 
made:

1) Show both the original *and* the current page protections 
2) Add code to translate the Protection constants (like 4, 18, 24) into 
something human readable like Read/Write/Execute. 

Original issue reported on code.google.com by michael.hale@gmail.com on 29 Sep 2011 at 3:53

[GoogleCodeExporter]

By the way, some more documentation on the difference between original and 
current protection can be found here:

http://undocumented.ntinternals.net/UserMode/Undocumented%20Functions/Memory%20M
anagement/Virtual%20Memory/MEMORY_BASIC_INFORMATION.html

Note the AllocationProtect vs Protect fields of that structure. 

Original comment by michael.hale@gmail.com on 29 Sep 2011 at 3:56

[GoogleCodeExporter]

Also, to address #2 above, the vad.Flags.Protection >> 24 value is an index 
into the array nt!MmProtectToValue:

PROTECT_FLAGS = [
    'PAGE_NOACCESS',
    'PAGE_READONLY',
    'PAGE_EXECUTE',
    'PAGE_EXECUTE_READ',
    'PAGE_READWRITE',
    'PAGE_WRITECOPY',
    'PAGE_EXECUTE_READWRITE',
    'PAGE_EXECUTE_WRITECOPY',
    'PAGE_NOACCESS',
    'PAGE_NOCACHE | PAGE_READONLY',
    'PAGE_NOCACHE | PAGE_EXECUTE',
    'PAGE_NOCACHE | PAGE_EXECUTE_READ',
    'PAGE_NOCACHE | PAGE_READWRITE',
    'PAGE_NOCACHE | PAGE_WRITECOPY',
    'PAGE_NOCACHE | PAGE_EXECUTE_READWRITE',
    'PAGE_NOCACHE | PAGE_EXECUTE_WRITECOPY',
    'PAGE_NOACCESS',
    'PAGE_GUARD | PAGE_READONLY',
    'PAGE_GUARD | PAGE_EXECUTE',
    'PAGE_GUARD | PAGE_EXECUTE_READ',
    'PAGE_GUARD | PAGE_READWRITE',
    'PAGE_GUARD | PAGE_WRITECOPY',
    'PAGE_GUARD | PAGE_EXECUTE_READWRITE',
    'PAGE_GUARD | PAGE_EXECUTE_WRITECOPY',
    'PAGE_NOACCESS',
    'PAGE_WRITECOMBINE | PAGE_READONLY',
    'PAGE_WRITECOMBINE | PAGE_EXECUTE',
    'PAGE_WRITECOMBINE | PAGE_EXECUTE_READ',
    'PAGE_WRITECOMBINE | PAGE_READWRITE',
    'PAGE_WRITECOMBINE | PAGE_WRITECOPY',
    'PAGE_WRITECOMBINE | PAGE_EXECUTE_READWRITE',
    'PAGE_WRITECOMBINE | PAGE_EXECUTE_WRITECOPY',
]

In other words, if vad.Flags.Protection >> 24 is 2 then the protection is 
PAGE_EXECUTE. 

A reference for the ordering is here:

http://www.reactos.org/pipermail/ros-diffs/2010-October/038897.html
http://doxygen.reactos.org/dc/d2c/ntoskrnl_2mm_2arm_2page_8c_source.html

For a while, I've defined the PROTECT_FLAGS in malware.py, but since this is 
something specific to Windows (not malware) I propose that we add it into the 
core. The question is where. From what I can tell, it hasn't changed across 
Windows versions. I was thinking we could do something like tcpip_vtypes.py 
even though its a set of constants and not a structure. 

Thoughts?

Original comment by michael.hale@gmail.com on 29 Sep 2011 at 11:58

[GoogleCodeExporter]

Hmm, I think we should take this one in two steps. First the simple one:

* Add code to translate the Protection constants (like 4, 18, 24) into 
something human readable like Read/Write/Execute. 

Given the definition of PROTECT_FLAGS above, you can gather an Enumeration type 
may work, but there are two problems I ran into:

* vad.Flags.Protection is already a BitField (can BitFields also be 
Enumerations?)
* the BitField value needs to be >> 24 before it can be used with PROTECT_FLAGS

It seems like we'd need a special obj.NativeType to make this work so that the 
BitField can be shifted >> 24 *and* the used as an Enumeration, but I'm not 
sure that's worth the trouble since the type would be specific to this one 
structure member. 

Do you think we should just include PROTECT_FLAGS as-is (just an array) in 
vadinfo.py or something and then change the vadinfo print statements to show 
PROTECT_FLAGS[vad.Flags.Protection >> 24]? 

Original comment by michael.hale@gmail.com on 12 Oct 2011 at 2:10

[GoogleCodeExporter]

Here's a patch for this issue (please also see the description/discussion in 
our team email).

Original comment by michael.hale@gmail.com on 11 Jan 2012 at 4:40

Attachments:

• issue_143_vad.patch

[GoogleCodeExporter]

Patch looks good to me, but please investigate if you can use hasattr over a 
try: except AttributeError.  Exception handling's expensive and we should only 
be using it to handle errors, not to do program flow control if possible...  5:)

Original comment by mike.auty@gmail.com on 11 Jan 2012 at 7:44

[GoogleCodeExporter]

This issue was closed by revision r1239.

Original comment by michael.hale@gmail.com on 11 Jan 2012 at 7:49

• Changed state: Fixed

[GoogleCodeExporter]

Patch looks great. Re use of hasattr vs try: except:, this really depends on a 
number of factors as discussed here:

http://stackoverflow.com/questions/903130/hasattr-vs-try-except-block-to-deal-wi
th-non-existent-attributes

In particular in the common case where the attribute exists a try/except is 
quite a bit faster than the hasattr approach. Also if you need to check more 
than one item this is much faster too. Try/except idiom is also more pythonic 
and easier to read.

The only time a hasattr approach might be better is when you know almost 
certainly that the attribute does not exist. In that case you will be paying 
the overhead of exception construction for each iteration. This does not appear 
to me to be the case here (in fact I cant see where the attribute could be 
missing). I think the usual behaviour is to use hasattr only in exceptional 
cases where you know its going to be faster, but generally use try/except for 
the enhanced readability.

Original comment by scude...@gmail.com on 11 Jan 2012 at 8:21

[GoogleCodeExporter]

The issue with EAFP (easier to ask forgiveness than permission) vs LBYL (look 
before you leap) is that when done wrong it hides bugs, and it's been done 
wrong in our code in several places.

I recently found a try/except that caught KeyError testing whether a list had a 
particular element, but also wrapped an extremely complex function that could 
(and did) return KeyErrors for multiple reasons.  We also have some try/excepts 
that still don't specify an exception type.

As such, I would not recommend EAFP unless everyone implementing it knows how 
to do it correctly, particularly given the complex nature of our object model's 
attribute lookup mechanism.  I also disagree in simple circumstances (such as 
this) that having three lines of try/except/pass over the one if check is 
easier to read.

Original comment by mike.auty@gmail.com on 6 Feb 2012 at 3:36

[GoogleCodeExporter]

No fix for 1) Show both the original *and* the current page protections ?

Original comment by Flo...@gmail.com on 16 Sep 2012 at 5:47