Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Fix bug with not returning all OPTIONAL_HEADER DataDirectory entries #451

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nightlark
Copy link

@nightlark nightlark commented Dec 12, 2024

Currently an assumption is made that the number of address/size pairs in the OPTIONAL_HEADER DataDirectory array is given by OPTIONAL_HEADER.NumberOfRvaAndSizes. From the looks of it, most implementations I've found just have a fixed size of 16 (see references below where this quirk seems to be "documented"). From how it seems to be getting used in practice for various PE files that don't set it to 16, NumberOfRvaAndSizes seems to be getting treated as count of how many of the pointers in the DataDirectory list are "null" (zero address and zero size) -- which is kinda obnoxious since it goes against what several of the Microsoft documentation pages say about using it to avoid probing too far.

This issue is the underlying cause of a bug report I got, LLNL/Surfactant#295, and is related to #264.

I can't share the actual file, but I've attached a screenshot from XPEViewer showing how the first entries are 0's for their address/size, and its only index 6 and onward that are actually set. If you can find a bootable UEFI kernel image, I think that would likely exhibit similar behavior. The NumberOfRvaAndSizes for this file is 6.

Before the changes in this PR, this is what pefile shows when I print OPTIONAL_HEADER.DATA_DIRECTORY, and you can see that only the first 6 (empty) entries are in the list (everything else is inaccessible...):

[<Structure: [IMAGE_DIRECTORY_ENTRY_EXPORT] 0xC8 0x0 VirtualAddress: 0x0 0xCC 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_IMPORT] 0xD0 0x0 VirtualAddress: 0x0 0xD4 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_RESOURCE] 0xD8 0x0 VirtualAddress: 0x0 0xDC 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_EXCEPTION] 0xE0 0x0 VirtualAddress: 0x0 0xE4 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_SECURITY] 0xE8 0x0 VirtualAddress: 0x0 0xEC 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_BASERELOC] 0xF0 0x0 VirtualAddress: 0x0 0xF4 0x4 Size: 0x0>]

After the changes in this PR, this is what pefile shows when I print OPTIONAL_HEADER.DATA_DIRECTORY, which matches what XPEViewer shows for the addresses and sizes of the data directory entries:

[<Structure: [IMAGE_DIRECTORY_ENTRY_EXPORT] 0xC8 0x0 VirtualAddress: 0x0 0xCC 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_IMPORT] 0xD0 0x0 VirtualAddress: 0x0 0xD4 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_RESOURCE] 0xD8 0x0 VirtualAddress: 0x0 0xDC 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_EXCEPTION] 0xE0 0x0 VirtualAddress: 0x0 0xE4 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_SECURITY] 0xE8 0x0 VirtualAddress: 0x0 0xEC 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_BASERELOC] 0xF0 0x0 VirtualAddress: 0x0 0xF4 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_DEBUG] 0xF8 0x0 VirtualAddress: 0x7865742E 0xFC 0x4 Size: 0x74>,
<Structure: [IMAGE_DIRECTORY_ENTRY_COPYRIGHT] 0x100 0x0 VirtualAddress: 0xAD4000 0x104 0x4 Size: 0x1000>,
<Structure: [IMAGE_DIRECTORY_ENTRY_GLOBALPTR] 0x108 0x0 VirtualAddress: 0xAD4000 0x10C 0x4 Size: 0x1000>,
<Structure: [IMAGE_DIRECTORY_ENTRY_TLS] 0x110 0x0 VirtualAddress: 0x0 0x114 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG] 0x118 0x0 VirtualAddress: 0x0 0x11C 0x4 Size: 0x60000020>,
<Structure: [IMAGE_DIRECTORY_ENTRY_BOUND_IMPORT] 0x120 0x0 VirtualAddress: 0x7461642E 0x124 0x4 Size: 0x61>,
<Structure: [IMAGE_DIRECTORY_ENTRY_IAT] 0x128 0x0 VirtualAddress: 0x21E000 0x12C 0x4 Size: 0xAD5000>,
<Structure: [IMAGE_DIRECTORY_ENTRY_DELAY_IMPORT] 0x130 0x0 VirtualAddress: 0x1C6200 0x134 0x4 Size: 0xAD5000>,
<Structure: [IMAGE_DIRECTORY_ENTRY_COM_DESCRIPTOR] 0x138 0x0 VirtualAddress: 0x0 0x13C 0x4 Size: 0x0>,
<Structure: [IMAGE_DIRECTORY_ENTRY_RESERVED] 0x140 0x0 VirtualAddress: 0x0 0x144 0x4 Size: 0xC0000040>]

image

References for DataDirectory having a fixed size of 16:

…tries

In the format for the PE optional header, the DataDirectory list has a fixed size of 16 address/size entries.
Previously, an incorrect assumption was made that the list of entries is dynamic and given by OPTIONAL_HEADER.NumberOfRvaAndSizes.
This resulted in only parsing the first entries in the list, which might not be the entries that are not empty.
The most extreme case of this was NumberOfRvaAndSizes being 6, and the first 6 data directories were all empty, so none of the directories with actual data were parsed.
@nightlark nightlark changed the title Fix bug with not parsing all OPTIONAL_HEADER DataDirectory entries Fix bug with not returning all OPTIONAL_HEADER DataDirectory entries Dec 12, 2024
@nightlark
Copy link
Author

Actually not sure if this is the best way to handle the issue. I was thinking SizeOfOptionalHeader mentioned in https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#optional-header-image-only as a thing to check to make sure data directory probes don't go too far -- but from a sample file, that field isn't a reliable indicator of the size of the optional header either (what???).

The conclusion I'm slowly approaching is that this aspect of the PE file format is a mess, since the data directory array is documented as being a variable length in some places, but then most implementations treat it as being a fixed size of 16 and don't bother checking either SizeOfOptionalHeader or NumberOfRvaAndSizes when probing for directories.

I'm leaning towards perhaps the only way to make everyone happy is provide a flag for users to specify how they want it treated -- a "strict" mode for NumberOfRvaAndSizes bounding how far the probe happens, and a fixed size method that assumes all 16 entries are present with NumberOfRvaAndSizes just being reduced by how many "missing" entries there are (ones whose address and size == 0). Or a "heuristic" mode that parses the data directories but doesn't count entries with address and size == 0 as being present.

@nightlark nightlark changed the title Fix bug with not returning all OPTIONAL_HEADER DataDirectory entries [DRAFT] Fix bug with not returning all OPTIONAL_HEADER DataDirectory entries Dec 12, 2024
@j-t-1
Copy link
Contributor

j-t-1 commented Dec 13, 2024

pefile has IMAGE_NUMBEROF_DIRECTORY_ENTRIES = 16 so any changes could use this. This variable is currently unused, but thanks to reading this I created #452 to use it elsewhere in the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants