Import table with no relaction blocks are seen as "status": -1

[terrybr]

Hello hasherezade,

I believe there's a bug in libpeconv where a malware that doesn't have any relocation blocks returns the status: -1 in pe-sieve (with /jlvl 2) and thus is not processed.

I found that the specific malware I tested with doesn't have any relocation blocks (using your great PE-Bear tool):

It's happening in this function: peconv::process_relocation_table which I see you already addressed the issue when relocation blocks are null in this fix #22 .

A friend of mine (Ron, who found the memory leak in pe-sieve) came up with a potential fix which I tested and works. Basically the function has to return true in the case there's nothing to relocate. We added bool NoBlocks = true which is set to false if there are valid blocks and is used for the return value condition.

bool peconv::process_relocation_table(IN PVOID modulePtr, IN SIZE_T moduleSize, IN RelocBlockCallback *callback)
{
    IMAGE_DATA_DIRECTORY* relocDir = peconv::get_directory_entry((const BYTE*)modulePtr, IMAGE_DIRECTORY_ENTRY_BASERELOC);
    if (relocDir == NULL) {
#ifdef _DEBUG
        std::cout << "[!] WARNING: no relocation table found!\n";
#endif
        return false;
    }
    if (!validate_ptr(modulePtr, moduleSize, relocDir, sizeof(IMAGE_DATA_DIRECTORY))) {
        std::cerr << "[!] Invalid relocDir pointer\n";
        return false;
    }
    DWORD maxSize = relocDir->Size;
    DWORD relocAddr = relocDir->VirtualAddress;
    bool is64b = is64bit((BYTE*)modulePtr);

    IMAGE_BASE_RELOCATION* reloc = NULL;

    DWORD parsedSize = 0;
    DWORD validBlocks = 0;
    bool NoBlocks = true;

    while (parsedSize < maxSize) {
        reloc = (IMAGE_BASE_RELOCATION*)(relocAddr + parsedSize + (ULONG_PTR)modulePtr);
        if (!validate_ptr(modulePtr, moduleSize, reloc, sizeof(IMAGE_BASE_RELOCATION))) {
#ifdef _DEBUG
            std::cerr << "[-] Invalid address of relocations\n";
#endif
            return false;
        }
        if (reloc->SizeOfBlock == 0) {
            break;
        }
        size_t entriesNum = (reloc->SizeOfBlock - 2 * sizeof(DWORD)) / sizeof(WORD);
        DWORD page = reloc->VirtualAddress;

        BASE_RELOCATION_ENTRY* block = (BASE_RELOCATION_ENTRY*)((ULONG_PTR)reloc + sizeof(DWORD) + sizeof(DWORD));
        if (!validate_ptr(modulePtr, moduleSize, block, sizeof(BASE_RELOCATION_ENTRY))) {
            std::cerr << "[-] Invalid address of relocations block\n";
            return false;
        }
        if (!is_empty_reloc_block(block, entriesNum, page, modulePtr, moduleSize)) {
            if (process_reloc_block(block, entriesNum, page, modulePtr, moduleSize, is64b, callback)) {
                validBlocks++;
                NoBlocks = false;
            }
            else {
                // the block was malformed
                return false;
            }
        }
        parsedSize += reloc->SizeOfBlock;
    }
    return (NoBlocks || validBlocks != 0);

Thank you, and keep up the fantastic work!

Terry

[hasherezade]

Hi! Thank you! I really appreciate your contributions, guys! Your fix looks fine, would you like to send me a pull request?

[terrybr]

No problem! Thank you again for creating those tools and for making them available. Cheers!

[hasherezade]

@terrybr - I merged your pull request, and now I am checking it again, and something in the logic of it is not ok... Well, basically, all what NoBlocks does, is reverting what was achieved with validBlocks... Now, the return condition:

    return (noBlocks || validBlocks != 0);

is an equivalent of:

    return (validBlocks == 0 || validBlocks != 0);

Please take a closer look, and you will know what I mean... So, the whole thing is now an equivalent of this code:

    DWORD parsedSize = 0;
    while (parsedSize < maxSize) {
        reloc = (IMAGE_BASE_RELOCATION*)(relocAddr + parsedSize + (ULONG_PTR)modulePtr);
        if (!validate_ptr(modulePtr, moduleSize, reloc, sizeof(IMAGE_BASE_RELOCATION))) {
#ifdef _DEBUG
            std::cerr << "[-] Invalid address of relocations\n";
#endif
            return false;
        }
        if (reloc->SizeOfBlock == 0) {
            break;
        }
        size_t entriesNum = (reloc->SizeOfBlock - 2 * sizeof(DWORD)) / sizeof(WORD);
        DWORD page = reloc->VirtualAddress;

        BASE_RELOCATION_ENTRY* block = (BASE_RELOCATION_ENTRY*)((ULONG_PTR)reloc + sizeof(DWORD) + sizeof(DWORD));
        if (!validate_ptr(modulePtr, moduleSize, block, sizeof(BASE_RELOCATION_ENTRY))) {
            std::cerr << "[-] Invalid address of relocations block\n";
            return false;
        }
        if (!is_empty_reloc_block(block, entriesNum, page, modulePtr, moduleSize)) {
            if (!process_reloc_block(block, entriesNum, page, modulePtr, moduleSize, is64b, callback)) {
                // the block was malformed
                return false;
            }
        }
        parsedSize += reloc->SizeOfBlock;
    }
    return true;

I will still be double-checking if skipping of the validBlocks check does not have any side effects... Can you share your sample that caused that problem?

[terrybr]

@hasherezade No problem at all. We were anticipating that the fix might cause other issues.

I'm attaching the "malware" in question. Hopefully that will help you find the root cause.

malware.zip

[hasherezade]

@terrybr - thank you! After more tests I think it should be fine - I am anyways breaking the processing and returning false if any of the fields is malformed, i.e. https://github.com/hasherezade/libpeconv/blob/1e921085a4c32c8e50310489374955dd262886da/libpeconv/src/relocate.cpp#L141-L144

So, in the situation where the table does not have any valid blocks, but also no malformations, I can still accept it as valid. I just cleaned up your changes a bit, and I think it can stay. (But of course I will be testing more before the new official PE-sieve release).

[hasherezade]

The changes are now included in PE-sieve: https://github.com/hasherezade/pe-sieve/commit/9671e090003aae3b640a8b3d81a163df6eb64542 and in HollowsHunter: https://github.com/hasherezade/hollows_hunter/commit/24830daab2e2ad0a64a7dc50909d35936121fd62

[terrybr]

Thank you @hasherezade!

[hasherezade]

Thank you too!