perf: shallow-scan in containsDangerousConstructor (#564) (#565)

The recursive own-property scan walked the entire reachable graph on
every bridge crossing. For host libraries returning objects with deep
cross-references (e.g. DOM wrappers with ownerDocument back-pointers),
this caused ~175x slowdown per the reproduction in #564.

Replace the recursive scan with a shallow scan of direct own-property
descriptors. Nested host objects are independently scanned when they
themselves cross the bridge, so the existing layered descriptor-extraction
attacks (covered by the depth-2 and depth-3 nested attack tests) remain
blocked at the layer where the Function constructor is exposed at depth 1.

Adds a regression test that builds a 1000-node cross-referenced graph and
asserts the bridge crossing completes under 2s; the unfixed code takes
~8s on the same machine.

Reported by @bglick. Fixes #564.

Author: patriksimek

docs/ATTACKS.md

@@ -1055,7 +1055,7 @@ When sandbox proxies are passed to host functions, they are unwrapped via `mappi
 
 ### Mitigation
 
-Objects containing dangerous constructors are proxied with `preventUnwrap` -- they are NOT registered in `mappingThisToOther`. When passed to host functions, they cannot be unwrapped; instead the bridge creates a double-proxy where all property access goes through sanitizing traps. The proxy's `get` trap returns `{}` for dangerous constructor values. Dangerous constructors are detected recursively at any nesting depth via `containsDangerousConstructor` with cycle detection (WeakMap).
+Objects containing dangerous constructors are proxied with `preventUnwrap` -- they are NOT registered in `mappingThisToOther`. When passed to host functions, they cannot be unwrapped; instead the bridge creates a double-proxy where all property access goes through sanitizing traps. The proxy's `get` trap returns `{}` for dangerous constructor values. `containsDangerousConstructor` performs a shallow scan of own property descriptors at each bridge crossing; nested host objects are scanned independently when they themselves cross the bridge, so layered descriptor-extraction attacks (`getOwnPropertyDescriptor` on `getOwnPropertyDescriptors` results, etc.) are caught at the layer where the Function constructor is exposed at depth 1.
 
 ### Detection Rules
 

lib/bridge.js

@@ -544,17 +544,30 @@ function createBridge(otherInit, registerProxy) {
 			(otherAsyncGeneratorFunctionCtor && value === otherAsyncGeneratorFunctionCtor); // AsyncGeneratorFunction is not available on Node < 10
 	}
 
-	// Check if an object contains a dangerous function constructor in ANY of its
-	// own property values (data or accessor), recursively at any nesting depth.
-	// Uses cycle detection and otherSafeGetOwnPropertyDescriptor (not otherReflectGet)
-	// to avoid triggering getters.
-	function containsDangerousConstructor(obj, visited) {
+	// Check if an object's own property descriptors contain a dangerous function
+	// constructor (data value or accessor get/set). This is a shallow check —
+	// it does NOT recurse into nested object values.
+	//
+	// Why shallow is sufficient:
+	// Each value crossing the bridge is wrapped in its own proxy, which triggers
+	// its own call to this function. A nested host object reachable as obj.x.y
+	// only becomes a separate bridge crossing when the sandbox accesses obj.x —
+	// at which point the inner object is shallow-checked. If the inner object's
+	// own descriptors hold a Function constructor, it gets flagged dangerous on
+	// its own crossing and cannot be unwrapped back to the host. The existing
+	// tests `getOwnPropertyDescriptor on getOwnPropertyDescriptors result` and
+	// `triple-nested getOwnPropertyDescriptors attack` verify this preserves
+	// protection against depth-2 and depth-3 chained-descriptor attacks.
+	//
+	// Why this avoids the perf regression in #564:
+	// The previous recursive implementation walked all reachable own properties
+	// on every bridge crossing. For host libraries returning objects with deep
+	// cross-references (e.g. DOM wrappers where every node points back to
+	// ownerDocument), a single crossing triggered a walk of the entire object
+	// graph — O(graph) per crossing instead of O(direct properties).
+	function containsDangerousConstructor(obj) {
 		if (obj === null || typeof obj !== 'object') return false;
 
-		if (!visited) visited = new ThisWeakMap();
-		if (thisReflectApply(thisWeakMapGet, visited, [obj])) return false;
-		thisReflectApply(thisWeakMapSet, visited, [obj, true]);
-
 		let keys;
 		try {
 			keys = otherReflectOwnKeys(obj);
@@ -573,11 +586,8 @@ function createBridge(otherInit, registerProxy) {
 
 			if (desc.get || desc.set) {
 				if (isDangerousFunctionConstructor(desc.get) || isDangerousFunctionConstructor(desc.set)) return true;
-			} else {
-				if (isDangerousFunctionConstructor(desc.value)) return true;
-				if (desc.value !== null && typeof desc.value === 'object') {
-					if (containsDangerousConstructor(desc.value, visited)) return true;
-				}
+			} else if (isDangerousFunctionConstructor(desc.value)) {
+				return true;
 			}
 		}
 		return false;

test/vm.js

@@ -408,6 +408,46 @@ describe('contextify', () => {
 		assert.doesNotThrow(() => vm.run('(o) => o.arguments')({arguments: 1}));
 	});
 
+	it('cross-referenced object graph (issue #564)', () => {
+		// Regression test for #564: when a host library returns objects whose
+		// own properties cross-reference each other (e.g. DOM nodes pointing
+		// back to ownerDocument), bridge crossings must not walk the entire
+		// reachable object graph. The recursive scan in containsDangerousConstructor
+		// caused ~175x slowdown for these workloads. This test builds a graph of
+		// 1000 nodes where every node references every other node, and verifies
+		// it crosses the bridge in a reasonable time. Reference timings:
+		// shallow scan (fixed): ~10ms, recursive scan (regression): ~8000ms.
+		const NODE_COUNT = 1000;
+		const nodes = [];
+		for (let i = 0; i < NODE_COUNT; i++) {
+			nodes.push({id: i, label: 'node-' + i, payload: {kind: 'data', index: i}});
+		}
+		// Cross-reference: every node sees every other node (worst case for
+		// recursive scanning, easy case for shallow scanning).
+		for (let i = 0; i < NODE_COUNT; i++) {
+			nodes[i].siblings = nodes;
+			nodes[i].self = nodes[i];
+			nodes[i].first = nodes[0];
+			nodes[i].last = nodes[NODE_COUNT - 1];
+		}
+		const sandbox = {graph: nodes};
+		const localVm = new VM({sandbox});
+		const start = Date.now();
+		// Touch every node so each one crosses the bridge at least once.
+		const visited = localVm.run(`
+			let count = 0;
+			for (const n of graph) {
+				if (n.label && n.payload.kind === 'data') count++;
+			}
+			count;
+		`);
+		const elapsed = Date.now() - start;
+		assert.strictEqual(visited, NODE_COUNT);
+		// Generous bound — on a developer machine this completes in <100ms;
+		// the unfixed bug would push it to several seconds for this graph.
+		assert.ok(elapsed < 2000, `bridge crossing too slow: ${elapsed}ms (regression of #564)`);
+	});
+
 	after(() => {
 		vm = null;
 	});