Reflective Loader for extensions in metsrv#797
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a custom “manual-map” style reflective loader to metsrv so in-memory extension DLLs can be loaded directly from a buffer pointer (without scanning for the PE header), addressing the linked extension-loading issue.
Changes:
- Introduces
extension_loader.c/.himplementing PE mapping (sections, relocations, imports, permissions) and invoking the DLL entry point. - Updates
request_core_loadlibto useLoadReflectively()for non-disk extension loading (with fallback tolibloader_load_library()). - Updates the Visual Studio project files to compile and include the new loader sources.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| c/meterpreter/workspace/metsrv/metsrv.vcxproj.filters | Adds the new loader files to the VS filters. |
| c/meterpreter/workspace/metsrv/metsrv.vcxproj | Adds the new loader files to the VS build. |
| c/meterpreter/source/metsrv/remote_dispatch.c | Switches in-memory extension loading to the new loader and adds related logging/formatting changes. |
| c/meterpreter/source/metsrv/extension_loader.c / .h | New implementation of the custom reflective loader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 3. Fix relocations | ||
| dprintf("[LOADREFLECTIVELY] Fixing relocations\n"); | ||
| if(!FixRelocations(pRelocationTableDirectory, (ULONG_PTR)pDLLBaseAddress, pNtHeaders->OptionalHeader.ImageBase)) | ||
| { | ||
| dprintf("[LOADREFLECTIVELY] Failed to fix relocations\n"); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
On failure after VirtualAlloc, LoadReflectively returns without freeing pDLLBaseAddress, leaking memory (e.g. when FixRelocations/FixIAT/FixPermissions/DllMain fails). Ensure all error paths release the allocated image (and consider resetting *phModule consistently).
There was a problem hiding this comment.
Copilot is right here, we have as a standard flow to use:
do {
BREAK_WITH_ERROR...
}while(0);
where we use a do-while block as try-catch. it's ugly but it works... check on poolparty.c to see how is done.
|
|
||
|
|
There was a problem hiding this comment.
Two extra blank lines were added here, creating unnecessary vertical whitespace. Please remove the redundant blank lines to keep formatting consistent in this file.
| BOOL FixRelocations(PIMAGE_DATA_DIRECTORY dRelocBaseAddress, ULONG_PTR pBaseAddress, ULONG_PTR pPreferrableAddress) | ||
| { | ||
| ULONG_PTR delta = pBaseAddress - pPreferrableAddress; | ||
| PIMAGE_BASE_RELOCATION pBaseRelocationTable = (PIMAGE_BASE_RELOCATION)(pBaseAddress + dRelocBaseAddress->VirtualAddress); |
There was a problem hiding this comment.
FixRelocations computes delta as an unsigned ULONG_PTR. If the allocated base is lower than the preferred image base, this underflows and relocations will be applied incorrectly. Use a signed type (e.g. LONG_PTR/intptr_t) for the delta and apply it accordingly for 32-bit/64-bit relocation types.
There was a problem hiding this comment.
I am not too sure about it, i will investigate
There was a problem hiding this comment.
original implementation uses ULONG_PTR I would lean tword leave it as it is if nothing is exploding
There was a problem hiding this comment.
Alright, sounds good to me
|
|
||
| dprintf("[FIXPERM] Setting permissions for section %p to %x with size %x\n",pBaseAddress + pSectionHeader[i].VirtualAddress , dwProtection, pSectionHeader[i].SizeOfRawData); | ||
| LPVOID lpAddress = (PBYTE)pBaseAddress + pSectionHeader[i].VirtualAddress; | ||
| if(met_api->win_api.kernel32.VirtualProtect(&(lpAddress), pSectionHeader[i].SizeOfRawData, dwProtection, &dwOldFlags) == 0) |
There was a problem hiding this comment.
VirtualProtect is called with &(lpAddress) (a pointer-to-pointer) instead of the section base address. This will attempt to change protections on the stack variable rather than the mapped image. Pass lpAddress directly.
| if(met_api->win_api.kernel32.VirtualProtect(&(lpAddress), pSectionHeader[i].SizeOfRawData, dwProtection, &dwOldFlags) == 0) | |
| if(met_api->win_api.kernel32.VirtualProtect(lpAddress, pSectionHeader[i].SizeOfRawData, dwProtection, &dwOldFlags) == 0) |
There was a problem hiding this comment.
@msutovsky-r7 I think this is a good catch from Copilot.
| #include "common.h" | ||
|
|
||
| typedef struct _BASE_RELOCATION_ENTRY { | ||
| WORD Offset : 12; // Specifies where the base relocation is to be applied. | ||
| WORD Type : 4; // Indicates the type of base relocation to be applied. | ||
| } BASE_RELOCATION_ENTRY, * PBASE_RELOCATION_ENTRY; | ||
|
|
||
| BOOL LoadReflectively(IN ULONG_PTR lpBuffer, OUT HMODULE* phModule); No newline at end of file |
There was a problem hiding this comment.
extension_loader.h is missing an include guard (or #pragma once). All other metsrv headers appear to use #ifndef/#define/#endif guards (e.g. libloader.h), so this risks accidental double-inclusion and breaks the established header pattern.
| PIMAGE_BASE_RELOCATION pBaseRelocationTable = (PIMAGE_BASE_RELOCATION)(pBaseAddress + dRelocBaseAddress->VirtualAddress); | ||
| PBASE_RELOCATION_ENTRY pBaseRelocEntry = NULL; | ||
|
|
||
| while (pBaseRelocationTable->SizeOfBlock != 0) |
There was a problem hiding this comment.
FixRelocations walks the relocation blocks until it sees a SizeOfBlock == 0, but it never validates dRelocBaseAddress->VirtualAddress/Size nor enforces an upper bound. If the image has no relocation directory (common when stripped) or the directory is malformed, this will read/patch out of bounds. Guard on dRelocBaseAddress->Size != 0 and stop after processing Size bytes.
| PIMAGE_BASE_RELOCATION pBaseRelocationTable = (PIMAGE_BASE_RELOCATION)(pBaseAddress + dRelocBaseAddress->VirtualAddress); | |
| PBASE_RELOCATION_ENTRY pBaseRelocEntry = NULL; | |
| while (pBaseRelocationTable->SizeOfBlock != 0) | |
| // Validate relocation directory before using it. If there is no relocation | |
| // directory (common for stripped images) or it is malformed, treat this as | |
| // "no relocations to apply" instead of walking arbitrary memory. | |
| if (dRelocBaseAddress == NULL || | |
| dRelocBaseAddress->Size == 0 || | |
| dRelocBaseAddress->VirtualAddress == 0) | |
| { | |
| return TRUE; | |
| } | |
| PIMAGE_BASE_RELOCATION pBaseRelocationTable = (PIMAGE_BASE_RELOCATION)(pBaseAddress + dRelocBaseAddress->VirtualAddress); | |
| PBASE_RELOCATION_ENTRY pBaseRelocEntry = NULL; | |
| PBYTE relocDirStart = (PBYTE)pBaseRelocationTable; | |
| PBYTE relocDirEnd = relocDirStart + dRelocBaseAddress->Size; | |
| // Walk relocation blocks but never past the bounds of the relocation directory. | |
| while ((PBYTE)pBaseRelocationTable < relocDirEnd && | |
| pBaseRelocationTable->SizeOfBlock != 0 && | |
| (PBYTE)pBaseRelocationTable + pBaseRelocationTable->SizeOfBlock <= relocDirEnd) |
| #if defined(_M_ARM) | ||
| case IMAGE_REL_BASED_ARM_MOV32T: | ||
| DWORD dwInstruction = *(DWORD *)((ULONG_PTR)pBaseAddress + pBaseRelocationTable->VirtualAddress + pBaseRelocEntry->Offset + sizeof(DWORD)); | ||
| // Flip the words to get the instruction as expected (account for endianness/instruction packing). | ||
| dwInstruction = MAKELONG(HIWORD(dwInstruction), LOWORD(dwInstruction)); | ||
| if ((dwInstruction & ARM_MOV_MASK) == ARM_MOVT) | ||
| { | ||
| // Pull out the encoded 16-bit immediate value (high portion of the address-to-relocate). | ||
| WORD wImm = (WORD)(dwInstruction & 0x000000FF); | ||
| wImm |= (WORD)((dwInstruction & 0x00007000) >> 4); | ||
| wImm |= (WORD)((dwInstruction & 0x04000000) >> 15); | ||
| wImm |= (WORD)((dwInstruction & 0x000F0000) >> 4); | ||
| // Apply the relocation delta to the target address. | ||
| DWORD dwAddress = ((WORD)HIWORD(uiDelta) + wImm) & 0xFFFF; | ||
| // Create a new instruction with the same opcode and register parameters. | ||
| dwInstruction &= ARM_MOV_MASK2; | ||
| // Patch in the relocated address, re-encoding the immediate value. | ||
| dwInstruction |= (DWORD)(dwAddress & 0x00FF); | ||
| dwInstruction |= (DWORD)(dwAddress & 0x0700) << 4; | ||
| dwInstruction |= (DWORD)(dwAddress & 0x0800) << 15; | ||
| dwInstruction |= (DWORD)(dwAddress & 0xF000) << 4; | ||
| // Flip the instructions words and patch back into the code. | ||
| *(DWORD *)((ULONG_PTR)pBaseAddress + pBaseRelocationTable->VirtualAddress + pBaseRelocEntry->Offset + sizeof(DWORD)) = MAKELONG(HIWORD(dwInstruction), LOWORD(dwInstruction)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The _M_ARM relocation case references uiDelta (undefined) and uses ARM_MOV_MASK/ARM_MOV_MASK2/ARM_MOVT which are not declared in this compilation unit. Even though metsrv doesn't currently build for ARM, this block will not compile if an ARM target is added. Either remove this block or make it self-contained and correct (including a break after the case).
| #if defined(_M_ARM) | |
| case IMAGE_REL_BASED_ARM_MOV32T: | |
| DWORD dwInstruction = *(DWORD *)((ULONG_PTR)pBaseAddress + pBaseRelocationTable->VirtualAddress + pBaseRelocEntry->Offset + sizeof(DWORD)); | |
| // Flip the words to get the instruction as expected (account for endianness/instruction packing). | |
| dwInstruction = MAKELONG(HIWORD(dwInstruction), LOWORD(dwInstruction)); | |
| if ((dwInstruction & ARM_MOV_MASK) == ARM_MOVT) | |
| { | |
| // Pull out the encoded 16-bit immediate value (high portion of the address-to-relocate). | |
| WORD wImm = (WORD)(dwInstruction & 0x000000FF); | |
| wImm |= (WORD)((dwInstruction & 0x00007000) >> 4); | |
| wImm |= (WORD)((dwInstruction & 0x04000000) >> 15); | |
| wImm |= (WORD)((dwInstruction & 0x000F0000) >> 4); | |
| // Apply the relocation delta to the target address. | |
| DWORD dwAddress = ((WORD)HIWORD(uiDelta) + wImm) & 0xFFFF; | |
| // Create a new instruction with the same opcode and register parameters. | |
| dwInstruction &= ARM_MOV_MASK2; | |
| // Patch in the relocated address, re-encoding the immediate value. | |
| dwInstruction |= (DWORD)(dwAddress & 0x00FF); | |
| dwInstruction |= (DWORD)(dwAddress & 0x0700) << 4; | |
| dwInstruction |= (DWORD)(dwAddress & 0x0800) << 15; | |
| dwInstruction |= (DWORD)(dwAddress & 0xF000) << 4; | |
| // Flip the instructions words and patch back into the code. | |
| *(DWORD *)((ULONG_PTR)pBaseAddress + pBaseRelocationTable->VirtualAddress + pBaseRelocEntry->Offset + sizeof(DWORD)) = MAKELONG(HIWORD(dwInstruction), LOWORD(dwInstruction)); | |
| } | |
| #endif |
| dprintf("[FIXPERM] Setting permissions for section %p to %x with size %x\n",pBaseAddress + pSectionHeader[i].VirtualAddress , dwProtection, pSectionHeader[i].SizeOfRawData); | ||
| LPVOID lpAddress = (PBYTE)pBaseAddress + pSectionHeader[i].VirtualAddress; | ||
| if(met_api->win_api.kernel32.VirtualProtect(&(lpAddress), pSectionHeader[i].SizeOfRawData, dwProtection, &dwOldFlags) == 0) |
There was a problem hiding this comment.
The size passed to VirtualProtect is pSectionHeader[i].SizeOfRawData. This can be 0 for some sections, and it can be smaller than the in-memory size (Misc.VirtualSize), causing VirtualProtect failures or leaving part of a section with incorrect protection. Use an in-memory size (typically VirtualSize, rounded up to page alignment) and skip/handle zero-sized sections safely.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2889693 to
f037526
Compare
…lective-loader Collab: Reflective Loader for Meterpreter Extensions
|
I see is failing something with LoadLibraryR... checking it |
…lective-loader Reflective Loader Fixing Stuff
Fixes this issue
This PR
is a first draft foradds custom extension reflective loader in Metsrv. The reflective loader is similar to the reflective loader already used, however, it does not search for start of PE file - it rather accepts pointer to DLL as argument.x64x86