Skip to content

Commit 261da20

Browse files
patriksimekpatriksimek
committed
fix(GHSA-47x8-96vw-5wg6): structural identity collapse for host built-in intrinsics
The previous symbol-filter patch blocked the canonical RCE payload but left the underlying primitive intact — sandbox code could still walk host prototypes via `({}).__lookupGetter__('__proto__')` + `Buffer.apply` and obtain a *handle* on a host built-in constructor where `HObject !== sandbox Object`. Fix: at bridge init, pre-populate `mappingOtherToThis` (and its host-side mirror `mappingThisToOther`) with [hostIntrinsic, sandboxIntrinsic] for every well-known prototype + constructor (Object, Array, Number, String, Boolean, Date, RegExp, Map, WeakMap, Set, WeakSet, Promise, every error class). The cache check in `thisFromOtherWithFactory`, `thisFromOtherForThrow`, and `thisEnsureThis` short-circuits before any wrapping logic, so a host intrinsic crossing the bridge collapses to the sandbox-realm equivalent the moment it arrives. Function / AsyncFunction / GeneratorFunction / AsyncGeneratorFunction prototypes are deliberately NOT cached: leaving them wrapped means the proxy get trap continues to route `fp.constructor` reads through `isDangerousFunctionConstructor` → `emptyFrozenObject`, preserving the existing dangerous-constructor sentinel. See test/ghsa/GHSA-47x8-96vw-5wg6/structural-leak.js.
1 parent 7cc354c commit 261da20

5 files changed

Lines changed: 396 additions & 24 deletions

File tree

‎NOTES.md‎

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
# GHSA-47x8-96vw-5wg6 — Structural fix
2+
3+
## The invariant I closed
4+
5+
> Every well-known host built-in **prototype** and **constructor** must
6+
> be pre-mapped to its sandbox-realm equivalent in the bridge's identity
7+
> cache, so any code path that would otherwise surface a host intrinsic
8+
> into the sandbox returns the sandbox-realm intrinsic instead.
9+
10+
This is the right chokepoint because:
11+
12+
1. The bridge already maintains two paired weakmaps for identity:
13+
`mappingOtherToThis` (host → sandbox) and `mappingThisToOther`
14+
(sandbox → host). Every code path that converts a host value to a
15+
sandbox-side proxy consults `mappingOtherToThis` *before* any
16+
wrapping logic runs (the cache short-circuit at
17+
`lib/bridge.js:1600`). The same is true for `thisFromOtherForThrow`
18+
(line 1537) and `thisEnsureThis` (line 1499).
19+
2. Pre-populating those weakmaps at bridge init is a single, structural
20+
write — not a per-trap filter, not a per-call check. Once the cache
21+
is seeded, every lookup path benefits without any additional code.
22+
3. Adding the mappings symmetrically (host-side via `otherWeakMapSet`
23+
on `mappingThisToOther`) gives round-trip identity preservation, so
24+
sandbox values that flow to the host and back keep the same
25+
identity from the sandbox's view.
26+
27+
The previous symbol-filter patch (commit `67bc511`) closed the canonical
28+
RCE payload (extraction of `nodejs.util.inspect.custom`), but the
29+
underlying primitive — `HObject !== sandbox Object`, i.e., a *handle* on
30+
a host built-in — survived. Any future vulnerability that converts
31+
"I have a host built-in handle" into "I can call a host method that
32+
bypasses bridge sanitisation" would re-enable the same escape class.
33+
The structural fix removes that primitive: the sandbox can no longer
34+
*hold* a wrapped host built-in.
35+
36+
## Implementation
37+
38+
`lib/bridge.js` — added `thisAddIdentityMapping(thisProto, otherProto)`
39+
right after the existing `thisAddProtoMapping` calls (line ~1700).
40+
The helper:
41+
42+
1. Writes `[otherProto, thisProto]` to `mappingOtherToThis` (sandbox-side).
43+
2. Writes `[thisProto, otherProto]` to `mappingThisToOther` (host-side
44+
mirror, via `otherReflectApply(otherWeakMapSet, ...)`); failure here
45+
is best-effort because round-trip identity is a quality-of-life
46+
concern, not the security invariant.
47+
3. Reads the `.constructor` slot via `getOwnPropertyDescriptor` on both
48+
prototypes (so we never trigger a getter), guards against
49+
`isThisDangerousFunctionConstructor` and
50+
`isDangerousFunctionConstructor`, and writes the constructor
51+
identity mapping with the same dual-direction semantics.
52+
53+
Called for: `Object`, `Array`, every entry in `globalsList` *except*
54+
`Function`, every entry in `errorsList` (`RangeError`,
55+
`ReferenceError`, `SyntaxError`, `TypeError`, `EvalError`, `URIError`,
56+
`SuppressedError`, `Error`).
57+
58+
`Function` / `AsyncFunction` / `GeneratorFunction` /
59+
`AsyncGeneratorFunction` are deliberately skipped — see "Edge cases"
60+
below.
61+
62+
Every changed line is annotated with `// SECURITY (GHSA-47x8): ...`.
63+
64+
## Edge cases considered
65+
66+
### Function-family prototypes are NOT cached
67+
68+
If `host Function.prototype` were mapped to `sandbox Function.prototype`,
69+
then a sandbox proto-walk landing on the host `Function.prototype`
70+
would surface the sandbox prototype directly — and reading
71+
`.constructor` on a real sandbox prototype follows the prototype's own
72+
data slot, returning sandbox `Function`. Sandbox `Function` is callable
73+
(it creates sandbox-realm functions), which means the existing
74+
"`fp.constructor` returns `emptyFrozenObject`" defense
75+
(`isDangerousFunctionConstructor` in `thisFromOtherWithFactory`) would
76+
be silently bypassed for that path.
77+
78+
By leaving the four Function-family prototypes un-cached, the proxy
79+
`get` trap continues to handle `fp.constructor` reads: it reads
80+
`host Function.prototype.constructor` = host Function, and
81+
`thisFromOtherWithFactory` returns `emptyFrozenObject` because
82+
`isDangerousFunctionConstructor(hostFunction)` is true.
83+
84+
The structural-leak test "Function constructor block remains in force"
85+
exercises this exact invariant; the existing
86+
`getOwnPropertyDescriptor Function constructor bypass attack` regression
87+
in `test/vm.js:1279` exercises the descriptor variant.
88+
89+
### Promise mapping subtlety
90+
91+
`setup-sandbox.js` replaces sandbox `Promise` with
92+
`localPromise extends globalPromise` *after* `bridge.js` runs.
93+
At bridge-init time, `thisGlobalPrototypes.Promise` is the sandbox's
94+
**original** `globalPromise.prototype`, **not** `localPromise.prototype`.
95+
96+
Consequence: a host `Promise` flowing into the sandbox now collapses to
97+
`globalPromise` (sandbox-original), not to `localPromise`. From the
98+
sandbox's perspective, `hostPromise instanceof Promise` is false (this
99+
matches the pre-fix behaviour — verified by stash + re-test). The
100+
sandbox's runtime `Promise` is `localPromise` whose `.prototype.__proto__`
101+
*is* `globalPromise.prototype`, so the proto-chain still terminates at
102+
the original sandbox intrinsic, and bridge sanitisation is intact.
103+
Identity-equality with `Promise.prototype` is the only thing that
104+
changes, and that was already broken before this fix.
105+
106+
### Well-known symbols
107+
108+
V8's well-known symbols (`Symbol.iterator`, `Symbol.species`, etc.)
109+
are realm-shared by design — they are the same value across all realms
110+
in the same V8 isolate. The structural fix doesn't touch them; the
111+
existing dangerous-symbol filter in `isDangerousCrossRealmSymbol`
112+
(`Symbol.for('nodejs.util.inspect.custom')`,
113+
`Symbol.for('nodejs.rejection')`) is unchanged.
114+
115+
### Round-trip identity for sandbox values
116+
117+
The dual-direction mapping (writing both `mappingOtherToThis` and
118+
`mappingThisToOther`) means a sandbox-realm intrinsic flowing to the
119+
host now resolves to the host equivalent. This is consistent with
120+
how the bridge has always treated `Object.prototype.bind`,
121+
`__lookupGetter__` etc. via `connect()` in `setup-sandbox.js` — the
122+
existing `__lookupGetter__ / __lookupSetter__ attack` regression
123+
(`test/vm.js:839`) still passes:
124+
`Buffer.from.__lookupGetter__("__proto__") === Object.prototype.__lookupGetter__.call(Buffer.from, "__proto__")`
125+
remains `true`.
126+
127+
### Existing species-defense tests
128+
129+
The historical PoC for GHSA-grj5-jjm8-h35p (Array species self-return)
130+
used `op.constructor.entries({})` to mint a host array. After the
131+
structural fix, `op.constructor === sandbox Object`, so that chain
132+
returns a sandbox array. The species defense is still required for
133+
genuinely-host arrays (e.g., those exposed via sandbox config or
134+
`Buffer.from`), so I:
135+
136+
- updated `test/ghsa/GHSA-grj5-jjm8-h35p/repro.js` to mint a host array
137+
via `sandbox: { hostArrayFactory: () => [] }` (functions executing
138+
in the host frame still produce host-realm objects when called);
139+
- updated `test/vm.js`'s `neutralizeArraySpecies prevents species attack
140+
in apply trap` to use `Buffer.from([1,2,3]).slice(0)` (still a
141+
bridge-traversed host call where the apply trap fires).
142+
143+
The canonical PoC test for GHSA-grj5 still exercises the full
144+
Function-extraction pipeline and remains blocked. The species
145+
defense itself is unchanged.
146+
147+
## Variant attacks tried (all blocked)
148+
149+
I ran a 12-variant red-team probe (in `/tmp/redteam2.js` and
150+
`/tmp/redteam3.js`) to verify the structural fix closes the entire
151+
class:
152+
153+
| # | Variant | Outcome |
154+
|---|---------|---------|
155+
| 1 | `Buffer.apply.__proto__.__proto__` | `op === Object.prototype`, `op.constructor === Object` |
156+
| 2 | `Reflect.getPrototypeOf` chain | same |
157+
| 3 | `Object.getPrototypeOf` chain | same |
158+
| 4 | `Object.getOwnPropertyDescriptor(Object.prototype, '__proto__').get` walk | same |
159+
| 5 | sandbox-config-passed host array | `arr.constructor === sandbox Array` |
160+
| 6 | `Buffer.from(...)` proto-chain walk | terminates at sandbox `Object.prototype` |
161+
| 7 | `Object(42)` (Number wrapper) | sandbox `Number` identity |
162+
| 8 | host array `Symbol.iterator` | iterator API filtered (next is undefined) — no leak |
163+
| 9 | host TypeError caught in sandbox | sandbox `TypeError` identity, sandbox `instanceof` works |
164+
| 10 | host Promise via sandbox config | sandbox proto chain (`localPromise`/`globalPromise` subtlety, no leak) |
165+
| 11 | `Object.getPrototypeOf(Buffer.apply).constructor('return process')()` | `process is not defined` (sandbox Function is sandbox-safe) |
166+
| 12 | `getOwnPropertyDescriptor` on wrapped host `Function.prototype` | descriptor `.value` is undefined (filtered) |
167+
| w1 | `e.constructor.constructor('return process')()` | `process is not defined` |
168+
| w2 | `Buffer.from('hi')` proto walk to `.constructor.constructor` | sandbox Function (sandbox-safe) |
169+
| w11 | descriptor extraction on `fp` (uncached Function.prototype) | `.value` is undefined ✓ |
170+
171+
The only paths that "succeed" are sandbox-realm Function constructor
172+
calls — which are inherently safe because sandbox `Function` creates
173+
sandbox-realm functions where `process` is undefined, and this has
174+
always been the sandbox's contract.
175+
176+
## Second-order effects
177+
178+
- **Performance**: The fix runs at bridge init time only. The cache
179+
lookup at `thisFromOtherWithFactory` line ~1600 was already there;
180+
pre-populating it with ~20 extra entries is negligible. No runtime
181+
hot-path changes.
182+
- **Identity preservation**: For sandbox code that depended on
183+
`wrappedHostObject !== Object` to detect "this came from the host",
184+
that test is now unreliable. I am not aware of any consumer code
185+
that relied on this; the documented contract is that the bridge
186+
hides the host realm.
187+
- **Round-trip identity**: A sandbox `Object` that flows to the host
188+
and back now keeps its sandbox `Object` identity (instead of
189+
becoming a fresh wrapped host Object proxy on the way back). This
190+
is strictly better — it eliminates a class of identity-confusion
191+
bugs.
192+
- **`hostPromise instanceof Promise`**: This was already false before
193+
the fix because of `localPromise extends globalPromise`. No change.
194+
195+
## Files changed
196+
197+
- `lib/bridge.js` — `thisAddIdentityMapping` helper + invocation loop.
198+
- `docs/ATTACKS.md` — Category 8 mitigation paragraph + defense table
199+
row for "Host built-in identity leak".
200+
- `test/ghsa/GHSA-grj5-jjm8-h35p/repro.js` — switch host-array source
201+
from `ho.entries({})` to `hostArrayFactory()` (sandbox config).
202+
- `test/vm.js` — switch `neutralizeArraySpecies` test to use
203+
`Buffer.from([1,2,3]).slice(0)`.
204+
- `test/ghsa/GHSA-47x8-96vw-5wg6/structural-leak.js` — cherry-picked
205+
from base branch (was the failing test that drove this fix).
206+
207+
## Test results
208+
209+
- `test/ghsa/GHSA-47x8-96vw-5wg6/structural-leak.js`: 7/7 pass.
210+
- `test/vm.js`, `test/nodevm.js`, `test/compilers.js` (npm test):
211+
150 pass, 1 pending, 0 failing.
212+
- `test/ghsa/**/*.js`: 46/46 pass.

‎docs/ATTACKS.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,8 @@ Multi-layer defense. **Sandbox side** (`setup-sandbox.js`): overrides `Symbol.fo
694694
695695
**Bridge symbol-boundary filter (GHSA-47x8-96vw-5wg6, -qcp4-v2jj-fjx8, -f539-x546-3726)**: the `case 'symbol':` branches of the three primitive-value chokepoints `thisFromOtherWithFactory`, `thisEnsureThis`, and `thisFromOtherForThrow` now check `isDangerousCrossRealmSymbol(other)` when `!isHost` and return `undefined` for dangerous symbols. This is the single choke point through which every host-produced symbol primitive passes before reaching sandbox code — whether from a direct call result (apply trap), property read (get trap), iterator yield, descriptor getter, or throw value. Returning `undefined` (rather than a sandbox-local surrogate) is deliberate: `undefined` as a computed key coerces to the string `"undefined"`, so any subsequent attempt to register a handler via `obj[sym] = fn`, `{[sym]: fn}`, `Object.defineProperty(obj, sym, ...)`, or `Reflect.set(obj, sym, fn)` installs the key under the string `"undefined"` — which is never invoked by host internals as a custom-inspect/rejection protocol.
696696
697+
**Structural identity collapse for built-in intrinsics (GHSA-47x8-96vw-5wg6 follow-up)**: the symbol-filter patch above closed the canonical RCE payload but left the underlying primitive intact — sandbox code could still walk host prototypes (`({}).__lookupGetter__('__proto__').call(...)`, `Reflect.getPrototypeOf`, descriptor extraction of `Object.prototype.__proto__`, `Buffer.from(...)` proto chains, etc.) and surface a *reference* to host built-ins. Any later vulnerability that converts "I have a host built-in handle" into "I can read a host symbol / call a host method that bypasses bridge sanitisation" would re-enable the same escape class. Fix: at bridge init (`lib/bridge.js`'s `thisAddIdentityMapping`), pre-populate the `mappingOtherToThis` / `mappingThisToOther` weakmaps with `[hostIntrinsic, sandboxIntrinsic]` for every well-known prototype + constructor (`Object`, `Array`, `Number`, `String`, `Boolean`, `Date`, `RegExp`, `Map`, `WeakMap`, `Set`, `WeakSet`, `Promise`, every error class). The cache lookup in `thisFromOtherWithFactory` (line ~1600), `thisFromOtherForThrow`, and `thisEnsureThis` short-circuits *before* any wrapping logic, so a host intrinsic crossing the bridge is collapsed to the sandbox-realm equivalent the moment it arrives. The `Function`, `AsyncFunction`, `GeneratorFunction`, and `AsyncGeneratorFunction` prototypes are deliberately **NOT** cached: their `.constructor` is the dangerous-function sentinel surface, and leaving those prototypes wrapped means the proxy `get` trap continues to route `fp.constructor` reads through `isDangerousFunctionConstructor` → `emptyFrozenObject`. Same reasoning applies to `Function`-family constructors themselves, which are explicitly skipped inside `thisAddIdentityMapping`.
698+
697699
**Pre-wrap container scrub**: `apply` and `construct` traps invoke `stripDangerousSymbolsFromHostResult(ret)` on the raw host return value before wrapping. For host arrays, the scrub drops any element that is a dangerous symbol and compacts; for non-array host objects (such as the return value of `Object.getOwnPropertyDescriptors`), it deletes own-property slots keyed by the dangerous symbols. This closes iteration and descriptor-enumeration paths that would otherwise still see the dangerous symbol present on the host container.
698700
699701
**ownKeys trap rewrite**: iterates the raw host result via `otherReflectGet` rather than bridge-wrapping it, so dangerous symbols can be *dropped* (preserving the Proxy ownKeys invariant, which forbids `undefined` keys) rather than rewritten.
@@ -1494,6 +1496,7 @@ The most dangerous attacks combine multiple categories. Each pattern references
14941496
| Dynamic import | Throws `VMError` unconditionally |
14951497
| Prototype trap pollution | Handlers use null-prototype objects |
14961498
| Cross-realm symbols | Bridge proxy traps filter dangerous symbols; sandbox overrides reflection APIs |
1499+
| Host built-in identity leak | `thisAddIdentityMapping` pre-caches every well-known prototype + constructor in `mappingOtherToThis`/`mappingThisToOther`; cache check in `thisFromOtherWithFactory` short-circuits before wrapping. Function-family prototypes intentionally NOT cached so the dangerous-constructor sentinel still fires. |
14971500
| Proxy handler exposure | Closure-scoped WeakMap and conversion methods; `isThisDangerousFunctionConstructor` on `get` trap returns |
14981501
| Property descriptor extraction | `containsDangerousConstructor` + `preventUnwrap` blocks unwrapping |
14991502
| SuppressedError | `handleException` detects and recursively sanitizes `.error`/`.suppressed` |

0 commit comments

Comments
 (0)