New issue
Advanced search Search tips

Issue 1543 link

Starred by 4 users

Issue metadata

Status: Fixed
Closed: Apr 2018

Show other hotlists

Hotlists containing this issue:

Sign in to add a comment

mpengine contains unrar code forked from unrar prior to 5.0, introduces new bug while fixing others

Project Member Reported by, Mar 1 2018

Issue description

Windows Defender inspects a variety of different archive formats, among others RAR.

Inspection of mpengine.dll revealed that the code responsible for processing RAR archives appears to be a forked and modified version of the original unrar code; given that it still processes the VMSF_UPCASE filter (which was removed in unrar 5.0), it seems that the code is derived from a version of unrar older or equal than 4.2.4.

Interestingly, the issue discovered in CVE-2012-6706 (Sophos VMSF_DELTA, and in 2017 unrar) and other signedness issues in the RarVM::ExecuteStandardFilter function were fixed long ago (apparently without a report to upstream, most likely by simply turning the relevant variables from "signed" to "unsigned").

It appears that this blanket conversion from signed to unsigned ended up introducing a new vulnerability, though:

From unrar 4.2.4 rarvm.cpp:

    case VMSF_RGB:
        int DataSize=R[4],Width=R[0]-3,PosR=R[1];
        byte *SrcData=Mem,*DestData=SrcData+DataSize;
        const int Channels=3;
        if ((uint)DataSize>=VM_GLOBALMEMADDR/2 || PosR<0)
        for (int CurChannel=0;CurChannel<Channels;CurChannel++)

The code clearly ensures that PosR is positive from here on.

This check is no longer present in the binary version of the same code in mpengine, most likely since most signed comparisons in this function have been turned unsigned. 

This causes a vulnerability later in the same function (RarVM::ExecuteStandardFilter)

Decompile of the mpengine code snippet:

if ( PosR + 2 < DataSize ) {
  v50 = (_BYTE *)(v39 + PosR);
  do {
    v51 = v50[1];
    *v50 += v51;
    v50 += 3;
    *(v50 - 1) += v51;
  } while ( (unsigned int)&v50[2 - v39] < DataSize );

Original unrar code:
  for (int I=PosR,Border=DataSize-2;I<Border;I+=3)
    byte G=DestData[I+1];

An attacker that can set PosR to be -2, and DataSize to 1, will bypass the (PosR + 2 < DataSize) check. v50 above will then point to one byte *before* the allocated buffer (v50 respective DestData points into a buffer at index DataSize -- so adding -2 to index 1 will index to -1. The byte from the start of this array will be added into the byte preceding the array.

A minimal sample RAR file that exhibits these traits & causes mpengine to corrupt memory and crash is attached.

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.

438 bytes Download
Project Member

Comment 1 by, Mar 2 2018

Description: Show this description
Project Member

Comment 2 by, Apr 4 2018

Project Member

Comment 3 by, Apr 4 2018

Labels: -Restrict-View-Commit
Status: Fixed (was: New)

Sign in to add a comment