PE Loader improvement #173
Conversation
WalkthroughThe executable loader module is enhanced with comprehensive error handling and validation. Methods are refactored to return bool instead of void, enabling failure detection. A destructor and cleanup routine manage allocated memory. PE header validation, bounds checking, relocation error handling, and TLS callback execution are added throughout the loading pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Loader as ExecutableLoader
participant Mem as Memory
participant PE as PE Image
App->>Loader: LoadIntoModule(module)
activate Loader
Loader->>PE: ValidatePEHeaders()
alt Invalid Headers
Loader-->>App: return false
Loader->>Mem: CleanupOnError()
else Valid Headers
Note over Loader: Set _imageSize
Loader->>Loader: LoadSections()
alt Section Load Failed
Loader->>Mem: CleanupOnError()
Loader-->>App: return false
else Success
Loader->>Loader: LoadImports()
alt Import Load Failed
Loader->>Mem: CleanupOnError()
Loader-->>App: return false
else Success
Loader->>Loader: ApplyRelocations()
alt Relocation Failed
Loader->>Mem: CleanupOnError()
Loader-->>App: return false
else Success
Loader->>Loader: ExecuteTLSCallbacks()
Loader-->>App: return true
end
end
end
end
deactivate Loader
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/framework/src/launcher/loaders/exe_ldr.cpp (2)
323-328: Fix return type mismatch.Line 328 has a bare
return;statement, butLoadDelayImportsreturnsbool. This should bereturn true;to indicate successful handling of the no-delay-imports case.Apply this diff:
if (delayImportDirectory->VirtualAddress == 0 || delayImportDirectory->Size == 0) { - return; + return true; }
361-368: Critical: Race condition in LoadDelayImports - static buffer not fixed.Lines 366-368 still use a
static char ordinalNameBuf[32]buffer for ordinal formatting, creating the same race condition that was fixed inLoadImports(lines 150-152) usingthread_local. This is inconsistent and defeats the purpose of the security fix.Apply this diff to match the fix in
LoadImports:if (IMAGE_SNAP_BY_ORDINAL(nameArray[i].u1.Ordinal)) { WORD ordinal = IMAGE_ORDINAL(nameArray[i].u1.Ordinal); function = GetProcAddress(module, (LPCSTR)(ULONG_PTR)ordinal); - static char ordinalNameBuf[32]; - sprintf_s(ordinalNameBuf, "#%u", ordinal); + thread_local char ordinalNameBuf[64]; + ::snprintf(ordinalNameBuf, sizeof(ordinalNameBuf), "#%u", ordinal); functionName = ordinalNameBuf;
🧹 Nitpick comments (2)
code/framework/src/launcher/loaders/exe_ldr.cpp (1)
408-415: Simplify redundant platform-specific typedefs.The
TLS_CALLBACK_PROCtypedef is identical for both_M_AMD64and_M_IX86platforms. This can be simplified by removing the conditional compilation.Apply this diff:
// Execute TLS callbacks if they exist if (tlsData->AddressOfCallBacks) { -#ifdef _M_AMD64 typedef void (NTAPI *TLS_CALLBACK_PROC)(PVOID, DWORD, PVOID); TLS_CALLBACK_PROC *callbacks = reinterpret_cast<TLS_CALLBACK_PROC *>(tlsData->AddressOfCallBacks); -#else - typedef void (NTAPI *TLS_CALLBACK_PROC)(PVOID, DWORD, PVOID); - TLS_CALLBACK_PROC *callbacks = reinterpret_cast<TLS_CALLBACK_PROC *>(tlsData->AddressOfCallBacks); -#endifcode/framework/src/launcher/loaders/exe_ldr.h (1)
28-28: Remove unused TLSCallbackProc typedef or consolidate with local definition in ExecuteTLSCallbacks.Verification confirms
TLSCallbackProcis never used. TheExecuteTLSCallbacksfunction (exe_ldr.cpp:410, 413) defines its own local typedef with identical signature instead. Remove the public typedef at exe_ldr.h:28, or refactorExecuteTLSCallbacksto use it for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/framework/src/launcher/loaders/exe_ldr.cpp(10 hunks)code/framework/src/launcher/loaders/exe_ldr.h(3 hunks)code/framework/src/launcher/project.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
code/framework/src/launcher/loaders/exe_ldr.h (1)
code/framework/src/launcher/loaders/exe_ldr.cpp (24)
LoadSection(179-287)LoadSection(179-179)LoadSections(289-300)LoadSections(289-289)ApplyRelocations(545-646)ApplyRelocations(545-545)LoadExceptionTable(310-320)LoadExceptionTable(310-310)LoadImports(101-177)LoadImports(101-101)LoadDelayImports(323-392)LoadDelayImports(323-323)ValidatePEHeaders(53-99)ValidatePEHeaders(53-53)ExecuteTLSCallbacks(394-426)ExecuteTLSCallbacks(394-394)CleanupOnError(44-51)CleanupOnError(44-44)ResolveLibrary(648-650)ResolveLibrary(648-648)LoadIntoModule(428-543)LoadIntoModule(428-428)ExecutableLoader(23-38)ExecutableLoader(40-42)
code/framework/src/launcher/loaders/exe_ldr.cpp (1)
code/framework/src/launcher/loaders/exe_ldr.h (1)
ExecutableLoader(24-78)
🔇 Additional comments (9)
code/framework/src/launcher/loaders/exe_ldr.h (2)
64-78: Excellent bounds checking additions.The bounds checks in both
GetRVAandGetTargetRVAproperly prevent out-of-bounds memory access by validating RVAs against their respective size limits before dereferencing.
47-48: LGTM - Strong API improvements for error handling.The changes to return
boolfrom loader methods and the addition ofValidatePEHeaders,ExecuteTLSCallbacks,CleanupOnError, and the destructor significantly improve error handling and resource management.Also applies to: 56-60, 104-105
code/framework/src/launcher/project.cpp (1)
721-727: LGTM - Proper error handling for LoadIntoModule.The error handling correctly checks the boolean return value from
LoadIntoModuleand performs proper cleanup by unmapping the file view and closing handles before returning failure.code/framework/src/launcher/loaders/exe_ldr.cpp (6)
23-42: LGTM - Proper RAII implementation.The constructor correctly initializes
_imageSize, and the destructor ensures cleanup by callingCleanupOnError(), providing proper resource management.
53-99: Excellent PE header validation.The comprehensive validation of PE headers including signatures, machine types, section counts, and data directory limits effectively prevents malformed PE files from being processed.
101-177: LGTM - Race condition fixed with thread_local.The use of
thread_localforordinalNameBuf(lines 150-152) properly eliminates the race condition from the previous static buffer implementation. Error handling is comprehensive with proper validation and early returns.
179-287: Excellent security improvements in LoadSection.The comprehensive validation including bounds checking (lines 181-185, 204-208), integer overflow protection (lines 211-219), and deferred protection application (lines 281-284) significantly improves security and eliminates race conditions.
428-543: LGTM - Comprehensive error handling in LoadIntoModule.The refactored
LoadIntoModulemethod now includes:
- Proper validation at each step with early returns
- Consistent cleanup via
CleanupOnError()on failures- Detailed error logging
- TLS callback execution
This significantly improves robustness and security.
545-646: Excellent relocation handling improvements.The enhanced
ApplyRelocationsmethod now includes:
- Validation of relocation target RVAs (lines 582-585)
- Support for additional relocation types (HIGH and LOW at lines 595-611)
- Per-relocation protection changes with proper restoration (lines 635-638)
- Detailed error logging for all failure cases
This significantly improves both functionality and security.
|
|
||
| fu2::function<void(void **base, uint32_t *index)> _tlsInitializer; | ||
| std::vector<std::tuple<void *, DWORD, DWORD>> _targetProtections; | ||
| std::vector<void *> _allocatedMemory; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that _allocatedMemory is being populated.
The _allocatedMemory vector is declared and freed in CleanupOnError(), but no code appears to populate it via push_back(). If this is intended for tracking allocations for cleanup, ensure all relevant allocations are being tracked.
🏁 Script executed:
#!/bin/bash
# Search for push_back operations on _allocatedMemory
rg -n "_allocatedMemory\.push_back" --type=cppLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for all references to _allocatedMemory
rg -n "_allocatedMemory" --type=cpp -C 3Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Search for CleanupOnError function to see how _allocatedMemory is used
rg -n "CleanupOnError" --type=cpp -A 15Length of output: 5737
🏁 Script executed:
#!/bin/bash
# Check if there are any memory allocation or tracking patterns in the exe_ldr files
fd -e h -e cpp "exe_ldr" --exec cat {}Length of output: 31348
The review comment is accurate and should be addressed.
After verification, _allocatedMemory is indeed declared but never populated. No push_back() calls exist anywhere in the codebase, and none of the memory allocation operations (in LoadSections(), LoadImports(), LoadDelayImports(), ApplyRelocations(), or LoadIntoModule()) add entries to this vector.
Either remove the unused vector, or if allocation tracking is required, implement the tracking by adding push_back() calls wherever memory is allocated during the executable loading process.
🤖 Prompt for AI Agents
In code/framework/src/launcher/loaders/exe_ldr.h around line 44, the member
std::vector<void *> _allocatedMemory is declared but never populated; either
remove this unused member or implement allocation tracking. If removing: delete
the member declaration and any references (if any) to it. If keeping: update all
allocation sites in LoadSections(), LoadImports(), LoadDelayImports(),
ApplyRelocations(), and LoadIntoModule() to push_back the allocated pointers
into _allocatedMemory immediately after each successful allocation and ensure
they are freed/cleared from this vector during cleanup to avoid leaks. Ensure
thread-safety if allocations can occur concurrently (use mutex or document
single-threaded usage).
Fix Critical Security Issues and Improve Robustness in PE Loader
Summary
This PR addresses multiple critical security vulnerabilities and missing features in the PE loader (
exe_ldr.cpp), significantly improving its robustness and securityposture.
Critical Security Fixes
New Features
IMAGE_REL_BASED_HIGH/IMAGE_REL_BASED_LOWrelocationsRobustness Improvements
Breaking Changes
LoadIntoModule()now returnsboolinstead ofvoid- calling code updated accordinglyFiles Modified
code/framework/src/launcher/loaders/exe_ldr.h- Added new methods and improved templatescode/framework/src/launcher/loaders/exe_ldr.cpp- Complete security overhaulcode/framework/src/launcher/project.cpp- Updated error handling for new return valuesResolves security vulnerabilities that could lead to arbitrary code execution or memory corruption during PE loading.
Summary by CodeRabbit