From 5cbe7c2c8ca8108dcae5fcd75142f6ad26f014c9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 18:33:38 +0100 Subject: [PATCH] process: make source map getter resistant against prototype tampering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since this code runs during process and Worker shutdown, it should not call user-provided code and thereby e.g. provide a way to break out of `worker.terminate()`. PR-URL: https://github.com/nodejs/node/pull/30228 Reviewed-By: Joyee Cheung Reviewed-By: Ben Coe Reviewed-By: Michaël Zasso Reviewed-By: James M Snell --- lib/internal/source_map/source_map_cache.js | 63 +++++++++++++++---- .../test-worker-terminate-source-map.js | 40 ++++++++++++ 2 files changed, 91 insertions(+), 12 deletions(-) create mode 100644 test/parallel/test-worker-terminate-source-map.js diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js index 0e77203e9d6f1d..340615eb6cffcd 100644 --- a/lib/internal/source_map/source_map_cache.js +++ b/lib/internal/source_map/source_map_cache.js @@ -1,5 +1,28 @@ 'use strict'; +const { + JSON, + Object: { + create: ObjectCreate, + keys: ObjectKeys, + getOwnPropertyDescriptor: ObjectGetOwnPropertyDescriptor, + }, + ObjectPrototype: { + hasOwnProperty: ObjectHasOwnProperty + }, + MapPrototype: { + entries: MapEntries + }, uncurryThis +} = primordials; + +const MapIteratorNext = uncurryThis(MapEntries(new Map()).next); +const WeakMapGet = uncurryThis(WeakMap.prototype.get); + +function ObjectGetValueSafe(obj, key) { + const desc = ObjectGetOwnPropertyDescriptor(obj, key); + return ObjectHasOwnProperty(desc, 'value') ? desc.value : undefined; +} + // See https://sourcemaps.info/spec.html for SourceMap V3 specification. const { Buffer } = require('buffer'); const debug = require('internal/util/debuglog').debuglog('source_map'); @@ -9,7 +32,6 @@ const { getOptionValue } = require('internal/options'); const { normalizeReferrerURL, } = require('internal/modules/cjs/helpers'); -const { JSON, Object } = primordials; // For cjs, since Module._cache is exposed to users, we use a WeakMap // keyed on module, facilitating garbage collection. const cjsSourceMapCache = new WeakMap(); @@ -17,6 +39,7 @@ const cjsSourceMapCache = new WeakMap(); // on filenames. const esmSourceMapCache = new Map(); const { fileURLToPath, URL } = require('url'); +let Module; let experimentalSourceMaps; function maybeCacheSourceMap(filename, content, cjsModuleInstance) { @@ -40,6 +63,7 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) { const data = dataFromUrl(basePath, match.groups.sourceMappingURL); const url = data ? null : match.groups.sourceMappingURL; if (cjsModuleInstance) { + if (!Module) Module = require('internal/modules/cjs/loader').Module; cjsSourceMapCache.set(cjsModuleInstance, { filename, lineLengths: lineLengths(content), @@ -148,17 +172,27 @@ function rekeySourceMap(cjsModuleInstance, newInstance) { } } +// WARNING: The `sourceMapCacheToObject` and `appendCJSCache` run during +// shutdown. In particular, they also run when Workers are terminated, making +// it important that they do not call out to any user-provided code, including +// built-in prototypes that might have been tampered with. + // Get serialized representation of source-map cache, this is used // to persist a cache of source-maps to disk when NODE_V8_COVERAGE is enabled. function sourceMapCacheToObject() { - const obj = Object.create(null); + const obj = ObjectCreate(null); - for (const [k, v] of esmSourceMapCache) { + const it = MapEntries(esmSourceMapCache); + let entry; + while (!(entry = MapIteratorNext(it)).done) { + const k = entry.value[0]; + const v = entry.value[1]; obj[k] = v; } + appendCJSCache(obj); - if (Object.keys(obj).length === 0) { + if (ObjectKeys(obj).length === 0) { return undefined; } else { return obj; @@ -171,23 +205,28 @@ function sourceMapCacheToObject() { // TODO(bcoe): this means we don't currently serialize source-maps attached // to error instances, only module instances. function appendCJSCache(obj) { - const { Module } = require('internal/modules/cjs/loader'); - Object.keys(Module._cache).forEach((key) => { - const value = cjsSourceMapCache.get(Module._cache[key]); + if (!Module) return; + const cjsModuleCache = ObjectGetValueSafe(Module, '_cache'); + const cjsModules = ObjectKeys(cjsModuleCache); + for (let i = 0; i < cjsModules.length; i++) { + const key = cjsModules[i]; + const module = ObjectGetValueSafe(cjsModuleCache, key); + const value = WeakMapGet(cjsSourceMapCache, module); if (value) { + // This is okay because `obj` has a null prototype. obj[`file://${key}`] = { - lineLengths: value.lineLengths, - data: value.data, - url: value.url + lineLengths: ObjectGetValueSafe(value, 'lineLengths'), + data: ObjectGetValueSafe(value, 'data'), + url: ObjectGetValueSafe(value, 'url') }; } - }); + } } // Attempt to lookup a source map, which is either attached to a file URI, or // keyed on an error instance. function findSourceMap(uri, error) { - const { Module } = require('internal/modules/cjs/loader'); + if (!Module) Module = require('internal/modules/cjs/loader').Module; let sourceMap = cjsSourceMapCache.get(Module._cache[uri]); if (!uri.startsWith('file://')) uri = normalizeReferrerURL(uri); if (sourceMap === undefined) { diff --git a/test/parallel/test-worker-terminate-source-map.js b/test/parallel/test-worker-terminate-source-map.js new file mode 100644 index 00000000000000..d88a4c68366865 --- /dev/null +++ b/test/parallel/test-worker-terminate-source-map.js @@ -0,0 +1,40 @@ +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); + +// Attempts to test that the source map JS code run on process shutdown +// does not call any user-defined JS code. + +const { Worker, workerData, parentPort } = require('worker_threads'); + +if (!workerData) { + tmpdir.refresh(); + process.env.NODE_V8_COVERAGE = tmpdir.path; + + // Count the number of some calls that should not be made. + const callCount = new Int32Array(new SharedArrayBuffer(4)); + const w = new Worker(__filename, { workerData: { callCount } }); + w.on('message', common.mustCall(() => w.terminate())); + w.on('exit', common.mustCall(() => { + assert.strictEqual(callCount[0], 0); + })); + return; +} + +const { callCount } = workerData; + +function increaseCallCount() { callCount[0]++; } + +// Increase the call count when a forbidden method is called. +Object.getPrototypeOf((new Map()).entries()).next = increaseCallCount; +Map.prototype.entries = increaseCallCount; +Object.keys = increaseCallCount; +Object.create = increaseCallCount; +Object.hasOwnProperty = increaseCallCount; +Object.defineProperty(Object.prototype, 'value', { + get: increaseCallCount, + set: increaseCallCount +}); + +parentPort.postMessage('done');