A few days after having published the post about the Bitdefender stack buffer overflow via 7z PPMD, I discovered a new bug in Bitdefender’s product.
While this is a 7z bug, too, it has nothing to do with the previous bug or with the PPMD codec.
Instead, it concerns dynamic memory management.
In contrast to the previous post, which described an arbitrary free vulnerability in F-Secure’s anti-virus product,
this post presents the first heap buffer overflow of this blog series.
Introduction
For the write-up on the 7z PPMD bug, I read a lot of the original 7-Zip source code and discovered a few new things that looked promising to investigate in anti-virus products.
Therefore, I took another stab at analyzing Bitdefender’s 7z module.
I previously wrote about relaxed file processing.
The Bitdefender 7z PPMD stack buffer overflow was a good example of relaxed file processing by removing a check (that is, removing code).
This bug demonstrates another fundamental difficulty that arises when incorporating new code into an existing code base.
In particular, a minimal set of changes to the new code is often inevitable.
Mostly, this affects memory allocation and code that is concerned with file access, especially if a totally different file abstraction is used.
The presented bug is an example of the former type of difficulty.
More specifically, an incorrect use of a memory allocation function that extends the 7-Zip source code in Bitdefender’s 7z module causes a heap buffer overflow.
Getting Into the Details
When Bitdefender’s 7z module discovers an EncodedHeader in a 7z archive, it tries to decompress it with the LZMA decoder.
Their code seems to be based on 7-Zip, but they made a few changes.
Loosely speaking, the extraction of a 7z EncodedHeader is implemented as follows:
- Read the
unpackSize from the 7z EncodedHeader.
- Allocate
unpackSize bytes.
- Use the C API of the LZMA decoder that comes with 7-Zip and let it decompress the stream.
The following snippet shows how the allocation function is called:
1DD02A845FA lea rcx, [rdi+128h] //<-------- result
1DD02A84601 mov rbx, [rdi+168h]
1DD02A84608 mov [rsp+128h], rsi
1DD02A84610 mov rsi, [rax+10h]
1DD02A84614 mov [rsp+0E0h], r15
1DD02A8461C mov edx, [rsi] //<-------- size
1DD02A8461E call SZ_AllocBuffer
Recall the x64 calling convention. In particular, the first two integer arguments (from left to right) are passed via rcx and rdx.
SZ_AllocBuffer is a function within the Bitdefender 7z module. It has two arguments:
- The first argument
result is a pointer to which the result (a pointer to the allocated buffer in case of success or NULL in case of a failure) is written.
- The second argument
size is the allocation size.
Let us look at the functions’s implementation.
260ED3025D0 SZ_AllocBuffer proc near
260ED3025D0
260ED3025D0 mov [rsp+8], rbx
260ED3025D5 push rdi
260ED3025D6 sub rsp, 20h
260ED3025DA mov rbx, rcx
260ED3025DD mov edi, edx //<-------- edi holds size
260ED3025DF mov rcx, [rcx]
260ED3025E2 test rcx, rcx
260ED3025E5 jz short loc_260ED3025EC
260ED3025E7 call near ptr irrelevant_function
260ED3025EC
260ED3025EC loc_260ED3025EC:
260ED3025EC cmp edi, 0FFFFFFFFh //<------- {*}
260ED3025EF jbe short loc_260ED302606
260ED3025F1 xor ecx, ecx
260ED3025F3 mov [rbx], rcx
260ED3025F6 mov eax, ecx
260ED3025F8 mov [rbx+8], ecx
260ED3025FB mov rbx, [rsp+30h]
260ED302600 add rsp, 20h
260ED302604 pop rdi
260ED302605 retn
260ED302606 ; ------------------------------------
260ED302606
260ED302606 loc_260ED302606:
260ED302606 mov rcx, rdi //<------ set size argument for mymalloc
260ED302609 call mymalloc
//[rest of the function omitted]
Note that mymalloc is just a wrapper function that eventually calls malloc and returns the result.
Apparently, the programmer expected the size argument of SZ_AllocBuffer to be of a type with size greater than 32 bits.
Obviously, it is only a 32-bit value.
It is funny to see that the compiler failed to optimize away the comparison at {*},
given that its result is only used for an unsigned comparison jbe.
If you have any hints on why this might happen, I’d be very interested to hear them.
After SZ_AllocBuffer returns, the function LzmaDecode is called:
LzmaDecode(Byte *dest, SizeT *destLen, const Byte *src, SizeT *srcLen, /* further arguments omitted */)
Note that dest is the buffer allocated with SZ_AllocBuffer and destLen is supposed to be a pointer to the buffer’s size.
In the reference implementation, SizeT is defined as size_t.
Interestingly, Bitdefender’s 7z module uses a 64-bit type for SizeT in both the 32-bit and the 64-bit version, making both versions vulnerable to this bug.
I suspect that this is the result of an effort to create identical behavior for the 32-bit and 64-bit versions of the engine.
The LZMA decoder extracts the given src stream and writes (up to) *destLen bytes to the dest buffer, where *destLen is the 64-bit unpackSize from the 7z EncodedHeader. This results in a neat heap buffer overflow.
Triggering the Bug
To trigger the bug, we create a 7z LZMA stream containing the data we want to write on the heap.
Then, we construct a 7z EncodedHeader with a Folder that has an unpackSize of (1<<32) + 1.
This should make the function SZ_AllocBuffer allocate a buffer of 1 byte.
That sounds nice, but does this actually work?
0:000> g
!Heap block at 1F091472D40 modified at 1F091472D51 past requested size of 1
(2f8.14ec): Break instruction exception - code 80000003 (first chance)
ntdll!RtlpNtMakeTemporaryKey+0x435e:
00007ff9`d849c4ce cc int 3
0:000> db 1F091472D51
000001f0`91472d51 59 45 53 2c 20 54 48 49-53 20 57 4f 52 4b 53 ab YES, THIS WORKS.
Attacker Control and Exploitation
The attacker can write completely arbitrary data to the heap without any restriction.
A file system minifilter is used to scan all files that touch the disk,
making this vulnerability easily exploitable remotely,
for example by sending an e-mail with a crafted file as attachment to the victim.
Moreover, the engine runs unsandboxed and as NT Authority\SYSTEM.
Hence, this bug is highly critical.
However, since ASLR and DEP are in place,
successful exploitation for remote code execution might require another bug (e.g. an information leak) to bypass ASLR.
Note also that Bitdefender’s engine is licensed to many different anti-virus vendors, all of which could be affected by this bug.
The Fix
The patched version of the function SZ_AllocBuffer looks as follows:
1E0CEA52AE0 SZ_AllocBuffer proc near
1E0CEA52AE0
1E0CEA52AE0 mov [rsp+8], rbx
1E0CEA52AE5 mov [rsp+10h], rsi
1E0CEA52AEA push rdi
1E0CEA52AEB sub rsp, 20h
1E0CEA52AEF mov esi, 0FFFFFFFFh
1E0CEA52AF4 mov rdi, rdx //<-----rdi holds the size
1E0CEA52AF7 mov rbx, rcx
1E0CEA52AFA cmp rdx, rsi //<------------{1}
1E0CEA52AFD jbe short loc_1E0CEA52B11
1E0CEA52AFF xor eax, eax
1E0CEA52B01 mov rbx, [rsp+30h]
1E0CEA52B06 mov rsi, [rsp+38h]
1E0CEA52B0B add rsp, 20h
1E0CEA52B0F pop rdi
1E0CEA52B10 retn
1E0CEA52B11 ; -----------------------------------
1E0CEA52B11
1E0CEA52B11 loc_1E0CEA52B11:
1E0CEA52B11 mov rcx, [rcx]
1E0CEA52B14 test rcx, rcx
1E0CEA52B17 jz short loc_1E0CEA52B1E
1E0CEA52B19 call near ptr irrelevant_function
1E0CEA52B1E
1E0CEA52B1E loc_1E0CEA52B1E:
1E0CEA52B1E cmp edi, esi //<------------{2}
1E0CEA52B20 jbe short loc_1E0CEA52B29
1E0CEA52B22 xor ecx, ecx
1E0CEA52B24 mov [rbx], rcx
1E0CEA52B27 jmp short loc_1E0CEA52B3B
1E0CEA52B29 ; -----------------------------------
1E0CEA52B29
1E0CEA52B29 loc_1E0CEA52B29:
1E0CEA52B29 mov ecx, edi
1E0CEA52B2B call near ptr mymalloc
//[rest of the function omitted]
Most importantly, we see that the function’s second argument size has been changed to a 64-bit type.
Note that at {1}, a check ensures that the passed size is not greater than 0xFFFFFFFF.
At {2}, the value of rdi is guaranteed to be at most 0xFFFFFFFF, hence it suffices to use the 32-bit register edi.
However, just as in the original version (see above), it is useless to compare this 32-bit value once more to 0xFFFFFFFF
and it is a mystery to me why the compiler does not optimize this away.
Using a full 64-bit type for the second argument size resolves the described bug.
Conclusion
In a nutshell, the discovered bug is a 64-bit value size being passed to the allocation function SZ_AllocBuffer which looks roughly like this:
void* SZ_AllocBuffer(void *resultptr, uint32_t size);
Assuming that the size is not explicitly casted, the compiler should throw a warning of the following kind:
warning C4244: 'argument': conversion from 'uint64_t' to 'uint32_t', possible loss of data
Note that in Microsoft’s MSVC compiler, this is a Level2 warning (Level1 being the lowest and Level4 being the highest level).
Hence, this bug most likely could have been avoided simply by taking compiler warnings seriously.
For a critical codebase such as the engine of an anti-virus product,
it would be adequate to treat warnings as errors, at least up to a warning level of 2 or 3.
Nevertheless, the general type of bug shows that even if only few lines of additional code are necessary
to incorporate external code (such as the 7-Zip code) into a code base, those very lines can be particularly prone to error.
Do you have any comments, feedback, doubts, or complaints?
I would love to hear them. You can find my e-mail address on the about page.
Alternatively, you are invited to join the discussion on HackerNews or on /r/netsec.
Timeline of Disclosure
- 2017-07-24 - Discovery
- 2017-07-24 - Report
- 2017-07-24 - “Thank you for your report, we will investigate and come back with an answer."
- 2017-08-22 - “confirm that this vulnerability has been patched” and “will have an internal discussion about the reward”
- 2017-09-12 - Bug bounty paid
Thanks & Acknowledgements
I want to thank Bitdefender and especially Marius for their response as well as for fixing the bug.