New issue
Advanced search Search tips
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 4
Cc:


Show other hotlists

Hotlists containing this issue:
Hotlist-1


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 thomasdu...@google.com, Mar 1

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;
        SET_VALUE(false,&Mem[VM_GLOBALMEMADDR+0x20],DataSize);
        if ((uint)DataSize>=VM_GLOBALMEMADDR/2 || PosR<0)
          break;
        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];
    DestData[I]+=G;
    DestData[I+2]+=G;
  }

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.

 
compress_me_output3.rar
438 bytes Download
Project Member

Comment 1 by thomasdu...@google.com, Mar 2

Description: Show this description
Project Member

Comment 3 by thomasdu...@google.com, Apr 4

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

Sign in to add a comment