CVE-2023-23557 (C++, published 2023-05-18T22:15Z)
Description: 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.
Severity: nan (CVSS v3 base score: 9.8)
Weakness: CWE-843 \u2014 Access of Resource Using Incompatible Type ('Type Confusion')
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.
Repository: https://github.com/facebook/hermes
Files changed:
lib/VM/JSLib/HermesBuiltin.cpp
test/hermes/regress-spread-operator.js
unittests/API/APITest.cpp
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


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");
}