Skip to content

Commit ab4ee7d

Browse files
fix(GHSA-c48m-32m9-vx93): reject '..' traversal in allowlisted subpaths
`LegacyResolver.customResolve` decided whether to consult a custom `require.resolve` by testing the bare specifier against `externalCache` regexes built with no anchors, so `require: { external: ['left-pad'] }` matched `evil-left-pad` / `left-pad-evil` / `xleft-padx` by substring containment. The resolver then located the colliding host package, its resolved path was appended to `this.externals`, and under the default `context: 'host'` its top-level code ran with full host authority from a sandbox configured with `builtin: []`. Anchoring alone was insufficient: the matcher must permit a subpath tail (`left-pad/utils`), and that tail accepts `..` segments, so `left-pad/../evil-package` and `left-pad/sub/../../evil-package` walked back out of the package boundary to the same effect. A regex lookahead cannot close this — it only inspects the segment after the first separator. Two composed layers in lib/resolver-compat.js: - The `externalCache` matcher is anchored `^(?:<pattern>)(?:[\\/].*)?$`, so a specifier must equal the allowlisted name or be a subpath under it. Wildcard segment semantics are preserved (`@scope/*` still matches `@scope/pkg` and `@scope/pkg/sub`, not `x@scope/pkg`). The separate filename-side `this.externals` matcher is untouched. - On the bare-specifier branch only, the specifier is split on `[\\/]` and rejected if any segment is exactly `..`, before the custom resolver is consulted. Rejection returns undefined, so the standard loader runs, the path never enters `this.externals`, and the sandbox observes an ordinary module-not-found. Ordering: the `..` check runs on the raw, un-canonicalized specifier and therefore before any realpath(). This is the only correct placement -- canonicalization removes `..` segments by definition, so the same lexical check after realpath() would be a no-op. It composes with rather than duplicates `CustomResolver.isPathAllowed`'s realpath dereferencing (GHSA-cp6g-6699-wx9c), which is a filename-space check against `require.root` symlinks; this one is a specifier-space check against lexical escape of the package name boundary. Neither subsumes the other and no new path into `isPathAllowed` is introduced. Restores the external-package allowlist boundary asserted by docs/ATTACKS.md Defense Invariant 13's sibling for external modules. Tests: test/ghsa/GHSA-c48m-32m9-vx93/repro.js -- substring collisions, `..` traversal at three depths plus bare `left-pad/..`, wildcard segment matching, and the legitimate name/subpath cases, using resolver consultation as the oracle. docs/ATTACKS.md: new Category 45 (module-resolution/path-traversal; no existing category covered the external-package allowlist -- Category 21 is the builtin allowlist and Category 24 is the `require.root` filename boundary, both cross-referenced), new Compound Attack Pattern 28, and a "How The Bridge Defends" row. No renumbering was required. package.json version unchanged.
1 parent d6ef73b commit ab4ee7d

4 files changed

Lines changed: 186 additions & 1 deletion

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
- **GHSA-6w8r-xxw2-g3hx** — `node:sqlite`'s `DatabaseSync(':memory:', { allowExtension: true }).loadExtension(path)` loaded a native SQLite extension into the host process — native RCE from a NodeVM allowing only that builtin. The exposed `DatabaseSync` now forces `allowExtension` off (for object- and function-typed options alike, since Node accepts a function there), so Node throws `ERR_INVALID_STATE` from both `loadExtension()` and `enableLoadExtension()` while ordinary SQL keeps working. `lib/setup-node-sandbox.js` additionally rejects repeated `node:` prefixes, closing the `require('node:node:sqlite')` second spelling and making the canonical `require('node:sqlite')` resolve. See ATTACKS.md Category 40 and `test/ghsa/GHSA-6w8r-xxw2-g3hx/`.
1313
- **GHSA-8686-vhfx-7r3j** — NodeVM's `builtin: ['*', '-node:child_process']` deny token was a silent no-op: the wildcard expansion matched deny tokens by exact string, so the `node:`-prefixed spelling never matched the canonical `child_process` name and the host module (RCE via `execSync`/`spawn`) stayed exposed. The `'*'` deny check in `lib/builtin.js` now matches both `-${name}` and `-node:${name}`, mirroring the `node:` normalization the resolver already applies on the require side. See ATTACKS.md Category 21 and `test/ghsa/GHSA-8686-vhfx-7r3j/`.
1414
- **GHSA-98xx-8mx4-x7cm** — `tls.setDefaultCACertificates()` let sandbox code replace the host thread's process-wide default CA trust store, so subsequent host TLS clients accepted attacker-signed certificates. Argument-side defenses were insufficient (the required host array is forgeable through `URLSearchParams.getAll()`), so the member itself is now neutralized in `lib/builtin.js`; the rest of `tls` is unaffected. See ATTACKS.md Category 40 and `test/ghsa/GHSA-98xx-8mx4-x7cm/`.
15+
- **GHSA-c48m-32m9-vx93** — NodeVM `require.external` allowlist bypass when a custom `require.resolve` is configured. The bare-specifier pre-check in `LegacyResolver.customResolve` matched by substring, so `external: ['left-pad']` also admitted `evil-left-pad` / `left-pad-evil`; anchoring that matcher then left a second route, since the permitted subpath tail accepted `..` segments (`left-pad/../evil-package`). Either way the resolver located an un-allowlisted host package and, under the default `context: 'host'`, ran its top-level code in host context. Two composed layers in `lib/resolver-compat.js`: the `externalCache` matcher is anchored to the whole specifier (wildcard segment semantics preserved), and any bare specifier carrying a `..` path segment is rejected before the custom resolver is consulted. See ATTACKS.md Category 45 and `test/ghsa/GHSA-c48m-32m9-vx93/`.
1516
- **GHSA-fcqc-726x-5wfc** — sandbox read/write of host-realm memory through Node's shared small-buffer pool. Node serves small `Buffer.from(...)` / `Buffer.concat(...)` / `Buffer.of(...)` allocations out of one shared 64 KiB backing `ArrayBuffer`, and a pooled buffer's `.buffer` getter exposed that whole pool — so `Buffer.from(Buffer.from([0]).buffer, 0, 65536)` inside the sandbox could disclose and corrupt any host buffer (secrets, tokens, DB rows) sharing it. `lib/setup-sandbox.js` now enforces a backing-store ownership invariant (`byteOffset === 0 && buffer.byteLength === length`): every pooling factory (`Buffer.from` non-ArrayBuffer overloads, `concat`, `of`, `copyBytesFrom`, and the deprecated `Buffer(...)` / `new Buffer(...)` forms) copies a pool-backed result into a standalone non-pooled buffer, while the documented `Buffer.from(arrayBuffer, byteOffset, length)` sharing overload is preserved via a spoof-proof brand test. Independent of `bufferAllocLimit`. See ATTACKS.md Category 41 and `test/ghsa/GHSA-fcqc-726x-5wfc/`.
1617
- **GHSA-h85j-hv3c-qfgq** — `http.globalAgent` / `https.globalAgent` handed the sandbox the real shared host singleton, so a `.on('free')` listener received live host request options (including `Authorization` headers) and released sockets from unrelated host requests. The sandbox now sees a dedicated `Agent`, and the module's `request()` / `get()` default to it so `req.agent` cannot re-expose the host singleton; a caller-supplied `agent` is preserved. See ATTACKS.md Category 40 and `test/ghsa/GHSA-h85j-hv3c-qfgq/`.
1718
- **GHSA-jf8q-945g-9q4c** — incomplete `nodejs.*` symbol filtering let sandbox code corrupt host-visible WebStream state. The dangerous cross-realm symbol checks were fixed lists that omitted `nodejs.stream.disturbed` / `nodejs.stream.errored`, so sandbox code could extract those real host symbols from a `ReadableStream.prototype` reachable through the sandbox and `defineProperty` them onto a host stream, flipping `stream.Readable.isDisturbed()` / `isErrored()` host-side on an already-consumed stream. `isDangerousSymbol` (`lib/setup-sandbox.js`) and `isDangerousCrossRealmSymbol` (`lib/bridge.js`) now flag any *registered* symbol whose `Symbol.keyFor` is in the reserved `nodejs.` namespace, covering the extraction filter, the `getOwnPropertyDescriptors` scrub and the `set` / `defineProperty` / `deleteProperty` write traps, so the guard no longer goes stale as Node adds `nodejs.*` symbols; well-known symbols and benign registered symbols still cross. See ATTACKS.md Category 8 (extended) and `test/ghsa/GHSA-jf8q-945g-9q4c/`.

0 commit comments

Comments
 (0)