CVE-2021-21413: Improper Control of Dynamically-Managed Code Resources (CWE-913)
value: isolated-vm is a library for nodejs which gives you access to v8's Isolate interface. Versions of isolated-vm before v4.0.0 have API pitfalls which may make it easy for implementers to expose supposed secure isolates to the permissions of the main nodejs isolate. Reference objects allow access to the underlying reference's full prototype chain. In an environment where the implementer has exposed a Reference instance to an attacker they would be able to use it to acquire a Reference to the nodejs context's Function object. Similar application-specific attacks could be possible by modifying the local prototype of other API objects. Access to NativeModule objects could allow an attacker to load and run native code from anywhere on the filesystem. If combined with, for example, a file upload API this would allow for arbitrary code execution. This is addressed in v4.0.0 through a series of related changes.
CWE description: The product does not properly restrict reading from or writing to dynamically-managed code resources such as variables, objects, classes, attributes, functions, or executable instructions or statements.
CVSS v3 base score: 9.6
Published: 2021-03-30T23:15Z
Repository: https://github.com/laverdet/isolated-vm
Language: Markdown
Files changed: CHANGELOG.md, README.md, isolated-vm.d.ts, src/isolate/strings.h, src/module/reference_handle.cc, src/module/reference_handle.h, tests/reference.js
Commit date: 2021-03-20 14:40:07 -0500
Commit message: Don't invoke accessors or proxies via Reference functions

Diff: diff --git a/CHANGELOG.md b/CHANGELOG.md
index 37ab864..112b529 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,8 +1,9 @@
 ## v4.0.0
 - `Callback` class addeed.
-- When possible, `reference.get()` will return a function proxy instead of a `Reference`.
+- When possible, `reference.get()` will return a function delegate instead of a `Reference`.
 - `reference.get()` will no longer return inherited properties by default.
 - `result` property on `eval` and `evalClosure` has been removed. The result is now just the return
 value.
 - All `isolated-vm` class prototypes, and most instances are frozen.
 - `isolate.cpuTime` and `isolate.wallTime` now return bigints.
+- Proxies and accessors are no longer tolerated via `reference.get`, and related functions.
diff --git a/README.md b/README.md
index 3946ac4..91aab53 100644
--- a/README.md
+++ b/README.md
@@ -342,11 +342,11 @@ functions to most isolated-vm functions.
 A instance of [`Reference`](#class-reference-transferable) is a pointer to a value stored in any isolate.
 
 ##### `new ivm.Reference(value, options)`
 * `value` - The value to create a reference to.
 * `options` *[object]*
-  * `inheritUnsafe` *[boolean]* - If enabled then the `get` family of functions will follow the
+  * `unsafeInherit` *[boolean]* - If enabled then the `get` family of functions will follow the
     object's prototype chain. References created with this option should never be given to untrusted
     code.
 
 ##### `reference.typeof` *[string]*
 
@@ -386,11 +386,10 @@ all attempts to access the reference will throw an error.
 
 ##### `reference.delete(property)` *[Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)*
 ##### `reference.deleteIgnored(property)`
 ##### `reference.deleteSync(property)`
 * `property` *[transferable]* - The property to access on this object.
-* **return** `true` or `false`
 
 Delete a property from this reference, as if using `delete reference[property]`
 
 ##### `reference.get(property, options)` *[Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)*
 ##### `reference.getSync(property, options)`
@@ -399,21 +398,19 @@ Delete a property from this reference, as if using `delete reference[property]`
 	* [`{ ...TransferOptions }`](#transferoptions)
 * **return** A [`Reference`](#class-reference-transferable) object.
 
 Will access a reference as if using `reference[property]` and transfer the value out.
 
+If the object is a proxy, or if the property is a getter, this method will throw.
+
 ##### `reference.set(property, value, options)` *[Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)*
 ##### `reference.setIgnored(property, value, options)`
 ##### `reference.setSync(property, value, options)`
 * `property` *[transferable]* - The property to set on this object.
 * `value` *[transferable]* - The value to set on this object.
 * `options` *[object]*
 	* [`{ ...TransferOptions }`](#transferoptions)
-* **return** `true` or `false`
-
-Returns a boolean indicating whether or not this operation succeeded. I'm actually not really sure
-when `false` would be returned, I'm just giving you the result back straight from the v8 API.
 
 ##### `reference.apply(receiver, arguments, options)` *[Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)*
 ##### `reference.applyIgnored(receiver, arguments, options)`
 ##### `reference.applySync(receiver, arguments, options)`
 ##### `reference.applySyncPromise(receiver, arguments, options)`
diff --git a/isolated-vm.d.ts b/isolated-vm.d.ts
index 6f366de..882cdb7 100644
--- a/isolated-vm.d.ts
+++ b/isolated-vm.d.ts
@@ -7,14 +7,16 @@ declare module "isolated-vm" {
 		| boolean
 		| Isolate
 		| Context
 		| Script
 		| ExternalCopy<any>
+		| Callback<any>
 		| Copy<any>
 		| Reference<any>
 		| Dereference<any>
 		| Module
+		| ((...args: any[]) => any)
 		| typeof import("isolated-vm");
 
 	/**
 	 * This is the main reference to an isolate. Every handle to an isolate is transferable, which
 	 * means you can give isolates references to each other. An isolate will remain valid as long as
@@ -282,11 +284,11 @@ declare module "isolated-vm" {
 	/**
 	 * A instance of Reference is a pointer to a value stored in any isolate.
 	 */
 	export class Reference<T = any> {
 		private __ivm_reference: T;
-		constructor(value: T, options?: { inheritUnsafe?: boolean });
+		constructor(value: T, options?: { unsafeInherit?: boolean });
 
 		/**
 		 * This is the typeof the referenced value, and is available at any time
 		 * from any isolate. Note that this differs from the real typeof operator in
 		 * that null is "null", and Symbols are "object".
@@ -329,56 +331,54 @@ declare module "isolated-vm" {
 		release(): void;
 
 		/**
 		 * Delete a property from this reference, as if using `delete reference[property]`
 		 */
-		 delete(property: keyof T): Promise<boolean>;
+		 delete(property: keyof T): Promise<void>;
 		 deleteIgnored(property: keyof T): void;
-		 deleteSync(property: keyof T): boolean;
+		 deleteSync(property: keyof T): void;
 
 		/**
 		 * Will access a reference as if using reference[property] and return a reference to that value.
+		 *
+		 * If the object is a proxy, or if the property is a getter, this method will throw.
 		 */
 		get<Options extends TransferOptions, Key extends keyof T>(
-			property: Key, options?: Options): ResultTypeAsync<Options & AsReference, T[Key]>;
+			property: Key, options?: Options): ResultTypeAsync<Options & FallbackReference, T[Key]>;
 		getSync<Options extends TransferOptions, Key extends keyof T>(
-			property: Key, options?: Options): ResultTypeSync<Options & AsReference, T[Key]>;
+			property: Key, options?: Options): ResultTypeSync<Options & FallbackReference, T[Key]>;
 
 		/**
 		 * Will access a reference as if using reference[property] and return a reference to that value.
-		 *
-		 * @return {boolean} Indicating whether or not this operation succeeded. I'm actually not really
-		 * sure when false would be returned, I'm just giving you the result back straight from the v8
-		 * API.
 		 */
 		set<Options extends TransferOptions, Key extends keyof T>(
-			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): Promise<boolean>;
+			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): Promise<void>;
 		setIgnored<Options extends TransferOptions, Key extends keyof T>(
 			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): void;
 		setSync<Options extends TransferOptions, Key extends keyof T>(
-			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): boolean;
+			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): void;
 
 		/**
 		 * Will attempt to invoke an object as if it were a function. If the return
 		 * value is transferable it will be returned to the called of apply,
 		 * otherwise an error will be thrown.
 		 */
 		apply<Options extends ReferenceApplyOptions>(
 			receiver?: ArgumentType<Options['arguments'], ApplyArgumentThis<T>>,
 			arguments?: ArgumentsTypeBidirectional<Options, ApplyArguments<T>>,
 			options?: Options
-		): ResultTypeBidirectionalAsync<Options & ApplyAsReference, ApplyResult<T>>;
+		): ResultTypeBidirectionalAsync<Options & FallbackReference, ApplyResult<T>>;
 		applyIgnored<Options extends ReferenceApplyOptions>(
 			receiver?: ArgumentType<Options['arguments'], ApplyArgumentThis<T>>,
-			arguments?: ArgumentsTypeBidirectional<Options & ApplyAsReference, ApplyArguments<T>>,
+			arguments?: ArgumentsTypeBidirectional<Options & FallbackReference, ApplyArguments<T>>,
 			options?: Options
 		): void;
 		applySync<Options extends ReferenceApplyOptions>(
 			receiver?: ArgumentType<Options['arguments'], ApplyArgumentThis<T>>,
 			arguments?: ArgumentsTypeBidirectional<Options, ApplyArguments<T>>,
 			options?: Options
-		): ResultTypeBidirectionalSync<Options & ApplyAsReference, ApplyResult<T>>;
+		): ResultTypeBidirectionalSync<Options & FallbackReference, ApplyResult<T>>;
 
 		/**
 		 * `applySyncPromise` is a special version of `applySync` which may only be invoked on functions
 		 * belonging to the default isolate AND may only be invoked from a non-default thread. Functions
 		 * invoked in this way may return a promise and the invoking isolate will wait for that promise
@@ -390,11 +390,11 @@ declare module "isolated-vm" {
 		 */
 		applySyncPromise<Options extends ReferenceApplyOptions>(
 			receiver?: ArgumentType<Options['arguments'], ApplyArgumentThis<T>>,
 			arguments?: ArgumentsTypeBidirectional<Options, ApplyArguments<T>>,
 			options?: Options
-		): ResultTypeBidirectionalSync<Options & ApplyAsReference, ApplyResult<T>>;
+		): ResultTypeBidirectionalSync<Options & FallbackReference, ApplyResult<T>>;
 	}
 
 	/**
 	 * Dummy type referencing a type dereferenced into a different Isolate.
 	 */
@@ -705,10 +705,11 @@ declare module "isolated-vm" {
 	// Discriminating types for TransferOptions
 	type WithPromise = { promise: true };
 	type AsCopy = { copy: true };
 	type AsExternal = { externalCopy: true };
 	type AsReference = { reference: true };
+	type FallbackReference = { _reference: true };
 	type ApplyAsReference = { result: AsReference };
 	type WithTransfer = AsCopy | AsExternal | AsReference;
 
 	// Wraps a type in Promise<> if the options specify { promise: true }
 	type CheckPromise<Options, Result> = Options extends WithPromise ? Promise<Result> : Result;
@@ -725,10 +726,11 @@ declare module "isolated-vm" {
 		Options extends AsCopy ? Result :
 		Options extends AsExternal ? ExternalCopy<Result> :
 		Options extends AsReference ? Reference<Result> :
 		Result extends Transferable ? Result :
 		Result extends void ? void :
+		Options extends FallbackReference ? Reference<Result> :
 		Transferable;
 	type ResultTypeAsync<Options extends TransferOptions, Result = any> = Promise<ResultTypeBase<Options, Result>>;
 	type ResultTypeSync<Options extends TransferOptions, Result = any> = CheckPromise<Options, ResultTypeBase<Options, Result>>;
 
 	// Arguments type for functions that accept TransferOptionsBidirectional
diff --git a/src/isolate/strings.h b/src/isolate/strings.h
index 8001aa3..bf019ba 100644
--- a/src/isolate/strings.h
+++ b/src/isolate/strings.h
@@ -56,11 +56,10 @@ class StringTable {
 		String externalCopy{"externalCopy"};
 		String filename{"filename"};
 		String function{"function"};
 		String global{"global"};
 		String ignored{"ignored"};
-		String inheritUnsafe{"inheritUnsafe"};
 		String inspector{"inspector"};
 		String isolateIsDisposed{"Isolate is disposed"};
 		String isolatedVm{"isolated-vm"};
 		String length{"length"};
 		String lineOffset{"lineOffset"};
@@ -80,10 +79,11 @@ class StringTable {
 		String timeout{"timeout"};
 		String transferIn{"transferIn"};
 		String transferList{"transferList"};
 		String transferOut{"transferOut"};
 		String undefined{"undefined"};
+		String unsafeInherit{"unsafeInherit"};
 
 		String does_zap_garbage{"does_zap_garbage"};
 		String externally_allocated_size{"externally_allocated_size"};
 		String heap_size_limit{"heap_size_limit"};
 		String malloced_memory{"malloced_memory"};
diff --git a/src/module/reference_handle.cc b/src/module/reference_handle.cc
index 6d6ca39..d18ed57 100644
--- a/src/module/reference_handle.cc
+++ b/src/module/reference_handle.cc
@@ -125,11 +125,11 @@ auto ReferenceHandle::Definition() -> Local<FunctionTemplate> {
 		"typeof", MemberAccessor<decltype(&ReferenceHandle::TypeOfGetter), &ReferenceHandle::TypeOfGetter>{}
 	));
 }
 
 auto ReferenceHandle::New(Local<Value> value, MaybeLocal<Object> options) -> unique_ptr<ReferenceHandle> {
-	auto inherit = ReadOption<bool>(options, StringTable::Get().inheritUnsafe, false);
+	auto inherit = ReadOption<bool>(options, StringTable::Get().unsafeInherit, false);
 	return std::make_unique<ReferenceHandle>(value, inherit);
 }
 
 auto ReferenceHandle::TransferOut() -> unique_ptr<Transferable> {
 	return std::make_unique<ReferenceHandleTransferable>(*this);
@@ -424,151 +424,181 @@ template <int async>
 auto ReferenceHandle::Copy() -> Local<Value> {
 	return ThreePhaseTask::Run<async, CopyRunner>(*isolate, *this, context, reference);
 }
 
 /**
- * Get a property from this reference, returned as another reference
+ * Base class for get, set, and delete runners
  */
-class GetRunner : public ThreePhaseTask {
+class AccessorRunner : public ThreePhaseTask {
 	public:
-		GetRunner(
-			const ReferenceHandle& that,
-			Local<Value> key_handle,
-			MaybeLocal<Object> maybe_options,
-			bool inherit
-		) :
-				context{that.context},
-				reference{that.reference},
-				options{maybe_options, inherit ?
-					TransferOptions::Type::DeepReference : TransferOptions::Type::Reference},
-				inherit{inherit} {
-			that.CheckDisposed();
-			key = ExternalCopy::CopyIfPrimitive(key_handle);
-			if (!key) {
+		AccessorRunner(ReferenceHandle& target, Local<Value> key_handle) :
+		context{target.context},
+		target{target.reference},
+		key{ExternalCopy::CopyIfPrimitive(key_handle)} {
+			target.CheckDisposed();
+			if (!key || (!key_handle->IsName() && !key_handle->IsUint32())) {
 				throw RuntimeTypeError("Invalid `key`");
+			} else if (target.type_of != decltype(target.type_of)::Object) {
+				throw RuntimeTypeError("Reference is not an object");
+			}
+		}
+
+	protected:
+		auto GetTargetAndAlsoCheckForProxy() -> Local<Object> {
+			auto object = Local<Object>::Cast(Deref(target));
+			if (HasProxy(object)) {
+				throw RuntimeTypeError("Object is or has proxy");
+			}
+			return object;
+		}
+
+		auto GetKey(Local<Context> context) -> Local<Name> {
+			auto key_inner = key->CopyInto();
+			return (key_inner->IsString() || key_inner->IsSymbol()) ?
+				key_inner.As<Name>() : Unmaybe(key_inner->ToString(context)).As<Name>();
+		}
+
+		RemoteHandle<Context> context;
+
+	private:
+		static auto HasProxy(Local<Object> object) -> bool {
+			if (object->IsProxy()) {
+				return true;
+			} else {
+				auto proto = object->GetPrototype();
+				if (proto->IsNullOrUndefined()) {
+					return false;
+				} else {
+					return HasProxy(proto.As<Object>());
+				}
 			}
 		}
 
+		RemoteHandle<Value> target;
+		unique_ptr<ExternalCopy> key;
+};
+
+/**
+ * Get a property from this reference, returned as another reference
+ */
+class GetRunner final : public AccessorRunner {
+	public:
+		GetRunner(ReferenceHandle& target, Local<Value> key_handle, MaybeLocal<Object> maybe_options) :
+		AccessorRunner{target, key_handle},
+		options{maybe_options, target.inherit ?
+			TransferOptions::Type::DeepReference : TransferOptions::Type::Reference},
+		inherit{target.inherit} {}
+
 		void Phase2() final {
-			Local<Context> context_handle = Deref(context);
-			Context::Scope context_scope{context_handle};
-			Local<Value> key_inner = key->CopyInto();
-			Local<Object> object = Local<Object>::Cast(Deref(reference));
-			bool allow = [&]() {
-				if (!inherit) {
-					if (key_inner->IsName()) {
-						return Unmaybe(object->HasRealNamedProperty(context_handle, key_inner.As<Name>()));
-					} else if (key_inner->IsNumber()) {
-						return Unmaybe(object->HasRealIndexedProperty(context_handle, HandleCast<uint32_t>(key_inner)));
-					} else {
-						return false;
-					}
+			// Setup
+			auto* isolate = Isolate::GetCurrent();
+			auto context = Deref(this->context);
+			Context::Scope context_scope{context};
+			auto name = GetKey(context);
+			auto object = GetTargetAndAlsoCheckForProxy();
+
+			// Get property
+			ret = TransferOut([&]() {
+				if (inherit) {
+					// To avoid accessors I guess we have to walk the prototype chain ourselves
+					auto target = object;
+					do {
+						if (Unmaybe(target->HasOwnProperty(context, name))) {
+							if (Unmaybe(target->HasRealNamedCallbackProperty(context, name))) {
+								throw RuntimeTypeError("Property is getter");
+							}
+							return Unmaybe(target->GetRealNamedProperty(context, name));
+						}
+						auto next = target->GetPrototype();
+						if (next->IsNullOrUndefined()) {
+							return Undefined(isolate).As<Value>();
+						}
+						target = next.As<Object>();
+					} while (true);
+				} else if (!Unmaybe(object->HasOwnProperty(context, name))) {
+					return Undefined(isolate).As<Value>();
+				} else if (Unmaybe(object->HasRealNamedCallbackProperty(context, name))) {
+					throw RuntimeTypeError("Property is getter");
+				} else {
+					return Unmaybe(object->Get(context, name));
 				}
-				return true;
-			}();
-			Local<Value> value = allow ?
-				Unmaybe(object->Get(context_handle, key_inner)) :
-				Undefined(Isolate::GetCurrent()).As<Value>();
-			ret = TransferOut(value, options);
+			}(), options);
 		}
 
 		auto Phase3() -> Local<Value> final {
 			return ret->TransferIn();
 		}
 
 	private:
-		unique_ptr<ExternalCopy> key;
-		RemoteHandle<Context> context;
-		RemoteHandle<Value> reference;
 		unique_ptr<Transferable> ret;
 		TransferOptions options;
 		bool inherit;
 };
 template <int async>
 auto ReferenceHandle::Get(Local<Value> key_handle, MaybeLocal<Object> maybe_options) -> Local<Value> {
-	return ThreePhaseTask::Run<async, GetRunner>(*isolate, *this, key_handle, maybe_options, inherit);
+	return ThreePhaseTask::Run<async, GetRunner>(*isolate, *this, key_handle, maybe_options);
 }
 
 /**
  * Delete a property on this reference
  */
-class DeleteRunner : public ThreePhaseTask {
+class DeleteRunner final : public AccessorRunner {
 	public:
 		DeleteRunner(ReferenceHandle& that, Local<Value> key_handle) :
-				key{ExternalCopy::CopyIfPrimitive(key_handle)},
-				context{that.context},
-				reference{that.reference} {
-			that.CheckDisposed();
-			if (!key) {
-				throw RuntimeTypeError("Invalid `key`");
-			}
-		}
+		AccessorRunner{that, key_handle} {}
 
 		void Phase2() final {
-			Local<Context> context_handle = Deref(context);
-			Context::Scope context_scope{context_handle};
-			Local<Object> object = Local<Object>::Cast(Deref(reference));
-			result = Unmaybe(object->Delete(context_handle, key->CopyInto()));
+			auto context = Deref(this->context);
+			Context::Scope context_scope{context};
+			auto object = GetTargetAndAlsoCheckForProxy();
+			if (!Unmaybe(object->Delete(context, GetKey(context)))) {
+				throw RuntimeTypeError("Delete failed");
+			}
 		}
 
 		auto Phase3() -> Local<Value> final {
-			return Boolean::New(Isolate::GetCurrent(), result);
+			return Undefined(Isolate::GetCurrent());
 		}
-
-	private:
-		unique_ptr<ExternalCopy> key;
-		RemoteHandle<Context> context;
-		RemoteHandle<Value> reference;
-		bool result = false;
 };
 template <int async>
 auto ReferenceHandle::Delete(Local<Value> key_handle) -> Local<Value> {
 	return ThreePhaseTask::Run<async, DeleteRunner>(*isolate, *this, key_handle);
 }
 
 /**
  * Attempt to set a property on this reference
  */
-class SetRunner : public ThreePhaseTask {
+class SetRunner final : public AccessorRunner {
 	public:
 		SetRunner(
 			ReferenceHandle& that,
 			Local<Value> key_handle,
 			Local<Value> val_handle,
 			MaybeLocal<Object> maybe_options
 		) :
-				key{ExternalCopy::CopyIfPrimitive(key_handle)},
-				val{TransferOut(val_handle, TransferOptions{maybe_options})},
-				context{that.context},
-				reference{that.reference} {
-			that.CheckDisposed();
-			if (!key) {
-				throw RuntimeTypeError("Invalid `key`");
-			}
-		}
+		AccessorRunner{that, key_handle},
+		val{TransferOut(val_handle, TransferOptions{maybe_options})} {}
 
 		void Phase2() final {
-			Local<Context> context_handle = Deref(context);
-			Context::Scope context_scope{context_handle};
-			Local<Value> key_inner = key->CopyInto();
-			Local<Object> object = Local<Object>::Cast(Deref(reference));
+			auto context = Deref(this->context);
+			Context::Scope context_scope{context};
+			auto name = GetKey(context);
+			auto object = GetTargetAndAlsoCheckForProxy();
 			// Delete key before transferring in, potentially freeing up some v8 heap
-			Unmaybe(object->Delete(context_handle, key_inner));
-			Local<Value> val_inner = val->TransferIn();
-			did_set = Unmaybe(object->Set(context_handle, key_inner, val_inner));
+			Unmaybe(object->Delete(context, name));
+			auto val_inner = val->TransferIn();
+			if (!Unmaybe(object->CreateDataProperty(context, GetKey(context), val_inner))) {
+				throw RuntimeTypeError("Set failed");
+			}
 		}
 
 		auto Phase3() -> Local<Value> final {
-			return Boolean::New(Isolate::GetCurrent(), did_set);
+			return Undefined(Isolate::GetCurrent());
 		}
 
 	private:
-		unique_ptr<ExternalCopy> key;
 		unique_ptr<Transferable> val;
-		RemoteHandle<Context> context;
-		RemoteHandle<Value> reference;
-		bool did_set = false;
 };
 template <int async>
 auto ReferenceHandle::Set(Local<Value> key_handle, Local<Value> val_handle, MaybeLocal<Object> maybe_options) -> Local<Value> {
 	return ThreePhaseTask::Run<async, SetRunner>(*isolate, *this, key_handle, val_handle, maybe_options);
 }
diff --git a/src/module/reference_handle.h b/src/module/reference_handle.h
index 700c153..55a6503 100644
--- a/src/module/reference_handle.h
+++ b/src/module/reference_handle.h
@@ -11,10 +11,11 @@ namespace detail {
 
 /**
  * Holds common data for ReferenceHandle and ReferenceHandleTransferable
  */
 class ReferenceData {
+	friend class AccessorRunner;
 	public:
 		enum class TypeOf { Null, Undefined, Number, String, Boolean, Object, Function };
 
 		explicit ReferenceData(v8::Local<v8::Value> value, bool inherit = false);
 		ReferenceData(
@@ -39,13 +40,12 @@ class ReferenceData {
  * This will be a reference to any v8 Value in any isolate.
  */
 class ReferenceHandle : public TransferableHandle, public detail::ReferenceData {
 	friend class ApplyRunner;
 	friend class CopyRunner;
+	friend class AccessorRunner;
 	friend class GetRunner;
-	friend class DeleteRunner;
-	friend class SetRunner;
 	public:
 		using TypeOf = detail::ReferenceData::TypeOf;
 
 		template <class ...Args>
 		explicit ReferenceHandle(Args&&... args) : ReferenceData{std::forward<Args>(args)...} {}
diff --git a/tests/reference.js b/tests/reference.js
index 92e3467..0852183 100644
--- a/tests/reference.js
+++ b/tests/reference.js
@@ -1,22 +1,120 @@
+'use strict';
 const ivm = require('isolated-vm');
-const { strictEqual } = require('assert');
+const { strictEqual, throws } = require('assert');
+let trap = false;
 
-const foo = { foo: 1 };
-const bar = Object.create(foo);
-bar.bar = 2;
-const etc = Object.create(bar);
-etc.etc = bar;
 {
-	const ref = new ivm.Reference(etc);
-	strictEqual(ref.getSync('bar'), undefined);
-	strictEqual(ref.getSync('etc').getSync('foo'), undefined);
-	strictEqual(ref.getSync('etc').getSync('bar'), 2);
+	// Set up inheritance
+	const foo = { foo: 1 };
+	const bar = Object.create(foo);
+	bar.bar = 2;
+	const etc = Object.create(bar);
+	etc.etc = bar;
+
+	{
+		// Test without inheritance
+		const ref = new ivm.Reference(etc);
+		strictEqual(ref.getSync('bar'), undefined);
+		strictEqual(ref.getSync('etc').getSync('foo'), undefined);
+		strictEqual(ref.getSync('etc').getSync('bar'), 2);
+		ref.setSync('prop', 1);
+		strictEqual(ref.getSync('prop'), 1);
+		ref.deleteSync('prop');
+		strictEqual(ref.getSync('prop'), undefined);
+	}
+
+	{
+		// Test with inheritance
+		const ref = new ivm.Reference(etc, { unsafeInherit: true });
+		strictEqual(ref.getSync('bar'), 2);
+		strictEqual(ref.getSync('etc').getSync('foo'), 1);
+		strictEqual(ref.getSync('etc').getSync('bar'), 2);
+		ref.getSync('etc').setSync('prop', 1);
+		ref.setSync('prop', 2);
+		strictEqual(ref.getSync('prop'), 2);
+		ref.deleteSync('prop');
+		strictEqual(ref.getSync('prop'), 1);
+	}
+}
+
+{
+	// Set up getter / setters
+	let setter = 0;
+	const foo = {
+		get str() { trap = true; return 'got' },
+		set str(_) { trap = true },
+	};
+	Object.defineProperty(foo, 0, {
+		get() { trap = true; return 'got' },
+		set() { trap = true },
+	});
+
+	{
+		// Test plain accessors
+		const ref = new ivm.Reference(foo);
+		throws(() => ref.getSync('str'));
+		throws(() => ref.getSync(0));
+		throws(() => ref.setSync(0, undefined));
+	}
+
+	{
+		// Test accessors + inheritance
+		const ref = new ivm.Reference(Object.create(foo), { unsafeInherit: true });
+		throws(() => ref.getSync('str'));
+		ref.setSync('str', undefined);
+		throws(() => ref.getSync(0));
+	}
+}
+
+{
+	// Set up evil proxy
+	const val = { prop: 1 };
+	const prox = new Proxy(val, {
+		get() { trap = true },
+    set() { trap = true },
+    getOwnPropertyDescriptor() { trap = true },
+    getPrototypeOf() { trap = true },
+    has() { trap = true },
+    ownKeys() { trap = true },
+	});
+	const inherited = Object.create(prox);
+
+	{
+		// Test proxy
+		const ref = new ivm.Reference(prox);
+		throws(() => ref.getSync('prop'));
+	}
+
+	{
+		// Test inherited proxy
+		const ref = new ivm.Reference(inherited);
+		throws(() => ref.getSync('prop'),);
+	}
+
+	{
+		// Test deep inherited proxy
+		const ref = new ivm.Reference(inherited, { unsafeInherit: true });
+		throws(() => ref.getSync('prop'));
+	}
 }
+
 {
-	const ref = new ivm.Reference(etc, { inheritUnsafe: true });
-	strictEqual(ref.getSync('bar'), 2);
-	strictEqual(ref.getSync('etc').getSync('foo'), 1);
-	strictEqual(ref.getSync('etc').getSync('bar'), 2);
+	// Test Array (numeric indices)
+	const val = [ 1, 2, 3 ];
+	Object.defineProperty(val, 1, { get() { trap = true; return 'got' }});
+	const ref = new ivm.Reference(val);
+	strictEqual(ref.getSync(0), 1);
+	throws(() => ref.getSync(1));
 }
 
+{
+	// Test ArrayBuffer (numeric indices)
+	const val = new Uint8Array([ 1, 2, 3 ]);
+	const ref = new ivm.Reference(val);
+	strictEqual(ref.getSync(0), 1);
+}
+
+if (trap) {
+	console.log('fail');
+}
 console.log('pass');

Vulnerable code: - When possible, `reference.get()` will return a function proxy instead of a `Reference`.
  * `inheritUnsafe` *[boolean]* - If enabled then the `get` family of functions will follow the
* **return** `true` or `false`
* **return** `true` or `false`

Returns a boolean indicating whether or not this operation succeeded. I'm actually not really sure
when `false` would be returned, I'm just giving you the result back straight from the v8 API.
		constructor(value: T, options?: { inheritUnsafe?: boolean });
		 delete(property: keyof T): Promise<boolean>;
		 deleteSync(property: keyof T): boolean;
			property: Key, options?: Options): ResultTypeAsync<Options & AsReference, T[Key]>;
			property: Key, options?: Options): ResultTypeSync<Options & AsReference, T[Key]>;
		 *
		 * @return {boolean} Indicating whether or not this operation succeeded. I'm actually not really
		 * sure when false would be returned, I'm just giving you the result back straight from the v8
		 * API.
			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): Promise<boolean>;
			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): boolean;
		): ResultTypeBidirectionalAsync<Options & ApplyAsReference, ApplyResult<T>>;
			arguments?: ArgumentsTypeBidirectional<Options & ApplyAsReference, ApplyArguments<T>>,
		): ResultTypeBidirectionalSync<Options & ApplyAsReference, ApplyResult<T>>;
		): ResultTypeBidirectionalSync<Options & ApplyAsReference, ApplyResult<T>>;
		String inheritUnsafe{"inheritUnsafe"};
	auto inherit = ReadOption<bool>(options, StringTable::Get().inheritUnsafe, false);
 * Get a property from this reference, returned as another reference
class GetRunner : public ThreePhaseTask {
		GetRunner(
			const ReferenceHandle& that,
			Local<Value> key_handle,
			MaybeLocal<Object> maybe_options,
			bool inherit
		) :
				context{that.context},
				reference{that.reference},
				options{maybe_options, inherit ?
					TransferOptions::Type::DeepReference : TransferOptions::Type::Reference},
				inherit{inherit} {
			that.CheckDisposed();
			key = ExternalCopy::CopyIfPrimitive(key_handle);
			if (!key) {
			Local<Context> context_handle = Deref(context);
			Context::Scope context_scope{context_handle};
			Local<Value> key_inner = key->CopyInto();
			Local<Object> object = Local<Object>::Cast(Deref(reference));
			bool allow = [&]() {
				if (!inherit) {
					if (key_inner->IsName()) {
						return Unmaybe(object->HasRealNamedProperty(context_handle, key_inner.As<Name>()));
					} else if (key_inner->IsNumber()) {
						return Unmaybe(object->HasRealIndexedProperty(context_handle, HandleCast<uint32_t>(key_inner)));
					} else {
						return false;
					}
				return true;
			}();
			Local<Value> value = allow ?
				Unmaybe(object->Get(context_handle, key_inner)) :
				Undefined(Isolate::GetCurrent()).As<Value>();
			ret = TransferOut(value, options);
		unique_ptr<ExternalCopy> key;
		RemoteHandle<Context> context;
		RemoteHandle<Value> reference;
	return ThreePhaseTask::Run<async, GetRunner>(*isolate, *this, key_handle, maybe_options, inherit);
class DeleteRunner : public ThreePhaseTask {
				key{ExternalCopy::CopyIfPrimitive(key_handle)},
				context{that.context},
				reference{that.reference} {
			that.CheckDisposed();
			if (!key) {
				throw RuntimeTypeError("Invalid `key`");
			}
		}
			Local<Context> context_handle = Deref(context);
			Context::Scope context_scope{context_handle};
			Local<Object> object = Local<Object>::Cast(Deref(reference));
			result = Unmaybe(object->Delete(context_handle, key->CopyInto()));
			return Boolean::New(Isolate::GetCurrent(), result);

	private:
		unique_ptr<ExternalCopy> key;
		RemoteHandle<Context> context;
		RemoteHandle<Value> reference;
		bool result = false;
class SetRunner : public ThreePhaseTask {
				key{ExternalCopy::CopyIfPrimitive(key_handle)},
				val{TransferOut(val_handle, TransferOptions{maybe_options})},
				context{that.context},
				reference{that.reference} {
			that.CheckDisposed();
			if (!key) {
				throw RuntimeTypeError("Invalid `key`");
			}
		}
			Local<Context> context_handle = Deref(context);
			Context::Scope context_scope{context_handle};
			Local<Value> key_inner = key->CopyInto();
			Local<Object> object = Local<Object>::Cast(Deref(reference));
			Unmaybe(object->Delete(context_handle, key_inner));
			Local<Value> val_inner = val->TransferIn();
			did_set = Unmaybe(object->Set(context_handle, key_inner, val_inner));
			return Boolean::New(Isolate::GetCurrent(), did_set);
		unique_ptr<ExternalCopy> key;
		RemoteHandle<Context> context;
		RemoteHandle<Value> reference;
		bool did_set = false;
	friend class DeleteRunner;
	friend class SetRunner;
const { strictEqual } = require('assert');
const foo = { foo: 1 };
const bar = Object.create(foo);
bar.bar = 2;
const etc = Object.create(bar);
etc.etc = bar;
	const ref = new ivm.Reference(etc);
	strictEqual(ref.getSync('bar'), undefined);
	strictEqual(ref.getSync('etc').getSync('foo'), undefined);
	strictEqual(ref.getSync('etc').getSync('bar'), 2);
	const ref = new ivm.Reference(etc, { inheritUnsafe: true });
	strictEqual(ref.getSync('bar'), 2);
	strictEqual(ref.getSync('etc').getSync('foo'), 1);
	strictEqual(ref.getSync('etc').getSync('bar'), 2);
Fixed code: - When possible, `reference.get()` will return a function delegate instead of a `Reference`.
- Proxies and accessors are no longer tolerated via `reference.get`, and related functions.
  * `unsafeInherit` *[boolean]* - If enabled then the `get` family of functions will follow the
If the object is a proxy, or if the property is a getter, this method will throw.

		| Callback<any>
		| ((...args: any[]) => any)
		constructor(value: T, options?: { unsafeInherit?: boolean });
		 delete(property: keyof T): Promise<void>;
		 deleteSync(property: keyof T): void;
		 *
		 * If the object is a proxy, or if the property is a getter, this method will throw.
			property: Key, options?: Options): ResultTypeAsync<Options & FallbackReference, T[Key]>;
			property: Key, options?: Options): ResultTypeSync<Options & FallbackReference, T[Key]>;
			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): Promise<void>;
			property: Key, value: ArgumentType<Options, T[Key]>, options?: Options): void;
		): ResultTypeBidirectionalAsync<Options & FallbackReference, ApplyResult<T>>;
			arguments?: ArgumentsTypeBidirectional<Options & FallbackReference, ApplyArguments<T>>,
		): ResultTypeBidirectionalSync<Options & FallbackReference, ApplyResult<T>>;
		): ResultTypeBidirectionalSync<Options & FallbackReference, ApplyResult<T>>;
	type FallbackReference = { _reference: true };
		Options extends FallbackReference ? Reference<Result> :
		String unsafeInherit{"unsafeInherit"};
	auto inherit = ReadOption<bool>(options, StringTable::Get().unsafeInherit, false);
 * Base class for get, set, and delete runners
class AccessorRunner : public ThreePhaseTask {
		AccessorRunner(ReferenceHandle& target, Local<Value> key_handle) :
		context{target.context},
		target{target.reference},
		key{ExternalCopy::CopyIfPrimitive(key_handle)} {
			target.CheckDisposed();
			if (!key || (!key_handle->IsName() && !key_handle->IsUint32())) {
			} else if (target.type_of != decltype(target.type_of)::Object) {
				throw RuntimeTypeError("Reference is not an object");
			}
		}

	protected:
		auto GetTargetAndAlsoCheckForProxy() -> Local<Object> {
			auto object = Local<Object>::Cast(Deref(target));
			if (HasProxy(object)) {
				throw RuntimeTypeError("Object is or has proxy");
			}
			return object;
		}

		auto GetKey(Local<Context> context) -> Local<Name> {
			auto key_inner = key->CopyInto();
			return (key_inner->IsString() || key_inner->IsSymbol()) ?
				key_inner.As<Name>() : Unmaybe(key_inner->ToString(context)).As<Name>();
		}

		RemoteHandle<Context> context;

	private:
		static auto HasProxy(Local<Object> object) -> bool {
			if (object->IsProxy()) {
				return true;
			} else {
				auto proto = object->GetPrototype();
				if (proto->IsNullOrUndefined()) {
					return false;
				} else {
					return HasProxy(proto.As<Object>());
				}
		RemoteHandle<Value> target;
		unique_ptr<ExternalCopy> key;
};

/**
 * Get a property from this reference, returned as another reference
 */
class GetRunner final : public AccessorRunner {
	public:
		GetRunner(ReferenceHandle& target, Local<Value> key_handle, MaybeLocal<Object> maybe_options) :
		AccessorRunner{target, key_handle},
		options{maybe_options, target.inherit ?
			TransferOptions::Type::DeepReference : TransferOptions::Type::Reference},
		inherit{target.inherit} {}

			// Setup
			auto* isolate = Isolate::GetCurrent();
			auto context = Deref(this->context);
			Context::Scope context_scope{context};
			auto name = GetKey(context);
			auto object = GetTargetAndAlsoCheckForProxy();

			// Get property
			ret = TransferOut([&]() {
				if (inherit) {
					// To avoid accessors I guess we have to walk the prototype chain ourselves
					auto target = object;
					do {
						if (Unmaybe(target->HasOwnProperty(context, name))) {
							if (Unmaybe(target->HasRealNamedCallbackProperty(context, name))) {
								throw RuntimeTypeError("Property is getter");
							}
							return Unmaybe(target->GetRealNamedProperty(context, name));
						}
						auto next = target->GetPrototype();
						if (next->IsNullOrUndefined()) {
							return Undefined(isolate).As<Value>();
						}
						target = next.As<Object>();
					} while (true);
				} else if (!Unmaybe(object->HasOwnProperty(context, name))) {
					return Undefined(isolate).As<Value>();
				} else if (Unmaybe(object->HasRealNamedCallbackProperty(context, name))) {
					throw RuntimeTypeError("Property is getter");
				} else {
					return Unmaybe(object->Get(context, name));
			}(), options);
	return ThreePhaseTask::Run<async, GetRunner>(*isolate, *this, key_handle, maybe_options);
class DeleteRunner final : public AccessorRunner {
		AccessorRunner{that, key_handle} {}
			auto context = Deref(this->context);
			Context::Scope context_scope{context};
			auto object = GetTargetAndAlsoCheckForProxy();
			if (!Unmaybe(object->Delete(context, GetKey(context)))) {
				throw RuntimeTypeError("Delete failed");
			}
			return Undefined(Isolate::GetCurrent());
class SetRunner final : public AccessorRunner {
		AccessorRunner{that, key_handle},
		val{TransferOut(val_handle, TransferOptions{maybe_options})} {}
			auto context = Deref(this->context);
			Context::Scope context_scope{context};
			auto name = GetKey(context);
			auto object = GetTargetAndAlsoCheckForProxy();
			Unmaybe(object->Delete(context, name));
			auto val_inner = val->TransferIn();
			if (!Unmaybe(object->CreateDataProperty(context, GetKey(context), val_inner))) {
				throw RuntimeTypeError("Set failed");
			}
			return Undefined(Isolate::GetCurrent());
	friend class AccessorRunner;
	friend class AccessorRunner;
'use strict';
const { strictEqual, throws } = require('assert');
let trap = false;
	// Set up inheritance
	const foo = { foo: 1 };
	const bar = Object.create(foo);
	bar.bar = 2;
	const etc = Object.create(bar);
	etc.etc = bar;

	{
		// Test without inheritance
		const ref = new ivm.Reference(etc);
		strictEqual(ref.getSync('bar'), undefined);
		strictEqual(ref.getSync('etc').getSync('foo'), undefined);
		strictEqual(ref.getSync('etc').getSync('bar'), 2);
		ref.setSync('prop', 1);
		strictEqual(ref.getSync('prop'), 1);
		ref.deleteSync('prop');
		strictEqual(ref.getSync('prop'), undefined);
	}

	{
		// Test with inheritance
		const ref = new ivm.Reference(etc, { unsafeInherit: true });
		strictEqual(ref.getSync('bar'), 2);
		strictEqual(ref.getSync('etc').getSync('foo'), 1);
		strictEqual(ref.getSync('etc').getSync('bar'), 2);
		ref.getSync('etc').setSync('prop', 1);
		ref.setSync('prop', 2);
		strictEqual(ref.getSync('prop'), 2);
		ref.deleteSync('prop');
		strictEqual(ref.getSync('prop'), 1);
	}
}

{
	// Set up getter / setters
	let setter = 0;
	const foo = {
		get str() { trap = true; return 'got' },
		set str(_) { trap = true },
	};
	Object.defineProperty(foo, 0, {
		get() { trap = true; return 'got' },
		set() { trap = true },
	});

	{
		// Test plain accessors
		const ref = new ivm.Reference(foo);
		throws(() => ref.getSync('str'));
		throws(() => ref.getSync(0));
		throws(() => ref.setSync(0, undefined));
	}

	{
		// Test accessors + inheritance
		const ref = new ivm.Reference(Object.create(foo), { unsafeInherit: true });
		throws(() => ref.getSync('str'));
		ref.setSync('str', undefined);
		throws(() => ref.getSync(0));
	}
}

{
	// Set up evil proxy
	const val = { prop: 1 };
	const prox = new Proxy(val, {
		get() { trap = true },
    set() { trap = true },
    getOwnPropertyDescriptor() { trap = true },
    getPrototypeOf() { trap = true },
    has() { trap = true },
    ownKeys() { trap = true },
	});
	const inherited = Object.create(prox);

	{
		// Test proxy
		const ref = new ivm.Reference(prox);
		throws(() => ref.getSync('prop'));
	}

	{
		// Test inherited proxy
		const ref = new ivm.Reference(inherited);
		throws(() => ref.getSync('prop'),);
	}

	{
		// Test deep inherited proxy
		const ref = new ivm.Reference(inherited, { unsafeInherit: true });
		throws(() => ref.getSync('prop'));
	}

	// Test Array (numeric indices)
	const val = [ 1, 2, 3 ];
	Object.defineProperty(val, 1, { get() { trap = true; return 'got' }});
	const ref = new ivm.Reference(val);
	strictEqual(ref.getSync(0), 1);
	throws(() => ref.getSync(1));
{
	// Test ArrayBuffer (numeric indices)
	const val = new Uint8Array([ 1, 2, 3 ]);
	const ref = new ivm.Reference(val);
	strictEqual(ref.getSync(0), 1);
}

if (trap) {
	console.log('fail');
}