[CVE-2019-0640] Bug report for Edge/Chakra: Missing marshalling for Promise result

    I also added mitigations for bad things that can happen when calling into a closed script context.
    1. We delete xdata before unregistering it, which can lead to UAF if we call address of a closed function. Windows Exception code unconditionally jumps to handler address (i.e. without CFG check), so this can bypass CFG. I changed to delete after unregistering.
    2. We zero code pages when we close script context, which could be exploitable on x86. I changed to fill with debugbreak.

Author: MikeHolman

lib/Backend/CodeGenWorkItem.cpp

@@ -210,19 +210,7 @@ void CodeGenWorkItem::OnWorkItemProcessFail(NativeCodeGenerator* codeGen)
 #if PDATA_ENABLED & defined(_WIN32)
         if (this->entryPointInfo)
         {
-            XDataAllocation * xdataAllocation = this->entryPointInfo->GetNativeEntryPointData()->GetXDataInfo();
-            if (xdataAllocation)
-            {
-                void* functionTable = xdataAllocation->functionTable;
-                if (functionTable)
-                {
-                    if (!DelayDeletingFunctionTable::AddEntry(functionTable))
-                    {
-                        PHASE_PRINT_TESTTRACE1(Js::XDataPhase, _u("[%d]OnWorkItemProcessFail: Failed to add to slist, table: %llx\n"), GetCurrentThreadId(), functionTable);
-                        DelayDeletingFunctionTable::DeleteFunctionTable(functionTable);
-                    }
-                }
-            }
+            this->entryPointInfo->GetNativeEntryPointData()->CleanupXDataInfo();
         }
 #endif
         codeGen->FreeNativeCodeGenAllocation(this->allocation->allocation->address);

lib/Backend/NativeEntryPointData.cpp

@@ -321,20 +321,14 @@ NativeEntryPointData::CleanupXDataInfo()
 {
     if (this->xdataInfo != nullptr)
     {
+        XDataAllocator::Unregister(this->xdataInfo);
 #ifdef _WIN32
         if (this->xdataInfo->functionTable
-            && !DelayDeletingFunctionTable::AddEntry(this->xdataInfo->functionTable))
+            && !DelayDeletingFunctionTable::AddEntry(this->xdataInfo))
         {
-            DelayDeletingFunctionTable::DeleteFunctionTable(this->xdataInfo->functionTable);
+            DelayDeletingFunctionTable::DeleteFunctionTable(this->xdataInfo);
         }
 #endif
-        XDataAllocator::Unregister(this->xdataInfo);
-#if defined(_M_ARM)
-        if (JITManager::GetJITManager()->IsOOPJITEnabled())
-#endif
-        {
-            HeapDelete(this->xdataInfo);
-        }
         this->xdataInfo = nullptr;
     }
 }

lib/Backend/NativeEntryPointData.h

@@ -71,17 +71,15 @@ class NativeEntryPointData
 
 #if PDATA_ENABLED
     XDataAllocation* GetXDataInfo() { return this->xdataInfo; }
-    void SetXDataInfo(XDataAllocation* xdataInfo) { this->xdataInfo = xdataInfo; }   
+    void CleanupXDataInfo();
+    void SetXDataInfo(XDataAllocation* xdataInfo) { this->xdataInfo = xdataInfo; }
 #endif
 private:
     void RegisterEquivalentTypeCaches(Js::ScriptContext * scriptContext, Js::EntryPointInfo * entryPointInfo);
     void UnregisterEquivalentTypeCaches(Js::ScriptContext * scriptContext);
     void FreePropertyGuards();
 
     void FreeNativeCode(Js::ScriptContext * scriptContext, bool isShutdown);
-#if PDATA_ENABLED
-    void CleanupXDataInfo();
-#endif
 
     FieldNoBarrier(Js::JavascriptMethod) nativeAddress;
     FieldNoBarrier(Js::JavascriptMethod) thunkAddress;

lib/Backend/PDataManager.cpp

@@ -51,8 +51,7 @@ void PDataManager::UnregisterPdata(RUNTIME_FUNCTION* pdata)
 {
     if (AutoSystemInfo::Data.IsWin8OrLater())
     {
-        // TODO: need to move to background?
-        DelayDeletingFunctionTable::DeleteFunctionTable(pdata);
+        NtdllLibrary::Instance->DeleteGrowableFunctionTable(pdata);
     }
     else
     {

lib/Common/Memory/Chakra.Common.Memory.vcxproj

@@ -26,6 +26,8 @@
     <ClCompile>
       <AdditionalIncludeDirectories>
         $(MSBuildThisFileDirectory);
+        $(MSBuildThisFileDirectory)..\..\JITClient;
+        $(ChakraJITIDLIntDir);
         $(MSBuildThisFileDirectory)..;
         %(AdditionalIncludeDirectories)
       </AdditionalIncludeDirectories>

lib/Common/Memory/DelayDeletingFunctionTable.cpp

@@ -7,6 +7,7 @@
 #include "Memory/XDataAllocator.h"
 #if PDATA_ENABLED && defined(_WIN32)
 #include "Core/DelayLoadLibrary.h"
+#include "JITClient.h"
 #include <malloc.h>
 
 CriticalSection DelayDeletingFunctionTable::cs;
@@ -34,14 +35,14 @@ DelayDeletingFunctionTable::~DelayDeletingFunctionTable()
 }
 
 
-bool DelayDeletingFunctionTable::AddEntry(FunctionTableHandle ft)
+bool DelayDeletingFunctionTable::AddEntry(XDataAllocation* xdata)
 {
     if (Head)
     {
         FunctionTableNode* node = (FunctionTableNode*)_aligned_malloc(sizeof(FunctionTableNode), MEMORY_ALLOCATION_ALIGNMENT);
         if (node)
         {
-            node->functionTable = ft;
+            node->xdata = xdata;
             InterlockedPushEntrySList(Head, &(node->itemEntry));
             return true;
         }
@@ -61,7 +62,7 @@ void DelayDeletingFunctionTable::Clear()
         while (entry)
         {
             FunctionTableNode* list = (FunctionTableNode*)entry;
-            DeleteFunctionTable(list->functionTable);
+            DeleteFunctionTable(list->xdata);
             _aligned_free(entry);
             entry = InterlockedPopEntrySList(Head);
         }
@@ -73,9 +74,16 @@ bool DelayDeletingFunctionTable::IsEmpty()
     return QueryDepthSList(Head) == 0;
 }
 
-void DelayDeletingFunctionTable::DeleteFunctionTable(void* functionTable)
+void DelayDeletingFunctionTable::DeleteFunctionTable(XDataAllocation* xdata)
 {
-    NtdllLibrary::Instance->DeleteGrowableFunctionTable(functionTable);
+    NtdllLibrary::Instance->DeleteGrowableFunctionTable(xdata->functionTable);
+
+#if defined(_M_ARM)
+    if (JITManager::GetJITManager()->IsOOPJITEnabled())
+#endif
+    {
+        HeapDelete(xdata);
+    }
 }
 
 #endif

lib/Common/Memory/SectionAllocWrapper.cpp

@@ -6,7 +6,10 @@
 
 #if _WIN32
 #if ENABLE_OOP_NATIVE_CODEGEN
-#include "../Core/DelayLoadLibrary.h"
+#include "Core/DelayLoadLibrary.h"
+
+#include "XDataAllocator.h"
+#include "CustomHeap.h"
 
 #ifdef NTDDI_WIN10_RS2
 #if (NTDDI_VERSION >= NTDDI_WIN10_RS2)
@@ -603,6 +606,19 @@ SectionAllocWrapper::AllocPages(LPVOID requestAddress, size_t pageCount, DWORD a
         {
             return nullptr;
         }
+
+        // pages could be filled with debugbreak
+        // zero one page at a time to minimize working set impact while zeroing
+        for (size_t i = 0; i < dwSize / AutoSystemInfo::PageSize; ++i)
+        {
+            LPVOID localAddr = AllocLocal((char*)requestAddress + i * AutoSystemInfo::PageSize, AutoSystemInfo::PageSize);
+            if (localAddr == nullptr)
+            {
+                return nullptr;
+            }
+            ZeroMemory(localAddr, AutoSystemInfo::PageSize);
+            FreeLocal(localAddr);
+        }
         address = requestAddress;
     }
 
@@ -677,10 +693,10 @@ BOOL SectionAllocWrapper::Free(LPVOID lpAddress, size_t dwSize, DWORD dwFreeType
             {
                 return FALSE;
             }
-            ZeroMemory(localAddr, AutoSystemInfo::PageSize);
+
+            CustomHeap::FillDebugBreak((BYTE*)localAddr, AutoSystemInfo::PageSize);
             FreeLocal(localAddr);
         }
-        UnlockMemory(this->process, lpAddress, dwSize);
     }
 
     return TRUE;
@@ -928,6 +944,19 @@ LPVOID PreReservedSectionAllocWrapper::AllocPages(LPVOID lpAddress, DECLSPEC_GUA
 
             addressToReserve = (char*)lpAddress;
             freeSegmentsBVIndex = (uint)((addressToReserve - (char*)this->preReservedStartAddress) / AutoSystemInfo::Data.GetAllocationGranularityPageSize());
+
+            // pages could be filled with debugbreak
+            // zero one page at a time to minimize working set impact while zeroing
+            for (size_t i = 0; i < dwSize / AutoSystemInfo::PageSize; ++i)
+            {
+                LPVOID localAddr = AllocLocal((char*)lpAddress + i * AutoSystemInfo::PageSize, AutoSystemInfo::PageSize);
+                if (localAddr == nullptr)
+                {
+                    return nullptr;
+                }
+                ZeroMemory(localAddr, AutoSystemInfo::PageSize);
+                FreeLocal(localAddr);
+            }
 #if DBG
             uint numOfSegments = (uint)ceil((double)dwSize / (double)AutoSystemInfo::Data.GetAllocationGranularityPageSize());
             Assert(numOfSegments != 0);
@@ -978,12 +1007,17 @@ PreReservedSectionAllocWrapper::Free(LPVOID lpAddress, size_t dwSize, DWORD dwFr
         {
             return FALSE;
         }
-        ZeroMemory(localAddr, AutoSystemInfo::PageSize);
+        if ((dwFreeType & MEM_RELEASE) == MEM_RELEASE)
+        {
+            ZeroMemory(localAddr, AutoSystemInfo::PageSize);
+        }
+        else
+        {
+            CustomHeap::FillDebugBreak((BYTE*)localAddr, AutoSystemInfo::PageSize);
+        }
         FreeLocal(localAddr);
     }
 
-    UnlockMemory(this->process, lpAddress, dwSize);
-
     size_t requestedNumOfSegments = dwSize / AutoSystemInfo::Data.GetAllocationGranularityPageSize();
     Assert(requestedNumOfSegments <= MAXUINT32);
 
@@ -1000,6 +1034,8 @@ PreReservedSectionAllocWrapper::Free(LPVOID lpAddress, size_t dwSize, DWORD dwFr
         AssertMsg(freeSegmentsBVIndex < PreReservedAllocationSegmentCount, "Invalid Index ?");
         freeSegments.SetRange(freeSegmentsBVIndex, static_cast<uint>(requestedNumOfSegments));
         PreReservedHeapTrace(_u("MEM_RELEASE: Address: 0x%p of size: 0x%x * 0x%x bytes\n"), lpAddress, requestedNumOfSegments, AutoSystemInfo::Data.GetAllocationGranularityPageSize());
+
+        UnlockMemory(this->process, lpAddress, dwSize);
     }
 
     return TRUE;

lib/Common/Memory/XDataAllocator.h

@@ -16,7 +16,7 @@
 struct FunctionTableNode
 {
     SLIST_ENTRY itemEntry;
-    FunctionTableHandle functionTable;
+    XDataAllocation* xdata;
 };
 
 struct DelayDeletingFunctionTable
@@ -28,9 +28,9 @@ struct DelayDeletingFunctionTable
     DelayDeletingFunctionTable();
     ~DelayDeletingFunctionTable();
 
-    static bool AddEntry(FunctionTableHandle ft);
+    static bool AddEntry(XDataAllocation* xdata);
     static void Clear();
     static bool IsEmpty();
-    static void DeleteFunctionTable(void* functionTable);
+    static void DeleteFunctionTable(XDataAllocation* xdata);
 };
 #endif

lib/Runtime/Library/JavascriptPromise.cpp

@@ -1144,15 +1144,17 @@ namespace Js
             sourcePromise->reactions->Prepend(pair);
             break;
         case PromiseStatusCode_HasResolution:
-            EnqueuePromiseReactionTask(resolveReaction, sourcePromise->result, scriptContext);
+            EnqueuePromiseReactionTask(resolveReaction, CrossSite::MarshalVar(scriptContext, sourcePromise->result), scriptContext);
             break;
         case PromiseStatusCode_HasRejection:
+        {
             if (!sourcePromise->GetIsHandled())
             {
-                scriptContext->GetLibrary()->CallNativeHostPromiseRejectionTracker(sourcePromise, sourcePromise->result, true);
+                scriptContext->GetLibrary()->CallNativeHostPromiseRejectionTracker(sourcePromise, CrossSite::MarshalVar(scriptContext, sourcePromise->result), true);
             }
-            EnqueuePromiseReactionTask(rejectReaction, sourcePromise->result, scriptContext);
+            EnqueuePromiseReactionTask(rejectReaction, CrossSite::MarshalVar(scriptContext, sourcePromise->result), scriptContext);
             break;
+        }
         default:
             AssertMsg(false, "Promise status is in an invalid state");
             break;