Explicitly tag fxjs native objects.

Native object callbacks have to distinguish whether the object
they have been given is actually a native object and not some
ordinary JS object.  For method/property calls, this happens
via v8's signature mechanism, but signature checks aren't applied
to method arguments themselves.

Currently, we do this by treating any object with an internal field
count of 2 as being such, but this is fragile, and it has been pointed
out that other objects with two internal fields are present.

Additionally, that the first field points to a structure with a
small zero-based object definition ID doesn't really have enough
entropy to trust that it isn't some other entity.  So add a pointer
to an internal address in the second slot to make this safer.

Note that we'll also get the same release_assert in the majority
of cases as described in the bug. This is great from a security
standpoint, but not great from a functional standpoint, except this
likely only occurs in the wild if they are trying to mess with us.
This just guards the theoretical cases that might pass the
existing release_assert.

BUG=695830

Change-Id: I42db27d6ed1143269a852805e4e4d862a8ab8773
Reviewed-on: https://pdfium-review.googlesource.com/2847
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

1 files changed, 44 insertions, 39 deletions

diff --git a/fxjs/fxjs_v8.cpp b/fxjs/fxjs_v8.cpp
index 0c211a130a..b0e1a1b260 100644
--- a/fxjs/fxjs_v8.cpp
+++ b/fxjs/fxjs_v8.cpp
@@ -20,12 +20,32 @@ static v8::Isolate* g_isolate = nullptr;
 static size_t g_isolate_ref_count = 0;
 static FXJS_ArrayBufferAllocator* g_arrayBufferAllocator = nullptr;
 static v8::Global<v8::ObjectTemplate>* g_DefaultGlobalObjectTemplate = nullptr;
+static wchar_t kPerObjectDataTag[] = L"CFXJS_PerObjectData";
 
 class CFXJS_PerObjectData {
  public:
   explicit CFXJS_PerObjectData(int nObjDefID)
       : m_ObjDefID(nObjDefID), m_pPrivate(nullptr) {}
 
+  static void SetInObject(CFXJS_PerObjectData* pData,
+                          v8::Local<v8::Object> pObj) {
+    if (pObj->InternalFieldCount() == 2) {
+      pObj->SetAlignedPointerInInternalField(0, pData);
+      pObj->SetAlignedPointerInInternalField(
+          1, static_cast<void*>(kPerObjectDataTag));
+    }
+  }
+
+  static CFXJS_PerObjectData* GetFromObject(v8::Local<v8::Object> pObj) {
+    if (pObj.IsEmpty() || pObj->InternalFieldCount() != 2 ||
+        pObj->GetAlignedPointerFromInternalField(1) !=
+            static_cast<void*>(kPerObjectDataTag)) {
+      return nullptr;
+    }
+    return static_cast<CFXJS_PerObjectData*>(
+        pObj->GetAlignedPointerFromInternalField(0));
+  }
+
   const int m_ObjDefID;
   void* m_pPrivate;
 };
@@ -241,13 +261,8 @@ CFXJS_Engine* CFXJS_Engine::CurrentEngineFromIsolate(v8::Isolate* pIsolate) {
 
 // static
 int CFXJS_Engine::GetObjDefnID(v8::Local<v8::Object> pObj) {
-  if (pObj.IsEmpty() || !pObj->InternalFieldCount())
-    return -1;
-  CFXJS_PerObjectData* pPerObjectData = static_cast<CFXJS_PerObjectData*>(
-      pObj->GetAlignedPointerFromInternalField(0));
-  if (!pPerObjectData)
-    return -1;
-  return pPerObjectData->m_ObjDefID;
+  CFXJS_PerObjectData* pData = CFXJS_PerObjectData::GetFromObject(pObj);
+  return pData ? pData->m_ObjDefID : -1;
 }
 
 // static
@@ -257,10 +272,10 @@ void CFXJS_Engine::FreeObjectPrivate(void* pPerObjectData) {
 
 // static
 void CFXJS_Engine::FreeObjectPrivate(v8::Local<v8::Object> pObj) {
-  if (pObj.IsEmpty() || !pObj->InternalFieldCount())
-    return;
-  FreeObjectPrivate(pObj->GetAlignedPointerFromInternalField(0));
+  CFXJS_PerObjectData* pData = CFXJS_PerObjectData::GetFromObject(pObj);
   pObj->SetAlignedPointerInInternalField(0, nullptr);
+  pObj->SetAlignedPointerInInternalField(1, nullptr);
+  delete pData;
 }
 
 int CFXJS_Engine::DefineObj(const char* sObjName,
@@ -381,17 +396,17 @@ void CFXJS_Engine::InitializeEngine() {
   for (int i = 0; i < maxID; ++i) {
     CFXJS_ObjDefinition* pObjDef = CFXJS_ObjDefinition::ForID(m_isolate, i);
     if (pObjDef->m_ObjType == FXJSOBJTYPE_GLOBAL) {
-      v8Context->Global()
-          ->GetPrototype()
-          ->ToObject(v8Context)
-          .ToLocalChecked()
-          ->SetAlignedPointerInInternalField(0, new CFXJS_PerObjectData(i));
-
-      if (pObjDef->m_pConstructor)
+      CFXJS_PerObjectData::SetInObject(new CFXJS_PerObjectData(i),
+                                       v8Context->Global()
+                                           ->GetPrototype()
+                                           ->ToObject(v8Context)
+                                           .ToLocalChecked());
+      if (pObjDef->m_pConstructor) {
         pObjDef->m_pConstructor(this, v8Context->Global()
                                           ->GetPrototype()
                                           ->ToObject(v8Context)
                                           .ToLocalChecked());
+      }
     } else if (pObjDef->m_ObjType == FXJSOBJTYPE_STATIC) {
       v8::Local<v8::String> pObjName =
           v8::String::NewFromUtf8(m_isolate, pObjDef->m_ObjName,
@@ -500,15 +515,14 @@ v8::Local<v8::Object> CFXJS_Engine::NewFxDynamicObj(int nObjDefnID,
   if (!pObjDef->GetInstanceTemplate()->NewInstance(context).ToLocal(&obj))
     return v8::Local<v8::Object>();
 
-  CFXJS_PerObjectData* pPerObjData = new CFXJS_PerObjectData(nObjDefnID);
-  obj->SetAlignedPointerInInternalField(0, pPerObjData);
+  CFXJS_PerObjectData* pObjData = new CFXJS_PerObjectData(nObjDefnID);
+  CFXJS_PerObjectData::SetInObject(pObjData, obj);
   if (pObjDef->m_pConstructor)
     pObjDef->m_pConstructor(this, obj);
 
-  if (!bStatic && FXJS_PerIsolateData::Get(m_isolate)->m_pDynamicObjsMap) {
-    FXJS_PerIsolateData::Get(m_isolate)->m_pDynamicObjsMap->set(pPerObjData,
-                                                                obj);
-  }
+  if (!bStatic && FXJS_PerIsolateData::Get(m_isolate)->m_pDynamicObjsMap)
+    FXJS_PerIsolateData::Get(m_isolate)->m_pDynamicObjsMap->set(pObjData, obj);
+
   return obj;
 }
 
@@ -534,34 +548,25 @@ void CFXJS_Engine::Error(const CFX_WideString& message) {
 }
 
 void CFXJS_Engine::SetObjectPrivate(v8::Local<v8::Object> pObj, void* p) {
-  if (pObj.IsEmpty() || !pObj->InternalFieldCount())
-    return;
-  CFXJS_PerObjectData* pPerObjectData = static_cast<CFXJS_PerObjectData*>(
-      pObj->GetAlignedPointerFromInternalField(0));
+  CFXJS_PerObjectData* pPerObjectData =
+      CFXJS_PerObjectData::GetFromObject(pObj);
   if (!pPerObjectData)
     return;
   pPerObjectData->m_pPrivate = p;
 }
 
 void* CFXJS_Engine::GetObjectPrivate(v8::Local<v8::Object> pObj) {
-  if (pObj.IsEmpty())
-    return nullptr;
-  CFXJS_PerObjectData* pPerObjectData = nullptr;
-  if (pObj->InternalFieldCount()) {
-    pPerObjectData = static_cast<CFXJS_PerObjectData*>(
-        pObj->GetAlignedPointerFromInternalField(0));
-  } else {
+  CFXJS_PerObjectData* pData = CFXJS_PerObjectData::GetFromObject(pObj);
+  if (!pData && !pObj.IsEmpty()) {
     // It could be a global proxy object.
     v8::Local<v8::Value> v = pObj->GetPrototype();
     v8::Local<v8::Context> context = m_isolate->GetCurrentContext();
     if (v->IsObject()) {
-      pPerObjectData = static_cast<CFXJS_PerObjectData*>(
-          v->ToObject(context)
-              .ToLocalChecked()
-              ->GetAlignedPointerFromInternalField(0));
+      pData = CFXJS_PerObjectData::GetFromObject(
+          v->ToObject(context).ToLocalChecked());
     }
   }
-  return pPerObjectData ? pPerObjectData->m_pPrivate : nullptr;
+  return pData ? pData->m_pPrivate : nullptr;
 }
 
 v8::Local<v8::Value> CFXJS_Engine::GetObjectProperty(