Perform additional checks in PEHeaders.

[teo-tsirpanis]

Fixes #48419.
Supersedes #112830 for now.

See also #112830 (comment).

This PR updates the constructor of PEHeaders to perform some additional checks to detect files with an obviously invalid structure. Specifically:

• CoffHeader.SizeOfOptionalHeader is checked to be zero, in object files.
• CoffHeader.PointerToSymbolTable is checked to point inside the file, in object files.
• CoffHeader.Characteristics is checked to not have the image-only bits set, in object files.
• For each section, SectionHeader.VirtualSize is checked to be zero, in object files.
• The size of the file is checked to fit as many as CoffHeader.NumberOfSections sections, in both object and image files.

PR Overview

This PR improves the robustness of PE file parsing by adding additional validity checks for object files (COFF files). The key changes include:

• Introducing stricter validation for COFF files by verifying SizeOfOptionalHeader, PointerToSymbolTable, and Characteristics.
• Updating section header reading to ensure file boundaries are not exceeded.
• Adding a per-section check to confirm that the VirtualSize is zero in COFF files.

Reviewed Changes

File	Description
src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs	Added COFF file validation and enhanced section header boundary checks

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs:102

• [nitpick] Verify that the check for '_coffHeader.PointerToSymbolTable >= size' correctly handles edge cases where the pointer equals the file size, ensuring no valid COFF files are mistakenly rejected.

if (_coffHeader.SizeOfOptionalHeader != 0 || _coffHeader.PointerToSymbolTable >= size || (_coffHeader.Characteristics & ImageOnlyCharacteristics) != 0)

src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs:308

• The boundary check for section headers is important; please verify that the arithmetic correctly prevents buffer overreads when the reader's offset is near the end of the file.

if (numberOfSections < 0 || numberOfSections * SectionHeader.Size > size - reader.Offset)

src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs:318

• [nitpick] The new check ensuring 'VirtualSize' is zero in COFF files is a critical validation step; confirm that this does not inadvertently flag any edge case valid files.

if (isCoffOnly && sectionHeader.VirtualSize != 0)

[teo-tsirpanis]

#112830 (comment) said that VirtualAddress is the field that must be zero in object files, but according to documentation it's VirtualSize.

[steveharter]

That sounds right. For VirtualAddress the doc says "For object files, this field is the address of the first byte before relocation is applied; for simplicity, compilers should set this to zero.". So yeah, we probably don't want to enforce zero for object files. Even for executable files this may be zero for some sections like .debug.

[jkotas]

Test failures:

    System.Reflection.Metadata.Tests.AssemblyDefinitionTests.ValidateAssemblyNameWithCultureSet [FAIL]
      System.BadImageFormatException : Invalid number of sections declared in PE header.
      Stack Trace:
        /_/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs(326,0): at System.Reflection.PortableExecutable.PEHeaders.ReadSectionHeaders(PEBinaryReader& reader, Int32 size, Boolean isCoffOnly)
        /_/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs(115,0): at System.Reflection.PortableExecutable.PEHeaders..ctor(Stream peStream, Int32 size, Boolean isLoadedImage)
        /_/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs(59,0): at System.Reflection.PortableExecutable.PEHeaders..ctor(Stream peStream, Int32 size)
        /_/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaders.cs(44,0): at System.Reflection.PortableExecutable.PEHeaders..ctor(Stream peStream)
        /_/src/libraries/System.Reflection.Metadata/tests/Metadata/MetadataReaderTests.cs(79,0): at System.Reflection.Metadata.Tests.MetadataReaderTests.GetMetadataReader(Byte[] peImage, Int32& metadataStartOffset, Boolean isModule, MetadataReaderOptions options, MetadataStringDecoder decoder)
        /_/src/libraries/System.Reflection.Metadata/tests/Metadata/MetadataReaderTests.cs(73,0): at System.Reflection.Metadata.Tests.MetadataReaderTests.GetMetadataReader(Byte[] peImage, Boolean isModule, MetadataReaderOptions options, MetadataStringDecoder decoder)
        /_/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeSystem/AssemblyDefinitionTests.cs(15,0): at System.Reflection.Metadata.Tests.AssemblyDefinitionTests.ValidateAssemblyNameWithCultureSet()

[teo-tsirpanis]

Most test failures were due to typos, but in one test we were opening an .snk file, which now (correctly) throws. That test was updated and moved to PEHeaders.

[steveharter]

What case is this catching from above, and related question is what coverage do we have on the new checks?

[teo-tsirpanis]

It catches the "size of optional header is nonzero" case. I can add more tests.

[MichalPetryka]

Should this be labeled as a breaking change?

[MichalStrehovsky]

The list of heuristics I gave in #112830 (comment) is somewhat haphazard. Now that I'm looking at the actual code, I wonder if it would be better to do fewer of these validations (it also helps in having to write fewer tests since there's fewer codepaths that need to be covered).

The things that we need to validate are things that would make us crash in bad ways or are structurally undefined:

• SizeOfOptionalHeader - if this is non-zero, there is an optional header following the COFF header that we should presumably read or skip so that we don't read optional header data as section headers. But this is all undefined territories. So throw if it's non-zero.
• Checking PointerToSymbolTable and Characteristics is one of the haphazard heuristics I gave. We don't really care if these are nonsensical, so maybe we don't need to look at them at all.
• Checking that we can read all the declared section headers and don't crash with a stream read exception is important, so we should do the validation.
• Checking the VirtualSize of the section doesn't really matter.

We'll allow more COFF object file-like files but looking at the APIs on PEHeaders, this should be fine:

• CoffHeader, SectionHeaders is fine - the values will be junk but the API doesn't judge. Trying to filter some of the junk wouldn't move the needle, even with the validation we'd let junk through.
• CoffHeaderStartOffset, IsCoffOnly, CorHeader, CorHeaderStartOffset, PEHeader, PEHeaderStartOffset all depend on this being an image so these APIs will be fine
• MetadataStartOffset and MetadataSize will be null because none of the junk SectionHeaders will have name .cormeta

The 99% use case of PEHeader is for getting MetadataStartOffset and MetadataSize so that we can get a metadata reader. And this is going to be pretty well defined if we do the above validation that prevents crashes only.

[teo-tsirpanis]

Updated PR to check only for SizeOfOptionalHeader and NumberOfSections and added test coverage for these cases. I'm not sure I agree with making less checks; we don't have an explicit indicator that a file is not a COFF file, so we should check for as many implicit indicators as reasonably possible in order to fail with a clear message, but for the invalid files that pass these two checks users will either get an exception or a negative HasMetadata result later either way.

[teo-tsirpanis]

Suggested change

	{
	{
	if (isLoadedImage)
	{
	throw new BadImageFormatException(SR.InvalidPESignature);
	}

What do you think about adding this check (a bit earlier)? The OS can't load an object file, can it?