Microsoft Windows Defender - 'mpengine.dll' Memory Corruption

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.


Proof of Concept:
https://gitlab.com/exploit-database/exploitdb-bin-sploits/-/raw/main/bin-sploits/44402.zip