CVE-2023-23557: Access of Resource Using Incompatible Type ('Type Confusion') (CWE-843)
value: An error in Hermes' algorithm for copying objects properties prior to commit a00d237346894c6067a594983be6634f4168c9ad could be used by a malicious attacker to execute arbitrary code via type confusion. Note that this is only exploitable in cases where Hermes is used to execute untrusted JavaScript. Hence, most React Native applications are not affected.
CWE description: The product allocates or initializes a resource such as a pointer, object, or variable using one type, but it later accesses that resource using a type that is incompatible with the original type.
CVSS v3 base score: 9.8
Published: 2023-05-18T22:15Z
Repository: https://github.com/facebook/hermes
Language: C++
Files changed: lib/VM/JSLib/HermesBuiltin.cpp, test/hermes/regress-spread-operator.js, unittests/API/APITest.cpp
Commit date: 2023-01-10 11:43:42 -0800
Commit message: Use copyDataProperties slow path for objects with accessors

Summary:
Fix a bug present in `hermesBuiltinCopyDataProperties`.
`hermesBuiltinCopyDataProperties` is meant to copy an object. It does
this by iterating over all the properties in the source object, and
then writing the key-value pair into a new object which is returned.The
iteration is done using `JSObject::forEachOwnPropertyWhile`, providing
a callback to process each property. It will take the property id, look
it up in the source object using `getNamedPropertyValue_RJS`, then take
that resulting value and write it into the target. However, that API
explicitly states the following:

> Obviously the callbacks shouldn't be  doing naughty things like modifying
the property map or creating new hidden classes (even implicitly)

But, `getNamedPropertyValue_RJS` can
perform arbitrary JS execution if it is a getter, including
adding/deleting new properties, which would modify the object's
HiddenClass, thus violating `JSObject::forEachOwnPropertyWhile`'s
precondition.Therefore, the fix is to use the correct, but slower path,
whenever there is an accessor on the object. This code path is more 1:1
to the spec. At the beginning, it will simply find all the properties
of the source object using `JSObject::getOwnPropertyKeys` and then
iterate on those, rather than use `JSObject::forEachOwnPropertyWhile`.
This way, we are allowed to manipulate the source object however we
wish, which is necessary in order to call `getNamedPropertyValue_RJS`.

Reviewed By: neildhar

Differential Revision: D41701871

fbshipit-source-id: c4be179f57d52827f12ca26b1ab8d17cdccd7447

Diff: diff --git a/lib/VM/JSLib/HermesBuiltin.cpp b/lib/VM/JSLib/HermesBuiltin.cpp
index 72bb2fe414..c2cbf23c13 100644
--- a/lib/VM/JSLib/HermesBuiltin.cpp
+++ b/lib/VM/JSLib/HermesBuiltin.cpp
@@ -249,16 +249,12 @@ namespace {
 CallResult<HermesValue> copyDataPropertiesSlowPath_RJS(
     Runtime &runtime,
     Handle<JSObject> target,
     Handle<JSObject> from,
     Handle<JSObject> excludedItems) {
-  assert(
-      from->isProxyObject() &&
-      "copyDataPropertiesSlowPath_RJS is only for Proxy");
-
   // 5. Let keys be ? from.[[OwnPropertyKeys]]().
-  auto cr = JSProxy::getOwnPropertyKeys(
+  auto cr = JSObject::getOwnPropertyKeys(
       from,
       runtime,
       OwnKeysFlags()
           .plusIncludeSymbols()
           .plusIncludeNonSymbols()
@@ -308,19 +304,21 @@ CallResult<HermesValue> copyDataPropertiesSlowPath_RJS(
         continue;
     }
 
     //   i. Let desc be ? from.[[GetOwnProperty]](nextKey).
     ComputedPropertyDescriptor desc;
-    CallResult<bool> crb =
-        JSProxy::getOwnProperty(from, runtime, nextKeyHandle, desc, nullptr);
+    CallResult<bool> crb = JSObject::getOwnComputedDescriptor(
+        from, runtime, nextKeyHandle, tmpSymbolStorage, desc);
     if (LLVM_UNLIKELY(crb == ExecutionStatus::EXCEPTION))
       return ExecutionStatus::EXCEPTION;
     //   ii. If desc is not undefined and desc.[[Enumerable]] is true, then
-    if (*crb && desc.flags.enumerable) {
+    // TODO(T141997867), move this special case behavior for host objects to
+    // getOwnComputedDescriptor.
+    if ((*crb && desc.flags.enumerable) || from->isHostObject()) {
       //     1. Let propValue be ? Get(from, nextKey).
       CallResult<PseudoHandle<>> crv =
-          JSProxy::getComputed(from, runtime, nextKeyHandle, from);
+          JSObject::getComputed_RJS(from, runtime, nextKeyHandle);
       if (LLVM_UNLIKELY(crv == ExecutionStatus::EXCEPTION))
         return ExecutionStatus::EXCEPTION;
       propValueHandle = std::move(*crv);
       //     2. Perform ! CreateDataProperty(target, nextKey, propValue).
       crb = JSObject::defineOwnComputed(
@@ -378,11 +376,17 @@ hermesBuiltinCopyDataProperties(void *, Runtime &runtime, NativeArgs args) {
   Handle<JSObject> excludedItems = args.dyncastArg<JSObject>(2);
   assert(
       (!excludedItems || !excludedItems->isProxyObject()) &&
       "excludedItems internal List is a Proxy");
 
-  if (source->isProxyObject()) {
+  // We cannot use the fast path if the object is a proxy, host object, or when
+  // there could potentially be an accessor defined on the object. This is
+  // because in order to use JSObject::forEachOwnPropertyWhile, we must not
+  // modify the underlying property map or hidden class. However, if we have an
+  // accessor, we cannot guarantee that condition, so we use the slow path.
+  if (source->isProxyObject() || source->isHostObject() ||
+      source->getClass(runtime)->getMayHaveAccessor()) {
     return copyDataPropertiesSlowPath_RJS(
         runtime, target, source, excludedItems);
   }
 
   MutableHandle<> nameHandle{runtime};
@@ -456,16 +460,13 @@ hermesBuiltinCopyDataProperties(void *, Runtime &runtime, NativeArgs args) {
               "but excludedItems should never be a proxy");
           if (*cr)
             return true;
         }
 
-        auto cr =
-            JSObject::getNamedPropertyValue_RJS(source, runtime, source, desc);
-        if (LLVM_UNLIKELY(cr == ExecutionStatus::EXCEPTION))
-          return false;
-
-        valueHandle = std::move(*cr);
+        SmallHermesValue shv =
+            JSObject::getNamedSlotValueUnsafe(*source, runtime, desc);
+        valueHandle = runtime.makeHandle(shv.unboxToHV(runtime));
 
         // sym can be an index-like property, so we have to bypass the assert in
         // defineOwnPropertyInternal.
         if (LLVM_UNLIKELY(
                 JSObject::defineOwnPropertyInternal(
diff --git a/test/hermes/regress-spread-operator.js b/test/hermes/regress-spread-operator.js
new file mode 100644
index 0000000000..aa06a1a04a
--- /dev/null
+++ b/test/hermes/regress-spread-operator.js
@@ -0,0 +1,32 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -O %s | %FileCheck %s
+
+// Check that the spread operator behaves correctly when modifying the
+// underlying hidden class while iterating over an object's properties in order
+// to copy it.
+
+var obj = {
+  get foo() {
+    this.tutu = 'hi';
+    delete obj.bar;
+    obj.tutu = 'lol';
+  },
+   bar: 'bar'
+};
+print({ ...obj }.tutu)
+// CHECK: undefined
+
+var obj2 = {
+  get foo() {
+    obj2.bar = 'foo'
+  },
+  bar: 'bar'
+};
+print({ ...obj2 }.bar)
+// CHECK: foo
diff --git a/unittests/API/APITest.cpp b/unittests/API/APITest.cpp
index ba81d78b9b..fe758e1e23 100644
--- a/unittests/API/APITest.cpp
+++ b/unittests/API/APITest.cpp
@@ -491,10 +491,34 @@ JSON.stringify(JSON.parse(out).callstack.map(x => x.SourceLocation));
   const char *expected =
       "[\"crashCode.js:2:41\",\"crashCode.js:3:27\",\"crashCode.js:4:27\",\"crashCode.js:5:14\",null]";
   EXPECT_EQ(callstack, expected);
 }
 
+TEST_F(HermesRuntimeTest, SpreadHostObjectWithOwnProperties) {
+  class HostObjectWithPropertyNames : public HostObject {
+    std::vector<PropNameID> getPropertyNames(Runtime &rt) override {
+      return PropNameID::names(rt, "prop1", "1", "2", "prop2", "3");
+    }
+    Value get(Runtime &runtime, const PropNameID &name) override {
+      return Value();
+    }
+  };
+
+  Object ho = Object::createFromHostObject(
+      *rt, std::make_shared<HostObjectWithPropertyNames>());
+  rt->global().setProperty(*rt, "ho", ho);
+
+  auto res = eval(R"###(
+var spreaded = {...ho};
+var props = Object.getOwnPropertyNames(spreaded);
+props.toString();
+)###")
+                 .getString(*rt)
+                 .utf8(*rt);
+  EXPECT_EQ(res, "1,2,3,prop1,prop2");
+}
+
 TEST_F(HermesRuntimeTest, HostObjectWithOwnProperties) {
   class HostObjectWithPropertyNames : public HostObject {
     std::vector<PropNameID> getPropertyNames(Runtime &rt) override {
       return PropNameID::names(rt, "prop1", "1", "2", "prop2", "3");
     }

Vulnerable code:   assert(
      from->isProxyObject() &&
      "copyDataPropertiesSlowPath_RJS is only for Proxy");

  auto cr = JSProxy::getOwnPropertyKeys(
    CallResult<bool> crb =
        JSProxy::getOwnProperty(from, runtime, nextKeyHandle, desc, nullptr);
    if (*crb && desc.flags.enumerable) {
          JSProxy::getComputed(from, runtime, nextKeyHandle, from);
  if (source->isProxyObject()) {
        auto cr =
            JSObject::getNamedPropertyValue_RJS(source, runtime, source, desc);
        if (LLVM_UNLIKELY(cr == ExecutionStatus::EXCEPTION))
          return false;

        valueHandle = std::move(*cr);
Fixed code:   auto cr = JSObject::getOwnPropertyKeys(
    CallResult<bool> crb = JSObject::getOwnComputedDescriptor(
        from, runtime, nextKeyHandle, tmpSymbolStorage, desc);
    // TODO(T141997867), move this special case behavior for host objects to
    // getOwnComputedDescriptor.
    if ((*crb && desc.flags.enumerable) || from->isHostObject()) {
          JSObject::getComputed_RJS(from, runtime, nextKeyHandle);
  // We cannot use the fast path if the object is a proxy, host object, or when
  // there could potentially be an accessor defined on the object. This is
  // because in order to use JSObject::forEachOwnPropertyWhile, we must not
  // modify the underlying property map or hidden class. However, if we have an
  // accessor, we cannot guarantee that condition, so we use the slow path.
  if (source->isProxyObject() || source->isHostObject() ||
      source->getClass(runtime)->getMayHaveAccessor()) {
        SmallHermesValue shv =
            JSObject::getNamedSlotValueUnsafe(*source, runtime, desc);
        valueHandle = runtime.makeHandle(shv.unboxToHV(runtime));
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

// RUN: %hermes -O %s | %FileCheck %s

// Check that the spread operator behaves correctly when modifying the
// underlying hidden class while iterating over an object's properties in order
// to copy it.

var obj = {
  get foo() {
    this.tutu = 'hi';
    delete obj.bar;
    obj.tutu = 'lol';
  },
   bar: 'bar'
};
print({ ...obj }.tutu)
// CHECK: undefined

var obj2 = {
  get foo() {
    obj2.bar = 'foo'
  },
  bar: 'bar'
};
print({ ...obj2 }.bar)
// CHECK: foo
TEST_F(HermesRuntimeTest, SpreadHostObjectWithOwnProperties) {
  class HostObjectWithPropertyNames : public HostObject {
    std::vector<PropNameID> getPropertyNames(Runtime &rt) override {
      return PropNameID::names(rt, "prop1", "1", "2", "prop2", "3");
    }
    Value get(Runtime &runtime, const PropNameID &name) override {
      return Value();
    }
  };

  Object ho = Object::createFromHostObject(
      *rt, std::make_shared<HostObjectWithPropertyNames>());
  rt->global().setProperty(*rt, "ho", ho);

  auto res = eval(R"###(
var spreaded = {...ho};
var props = Object.getOwnPropertyNames(spreaded);
props.toString();
)###")
                 .getString(*rt)
                 .utf8(*rt);
  EXPECT_EQ(res, "1,2,3,prop1,prop2");
}