Integer overflow in p_lx_elf.cpp for i386 UPX (devel branch)

[xiaoyinl]

Issue #128 was fixed by validating e_phoff, e_shoff, etc. However, it doesn't check for integer overflow correctly: casting unsigned int
to unsigned long works for 64-bit executable but unsigned long is 32-bit for 32-bit executable. So 32-bit UPX is still vulnerable to integer overflow.

A PoC is attached. Compile UPX as 32-bit, and then use it to pack the PoC. You can see UPX crashes immediately, due to reading invalid address.

My PoC is based on the POC1 file posted in Issue #128 by @hongphipham95. I changed e_type to 0x03, e_shoff to 0xFFFFE000, and e_shnum to 0x00FF.
This change makes (unsigned long)e_shoff + e_shnum * sizeof(Elf32_Shdr) overflow (at src/p_lx_elf.cpp#L262), and thus pass the checking.

Simply changing unsigned long to unsigned long long fixes this bug.

Version:

~/git/upx/src$ ./upx.out --version
upx 3.95-git-d698eb69e278
UCL data compression library 1.03
zlib data compression library 1.2.8
LZMA SDK version 4.43

ASAN:

~/git/upx/src$ ./upx.out ../poc/intoverflow2
                       Ultimate Packer for eXecutables
                          Copyright (C) 1996 - 2017
UPX git-d698eb  Markus Oberhumer, Laszlo Molnar & John Reiser   May 13th 2017

        File size         Ratio      Format      Name
   --------------------   ------   -----------   -----------
ASAN:SIGSEGV
=================================================================
==3212==ERROR: AddressSanitizer: SEGV on unknown address 0xb5da7804 (pc 0x0807e547 bp 0xbf973b58 sp 0xbf973b58 T0)
    #0 0x807e546 in get_le32(void const*) (/home/<username>/git/upx/src/upx.out+0x807e546)
    #1 0x8753d30 in N_BELE_RTP::LEPolicy::get32(void const*) const /home/<username>/git/upx/src/bele_policy.h:192
    #2 0x8290def in Packer::get_te32(void const*) const /home/<username>/git/upx/src/packer.h:296
    #3 0x81a2ff7 in PackLinuxElf32::elf_find_section_type(unsigned int) const /home/<username>/git/upx/src/p_lx_elf.cpp:1635
    #4 0x81535e8 in PackLinuxElf32::PackLinuxElf32help1(InputFile*) /home/<username>/git/upx/src/p_lx_elf.cpp:281
    #5 0x82949ca in PackLinuxElf32Le::PackLinuxElf32Le(InputFile*) (/home/<username>/git/upx/src/upx.out+0x82949ca)
    #6 0x82612f2 in PackLinuxElf32x86::PackLinuxElf32x86(InputFile*) /home/<username>/git/upx/src/p_lx_elf.cpp:4085
    #7 0x8261db7 in PackBSDElf32x86::PackBSDElf32x86(InputFile*) /home/<username>/git/upx/src/p_lx_elf.cpp:4102
    #8 0x8262627 in PackFreeBSDElf32x86::PackFreeBSDElf32x86(InputFile*) /home/<username>/git/upx/src/p_lx_elf.cpp:4113
    #9 0x864cc99 in PackMaster::visitAllPackers(Packer* (*)(Packer*, void*), InputFile*, options_t const*, void*) /home/<username>/git/upx/src/packmast.cpp:190
    #10 0x8657e73 in PackMaster::getPacker(InputFile*) /home/<username>/git/upx/src/packmast.cpp:240
    #11 0x8658694 in PackMaster::pack(OutputFile*) /home/<username>/git/upx/src/packmast.cpp:260
    #12 0x875595c in do_one_file(char const*, char*) /home/<username>/git/upx/src/work.cpp:158
    #13 0x8757ab6 in do_files(int, int, char**) /home/<username>/git/upx/src/work.cpp:271
    #14 0x80cd1c7 in main /home/<username>/git/upx/src/main.cpp:1535
    #15 0xb71f8636 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x18636)
    #16 0x804a3a0  (/home/<username>/git/upx/src/upx.out+0x804a3a0)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 get_le32(void const*)
==3212==ABORTING

[xiaoyinl]

Unzip it. Then run ./upx.out ./intoverflow2
intoverflow2.zip

[xiaoyinl]

@jreiser Could you please review? Also I requested a CVE for this issue on iwantacve.org.

[jreiser]

I prefer to check directly for CarryOut from unsigned addition as an indicator of wraparound. This avoids using wider precision which can be big and slow. For the unsigned addition C = A + B then CarryOut is (C < A) or also (C < B).

[xiaoyinl]

Yes, yours is better. Feel free to close this PR.

[kurtseifried]

Is this just a crash or is code execution possible?

[xiaoyinl]

@kurtseifried Probably just crash. I was not able to achieve code execution or memory disclosure with this bug.

[jreiser]

I believe that it is just a crash. "Memory disclosure" is not relevant: all of the input to "upx -d", the output from "upx -d", and upx itself contain no secrets. All of the instructions executed by upx (in any mode, including "upx -d") reside in upx itself or in the named shared libraries loaded by the runtime loader at start of execution of upx itself. upx does no dynamic loading, and no just-in-time compilation.

[kurtseifried]

As such this is a security hardening issue and not a security vulnerability so I'm going to REJECT the CVE request for this, thanks!