CVE-2020-1915: Out-of-bounds Read (CWE-125)
value: An out-of-bounds read in the JavaScript Interpreter in Facebook Hermes prior to commit 8cb935cd3b2321c46aa6b7ed8454d95c75a7fca0 allows attackers to cause a denial of service attack or possible further memory corruption via crafted JavaScript. Note that this is only exploitable if the application using Hermes permits evaluation of untrusted JavaScript. Hence, most React Native applications are not affected.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.5
Published: 2020-10-26T21:15Z
Repository: https://github.com/facebook/hermes
Language: C++
Files changed: lib/VM/JSObject.cpp, test/hermes/proxy.js
Commit date: 2020-09-25 20:54:05 -0700
Commit message: Handle set where internalSetter and Proxy are both true

Summary:
If putComputed is called on a proxy whose target's prototype
is an array with a propname of 'length', then internalSetter will be
true, and the receiver will be a proxy.  In that case, proxy needs to
win; the behavior may assert or be UB otherwise.

Reviewed By: tmikov

Differential Revision: D23916279

fbshipit-source-id: c760356d48a02ece565fb4bc1acdafd7ccad7c68

Diff: diff --git a/lib/VM/JSObject.cpp b/lib/VM/JSObject.cpp
index 1a55dc69e..b06dbad15 100644
--- a/lib/VM/JSObject.cpp
+++ b/lib/VM/JSObject.cpp
@@ -1697,37 +1697,42 @@ CallResult<bool> JSObject::putComputedWithReceiver_RJS(
     }
 
     if (LLVM_UNLIKELY(
             desc.flags.internalSetter || receiverHandle->isHostObject() ||
             receiverHandle->isProxyObject())) {
+      // If putComputed is called on a proxy whose target's prototype
+      // is an array with a propname of 'length', then internalSetter
+      // will be true, and the receiver will be a proxy.  In that case,
+      // proxy wins.
+      if (receiverHandle->isProxyObject()) {
+        if (*descDefinedRes) {
+          dpf.setValue = 1;
+        } else {
+          dpf = DefinePropertyFlags::getDefaultNewPropertyFlags();
+        }
+        return JSProxy::defineOwnProperty(
+            receiverHandle,
+            runtime,
+            nameValPrimitiveHandle,
+            dpf,
+            valueHandle,
+            opFlags);
+      }
       SymbolID id{};
       LAZY_TO_IDENTIFIER(runtime, nameValPrimitiveHandle, id);
       if (desc.flags.internalSetter) {
         return internalSetter(
             receiverHandle,
             runtime,
             id,
             desc.castToNamedPropertyDescriptorRef(),
             valueHandle,
             opFlags);
-      } else if (receiverHandle->isHostObject()) {
-        return vmcast<HostObject>(receiverHandle.get())->set(id, *valueHandle);
       }
       assert(
-          receiverHandle->isProxyObject() && "descriptor flags are impossible");
-      if (*descDefinedRes) {
-        dpf.setValue = 1;
-      } else {
-        dpf = DefinePropertyFlags::getDefaultNewPropertyFlags();
-      }
-      return JSProxy::defineOwnProperty(
-          receiverHandle,
-          runtime,
-          nameValPrimitiveHandle,
-          dpf,
-          valueHandle,
-          opFlags);
+          receiverHandle->isHostObject() && "descriptor flags are impossible");
+      return vmcast<HostObject>(receiverHandle.get())->set(id, *valueHandle);
     }
   }
 
   /// Can we add more properties?
   if (LLVM_UNLIKELY(!receiverHandle->isExtensible())) {
diff --git a/test/hermes/proxy.js b/test/hermes/proxy.js
index 1f82cdffa..bc8102f3e 100644
--- a/test/hermes/proxy.js
+++ b/test/hermes/proxy.js
@@ -2123,7 +2123,16 @@ checkDeep({...f})(_ => ({a:1, b:2}))
 
 // Check that defining a property in a Proxy target which is an array
 // uses fast array access (this will trip an assert otherwise)
 new Proxy([], {}).unshift(0);
 
+// If putComputed is called on a proxy whose target's prototype is an
+// array with a propname of 'length', then internalSetter will be
+// true, and the receiver will be a proxy.  In that case, proxy needs
+// to win; the behavior may assert or be UB otherwise.
+var p = new Proxy(Object.create([]), {});
+// using String() forces putComputed
+p[String('length')] = 0x123;
+p[0xABC] = 1111;
+
 print('done');
 // CHECK-LABEL: done

Vulnerable code:       } else if (receiverHandle->isHostObject()) {
        return vmcast<HostObject>(receiverHandle.get())->set(id, *valueHandle);
          receiverHandle->isProxyObject() && "descriptor flags are impossible");
      if (*descDefinedRes) {
        dpf.setValue = 1;
      } else {
        dpf = DefinePropertyFlags::getDefaultNewPropertyFlags();
      }
      return JSProxy::defineOwnProperty(
          receiverHandle,
          runtime,
          nameValPrimitiveHandle,
          dpf,
          valueHandle,
          opFlags);
Fixed code:       // If putComputed is called on a proxy whose target's prototype
      // is an array with a propname of 'length', then internalSetter
      // will be true, and the receiver will be a proxy.  In that case,
      // proxy wins.
      if (receiverHandle->isProxyObject()) {
        if (*descDefinedRes) {
          dpf.setValue = 1;
        } else {
          dpf = DefinePropertyFlags::getDefaultNewPropertyFlags();
        }
        return JSProxy::defineOwnProperty(
            receiverHandle,
            runtime,
            nameValPrimitiveHandle,
            dpf,
            valueHandle,
            opFlags);
      }
          receiverHandle->isHostObject() && "descriptor flags are impossible");
      return vmcast<HostObject>(receiverHandle.get())->set(id, *valueHandle);
// If putComputed is called on a proxy whose target's prototype is an
// array with a propname of 'length', then internalSetter will be
// true, and the receiver will be a proxy.  In that case, proxy needs
// to win; the behavior may assert or be UB otherwise.
var p = new Proxy(Object.create([]), {});
// using String() forces putComputed
p[String('length')] = 0x123;
p[0xABC] = 1111;