Skip to content

Bug results in crash when loading some DLLs. (with code fix!) #101

@Elmue

Description

@Elmue

I tried to load a DLL which is protected with WinLicense into a process.
When calling the entry point I got a crash when Data Execution Prevention was enabled for the process.
I analyzed the problem and found that fancycode has a fat bug which has not yet been reported here.
These bugs are not relevant when you load a "normal" DLL.
But a DLL which is protected with WinLicense has strong anti-debugging techniques built in.
WinLicense modifies the executing code at runtime!
It permanently writes into the code section.

Apart from that I found fancycode (written by Joachim Bauch) is very clumsy and awkward.
There is lot of code which is superfluous and far more complicated than necessary.

For example in CopySections() he calls
memset(dest, 0, section_size);
This is nonsense because VirtualAlloc() already returns memory which is zeroed.

It is also nonsense that he calls VirtualAlloc() first for the entire image of the DLL (in MemoryLoadLibraryEx()) and then he calls VirtualAlloc() again for each section in CopySections() although the memory is already allocated.

It is also unnecessary to call VirtualFree() in FinalizeSection() in the case that a section has the flag IMAGE_SCN_MEM_DISCARDABLE.
I studied how the Microsoft's LoadLibrary() works and it does NOT free sections which are marked as discardable.

But the real fat bug is in FinalizeSections()
When he sets the protection flags in FinalizeSection() he passes the wrong size.
This results in the crash in my WinLicense protected DLL if DEP is enabled.

Correct would be:
VirtualProtect(..., Section->Misc.VirtualSize, ...)
But he uses instead
VirtualProtect(..., GetRealSectionSize(), ...)

This is wrong because the entire section must have the Execute flag, not only the first part where he has copied to with memcpy().
It is wrong because there are DLLs which unpack or modify code at runtime.
If the pages where the code is copied to do not have the Execute flag set, Data Execution Prevention triggers an Access Violation.

I don't post a pull request because nobody cares here since years.
This is a dead project. Joachim Bauch neither merges pull requests nor does he answer emails.
So if you are interested take my code and fix it on your own.


// Copy all sections from file data (pu8_Data) to virtual address in memory
// Returns API error
DWORD CopySections(const BYTE* pu8_Data, DWORD u32_Size, MEMORYMODULE* pk_ModData)
{
    IMAGE_SECTION_HEADER* pk_Section = IMAGE_FIRST_SECTION(pk_ModData->headers); 
    for (int i=0; i<pk_ModData->headers->FileHeader.NumberOfSections; i++, pk_Section++)
    {
        size_t u32_SectSizeAligned = AlignValueUp(pk_Section->Misc.VirtualSize, pk_ModData->pageSize);

        if (u32_Size < pk_Section->PointerToRawData + pk_Section->SizeOfRawData ||
            pk_Section->SizeOfRawData > u32_SectSizeAligned)
            return ERROR_BAD_EXE_FORMAT;

        BYTE* pu8_Dest = pk_ModData->codeBase + pk_Section->VirtualAddress;
        memcpy(pu8_Dest, pu8_Data + pk_Section->PointerToRawData, pk_Section->SizeOfRawData);
    }
    return 0;
}

// Loop through all sections and set the access flags (Read, Write, Execute,...)
// Returns API error
DWORD FinalizeSections(MEMORYMODULE* pk_ModData)
{
    IMAGE_SECTION_HEADER* pk_Section = IMAGE_FIRST_SECTION(pk_ModData->headers);
    for (int i=0; i<pk_ModData->headers->FileHeader.NumberOfSections; i++, pk_Section++) 
    {
        DWORD u32_Size = pk_Section->Misc.VirtualSize;
        if (u32_Size == 0) 
            continue;

        BOOL b_Executable = (pk_Section->Characteristics & IMAGE_SCN_MEM_EXECUTE) != 0;
        BOOL b_Readable   = (pk_Section->Characteristics & IMAGE_SCN_MEM_READ)    != 0;
        BOOL b_Writeable  = (pk_Section->Characteristics & IMAGE_SCN_MEM_WRITE)   != 0;
        if (pk_Section->Characteristics & IMAGE_SCN_CNT_CODE)
        { 
            b_Executable = TRUE; 
            b_Readable   = TRUE; 
        }

        DWORD u32_Protect = ProtectionFlags[b_Executable][b_Readable][b_Writeable];
        if (pk_Section->Characteristics & IMAGE_SCN_MEM_NOT_CACHED) 
            u32_Protect |= PAGE_NOCACHE;

        BYTE* pu8_Address = pk_ModData->codeBase + pk_Section->VirtualAddress;

        DWORD u32_OldProtect;
        if (!gf_VirtualProtect(pu8_Address, u32_Size, u32_Protect, &u32_OldProtect))
            return GetLastError();
    }
    return 0;
}

The following clumsy functions and structures are not needed anymore:

delete: GetRealSectionSize()
delete: FinalizeSection()   // do not confuse with FinalizeSections()
delete: SECTIONFINALIZEDATA

As you see my functions are far shorter than the original code.
And they work without crash!

Also have a look at my further postings here:

Create Activation Context Implementation (important)
#100

Remove unneccessay functions for searching resources:
#53 (comment)

and more...

By the way: The code from Joachim Bauch is not cleanly written.
You find a much cleaner code to load a DLL to memory on Codeproject:
https://www.codeproject.com/Tips/430684/Loading-Win-DLLs-manually-without-LoadLibrary
But it is lacking support for 64 bit Safe SEH and the Activation Context is not implemented in neither project.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions