Fix undeclared next_section_virtual_address usage by gdesmar · Pull Request #429 · erocarrera/pefile

gdesmar · 2024-09-23T15:44:28Z

The following two files are making pefile crash with the next stacktrace:
aksdf.sys
hardlock.sys

  File ".../extract.py", line 937, in extract_setup_factory
    pe = pefile.PE(request.file_path, fast_load=True)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 2941, in __init__
    self.__parse__(name, data, fast_load)
  File ".../pefile.py", line 3313, in __parse__
    offset = self.parse_sections(sections_offset)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 3656, in parse_sections
    if section.Name.rstrip(b"\x00") == b"PAGE" and self.is_driver():
                                                   ^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 7865, in is_driver
    self.parse_data_directories(
  File ".../pefile.py", line 3768, in parse_data_directories
    value = entry[1](dir_entry.VirtualAddress, dir_entry.Size)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 5884, in parse_import_directory
    data = self.get_data(rva, image_import_descriptor_size)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 6370, in get_data
    s = self.get_section_by_rva(rva)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 6538, in get_section_by_rva
    if section.contains_rva(rva):
       ^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../pefile.py", line 1285, in contains_rva
    self.next_section_virtual_address is not None
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'SectionStructure' object has no attribute 'next_section_virtual_address'

From my understanding, the parse_sections function may end up calling self.is_driver() as it's parsing the sections, but before the parse_sections function goes over the newly parsed sections to configure the next_section_virtual_address variable. Since is_driver() ends up relying on it, it is crashing.

I believe the code that needs the is_driver() call is only adding warnings, so it would not cause any functional change to put it after the code assigning the value to next_section_virtual_address.

gdesmar · 2025-02-18T14:55:30Z

Another version of aksdf.sys and aksfridge.sys are also causing this issue.

gdesmar · 2025-04-23T18:11:57Z

@erocarrera, I was wondering if you had an opinion on this matter, or if you would need me to add a test for it?
There are always more versions of the same files that causes this issue: hardlock.sys, aksfridge.sys.
Thank you for your time.

nightlark · 2026-03-15T17:20:52Z

My initial take looking at this is it shouldn't break things.

@gdesmar I don't think I have a VT subscription that allows downloading the linked samples. Is there another way you can send a copy of them to be included as test cases?

gdesmar · 2026-03-16T12:47:34Z

Here is a password protected zip file containing this specific one (as it was the smallest of the ones I linked).
The password for the zip file is pefile.
Thank you for looking into it! :)

nightlark · 2026-03-28T21:36:18Z

pefile.py

        self.sections.sort(key=lambda a: a.VirtualAddress)
        for idx, section in enumerate(self.sections):
            if idx == len(self.sections) - 1:
                section.next_section_virtual_address = None
            else:
                section.next_section_virtual_address = self.sections[
                    idx + 1
                ].VirtualAddress


Not a change for this PR, but was looking at this block and thinking it could perhaps be rewritten in a way that makes it more readable.

for section, next_section in zip(self.sections, self.sections[1:]): section.next_section_virtual_address = next_section.VirtualAddress if self.sections: self.sections[-1].next_section_virtual_address = None

for index, section in enumerate(self.sections): is_last = index == len(self.sections) - 1 section.next_section_virtual_address = ( None if is_last else self.sections[index + 1].VirtualAddress )

nightlark · 2026-03-28T21:59:12Z

pefile.py

+        for section in self.sections:
+            if section.__dict__.get(
+                "IMAGE_SCN_MEM_WRITE", False
+            ) and section.__dict__.get("IMAGE_SCN_MEM_EXECUTE", False):
+                if section.Name.rstrip(b"\x00") == b"PAGE" and self.is_driver():
+                    # Drivers can have a PAGE section with those flags set without
+                    # implying that it is malicious
+                    pass
+                else:
+                    self.__warnings.append(
+                        f"Suspicious flags set for section {i}. "


It looks like i is stuck at self.FILE_HEADER.NumberOfSections. My initial impression is that updating the for loop to use enumerate would give an index that could be used in the warning message, and could be enough to address the issue for this PR.

However, I realize that self.sections gets sorted after that initial loop... which I think raises some questions as to what index the warning/error messages should be referring to. If it says there's an error in section 3 but then sorting moves what was at index 3 in the file to index 5 in the sorted list, inspecting sections with pefile would result in looking at the wrong section for the cause of the warning.

Essentially, which is better for users: error messages referring to the section number as it appears in the file, or after sorting by VirtualAddress?

The solution I'm leaning towards for the {i} numbering issue is that when the SectionStructure gets created in the first loop (start of this function), a field in the newly created section object (index_in_file?) could get set to i as a way to help users to find the section in the sorted list that corresponds to the warning messages they get.

And then in this warning message {section.index_in_file} can be used. Open to alternatives though if you've got ideas for how to handle the shifting index/section number issue.

Great catch on the {i}. I completed the rebase and added the small fix/improvement.
Looking forward to any other correction to do!

nightlark

Aside from the issue with the section number in the warning message, can you also rebase the PR on the latest master branch?

nightlark · 2026-03-28T22:24:46Z

pefile.py

+        for section in self.sections:
+            if section.__dict__.get(
+                "IMAGE_SCN_MEM_WRITE", False
+            ) and section.__dict__.get("IMAGE_SCN_MEM_EXECUTE", False):
+                if section.Name.rstrip(b"\x00") == b"PAGE" and self.is_driver():
+                    # Drivers can have a PAGE section with those flags set without
+                    # implying that it is malicious
+                    pass
+                else:
+                    self.__warnings.append(
+                        f"Suspicious flags set for section {i}. "


The solution I'm leaning towards for the {i} numbering issue is that when the SectionStructure gets created in the first loop (start of this function), a field in the newly created section object (index_in_file?) could get set to i as a way to help users to find the section in the sorted list that corresponds to the warning messages they get.

And then in this warning message {section.index_in_file} can be used. Open to alternatives though if you've got ideas for how to handle the shifting index/section number issue.

j-t-1

Nice work both.

j-t-1 · 2026-03-30T16:15:08Z

pefile.py

                    idx + 1
                ].VirtualAddress

+        for section in self.sections:


We may be able to use getattr(object, name[, default]) here and throughout the file?

getattr(section, "IMAGE_SCN_MEM_WRITE", False) and getattr(section, "IMAGE_SCN_MEM_EXECUTE", False)

Better outside of this PR as is not just in this function.

j-t-1 · 2026-03-30T16:31:47Z

pefile.py

-                        "This might indicate a packed executable."
-                    )
-
+            # Set the section's original index as it may change after being sorted by VirtualAddress


From the Microsoft official documentation:

The number of entries in the section table is given by the NumberOfSections field in the file header. Entries in the section table are numbered starting from one (1).

Due to us not doing, this is fine for this PR. After this PR could look into whether we need to change to one indexing or just add a comment to explain we use zero indexing. Check what PE-bear or similar does?

A comment saying we use zero indexing would be good. The output has been zero indexed for long enough that I don't think we want to change it at this time (it would also be a change that would be good to get thoughts from @erocarrera)

nightlark · 2026-03-31T06:59:26Z

I updated the regression tests with the reordered output lines due to the check being moved to after sorting has occurred instead of at the same time as the other checks for a given section.

It would be nice if they were still grouped together by section, but I don't think there is a clean way of doing that without introducing complexity with a temporary data structure to store warnings for each section before adding them to the _warnings list.

nightlark mentioned this pull request Jan 2, 2026

Combined PR #470

Closed

nightlark added this to the 2026 Release 1 milestone Mar 16, 2026

nightlark mentioned this pull request Mar 28, 2026

Exclude INIT section from packed executable warning #352

Open

nightlark reviewed Mar 28, 2026

View reviewed changes

nightlark requested changes Mar 28, 2026

View reviewed changes

Fix undeclared next_section_virtual_address usage

9366cec

gdesmar force-pushed the undeclared_next_section_virtual_address branch from b086f2e to 9366cec Compare March 30, 2026 14:23

Save the original section index and use it for logging.

fdfe543

gdesmar requested a review from nightlark March 30, 2026 15:32

j-t-1 reviewed Mar 30, 2026

View reviewed changes

nightlark mentioned this pull request Mar 31, 2026

Update dumps with results after fix for undeclared next_section_virtual_address attribute erocarrera/pefile-tests#7

Merged

nightlark approved these changes Mar 31, 2026

View reviewed changes

nightlark merged commit 9c485d4 into erocarrera:master Apr 1, 2026
26 of 47 checks passed

Conversation

gdesmar commented Sep 23, 2024

Uh oh!

gdesmar commented Feb 18, 2025

Uh oh!

gdesmar commented Apr 23, 2025

Uh oh!

nightlark commented Mar 15, 2026

Uh oh!

gdesmar commented Mar 16, 2026

Uh oh!

nightlark Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

nightlark Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightlark Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdesmar Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

nightlark left a comment

Choose a reason for hiding this comment

Uh oh!

nightlark Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-t-1 left a comment

Choose a reason for hiding this comment

Uh oh!

j-t-1 Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

j-t-1 Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

nightlark Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightlark commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nightlark Mar 28, 2026 •

edited

Loading

nightlark Mar 28, 2026 •

edited

Loading

nightlark Mar 28, 2026 •

edited

Loading

nightlark Mar 31, 2026 •

edited

Loading

nightlark commented Mar 31, 2026 •

edited

Loading