From d13531422c536f9fdf00386880298501ce090429 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Fri, 13 Sep 2024 16:47:41 -0700 Subject: [PATCH] fix(compartment-mapper): disallow dynamic requires of arbitrary, unknown paths This changes the logic so that this particular class of dynamic requires is disallowed. If the specifier is not accessible per policy, the usual policy-enforcement error message applies. If the specifier is not accessible because it's nowhere in the exit module, the error will mention that it can't load an unknown module. Also fixes: - dynamic requires of exit modules (e.g., Node.js builtins) - dynamic requires made from the entry compartment descriptor - some peer review comments --- .../compartment-mapper/src/import-hook.js | 163 ++++++++++-------- packages/compartment-mapper/src/link.js | 103 +++++------ packages/compartment-mapper/src/policy.js | 3 +- packages/compartment-mapper/src/types.js | 19 +- .../test/dynamic-require.test.js | 59 ++----- .../node_modules/badsprunt/README.md | 2 +- .../node_modules/badsprunt/package.json | 3 +- .../node_modules/hooked-app/index.js | 6 +- 8 files changed, 186 insertions(+), 172 deletions(-) diff --git a/packages/compartment-mapper/src/import-hook.js b/packages/compartment-mapper/src/import-hook.js index f67577d6c6..c76c8baa28 100644 --- a/packages/compartment-mapper/src/import-hook.js +++ b/packages/compartment-mapper/src/import-hook.js @@ -7,27 +7,33 @@ */ // @ts-check - -/** @import {ImportHook} from 'ses' */ -/** @import {ImportNowHook} from 'ses' */ -/** @import {StaticModuleType} from 'ses' */ -/** @import {RedirectStaticModuleInterface} from 'ses' */ -/** @import {CreateStaticModuleTypeOperators} from './types.js' */ -/** @import {CreateStaticModuleTypeOptions} from './types.js' */ -/** @import {CreateStaticModuleTypeYieldables} from './types.js' */ -/** @import {ParseResult} from './types.js' */ -/** @import {MakeImportNowHookMakerOptions} from './types.js' */ -/** @import {ModuleDescriptor} from './types.js' */ -/** @import {ReadFn} from './types.js' */ -/** @import {SyncReadPowers} from './types.js' */ -/** @import {ReadPowers} from './types.js' */ -/** @import {HashFn} from './types.js' */ -/** @import {Sources} from './types.js' */ -/** @import {CompartmentDescriptor} from './types.js' */ -/** @import {ImportHookMaker} from './types.js' */ -/** @import {ExitModuleImportHook} from './types.js' */ -/** @import {SourceMapHook} from './types.js' */ -/** @import {ImportNowHookMaker} from './types.js' */ +/** + * @import { + * ImportHook, + * ImportNowHook, + * RedirectStaticModuleInterface, + * StaticModuleType + * } from 'ses' + * @import { + * CompartmentDescriptor, + * CreateStaticModuleTypeOperators, + * CreateStaticModuleTypeOptions, + * CreateStaticModuleTypeYieldables, + * ExitModuleImportHook, + * FindRedirectParams, + * HashFn, + * ImportHookMaker, + * ImportNowHookMaker, + * MakeImportNowHookMakerOptions, + * ModuleDescriptor, + * ParseResult, + * ReadFn, + * ReadPowers, + * SourceMapHook, + * Sources, + * SyncReadPowers + * } from './types.js' + */ import { asyncTrampoline, syncTrampoline } from '@endo/trampoline'; import { resolve } from './node-module-specifier.js'; @@ -85,6 +91,63 @@ const nodejsConventionSearchSuffixes = [ '/index.node', ]; +/** + * Given a module specifier which is an absolute path, attempt to match it with + * an existing compartment; return a {@link RedirectStaticModuleInterface} if found. + * + * @throws If we determine `absoluteModuleSpecifier` is unknown + * @param {FindRedirectParams} params Parameters + * @returns {RedirectStaticModuleInterface|undefined} A redirect or nothing + */ +const findRedirect = ({ + compartmentDescriptor, + compartmentDescriptors, + compartments, + absoluteModuleSpecifier, + packageLocation, +}) => { + let specifierIsArbitrary = true; + for (const [someDescriptorName, someDescriptor] of entries( + compartmentDescriptors, + )) { + if (someDescriptorName !== ATTENUATORS_COMPARTMENT) { + const moduleSpecifierUrl = `${new URL(absoluteModuleSpecifier, packageLocation)}`; + if (!compartmentDescriptor.location.startsWith(moduleSpecifierUrl)) { + if (moduleSpecifierUrl.startsWith(someDescriptor.location)) { + // compartmentDescriptor is a dependency of someDescriptor; we + // can use that compartment + if (someDescriptor.name in compartmentDescriptor.modules) { + return { + specifier: absoluteModuleSpecifier, + compartment: compartments[someDescriptorName], + }; + } + // compartmentDescriptor is a dependent of someSomeDescriptor; + // this is more dubious but a common pattern + if (compartmentDescriptor.name in someDescriptor.modules) { + return { + specifier: absoluteModuleSpecifier, + compartment: compartments[someDescriptorName], + }; + } + if (compartmentDescriptor === someDescriptor) { + // this compartmentDescriptor wants to dynamically load its own module + // using an absolute path + specifierIsArbitrary = false; + } + } + } + } + } + + if (specifierIsArbitrary) { + throw new Error( + `Could not import unknown module: ${q(absoluteModuleSpecifier)}`, + ); + } + return undefined; +}; + /** * @param {object} params * @param {Record=} params.modules @@ -542,18 +605,11 @@ export function makeImportNowHookMaker( const makeImportNowHook = ({ packageLocation, packageName: _packageName, - entryCompartmentDescriptor, parse, compartments, }) => { const compartmentDescriptor = compartmentDescriptors[packageLocation] || {}; - // this is necessary, because there's nothing that prevents someone from calling this function with the entry compartment. - assert( - compartmentDescriptor !== entryCompartmentDescriptor, - 'Cannot create an ImportNowHook for the entry compartment', - ); - // this is not strictly necessary, since the types should handle it. assert( parse[Symbol.toStringTag] !== 'AsyncFunction', @@ -594,50 +650,15 @@ export function makeImportNowHookMaker( /** @type {ImportNowHook} */ const importNowHook = moduleSpecifier => { if (isAbsolute(moduleSpecifier)) { - let shouldCallExitModuleImportNowHook = true; - for (const [someDescriptorName, someDescriptor] of entries( + const record = findRedirect({ + compartmentDescriptor, compartmentDescriptors, - )) { - if (someDescriptorName !== ATTENUATORS_COMPARTMENT) { - const moduleSpecifierUrl = resolveLocation( - moduleSpecifier, - packageLocation, - ); - if ( - !compartmentDescriptor.location.startsWith(moduleSpecifierUrl) - ) { - if (moduleSpecifierUrl.startsWith(someDescriptor.location)) { - if (compartmentDescriptor.name in someDescriptor.modules) { - return { - specifier: moduleSpecifier, - compartment: compartments[someDescriptorName], - }; - } else if (compartmentDescriptor === someDescriptor) { - shouldCallExitModuleImportNowHook = false; - } - } - } - } - } - if (shouldCallExitModuleImportNowHook) { - if (exitModuleImportNowHook) { - // this hook is responsible for ensuring that the moduleSpecifier actually refers to an exit module - const record = exitModuleImportNowHook( - moduleSpecifier, - packageLocation, - ); - - if (!record) { - throw new Error(`Could not import module: ${q(moduleSpecifier)}`); - } - - return record; - } - throw new Error( - `Could not import module: ${q( - moduleSpecifier, - )}; try providing an importNowHook`, - ); + compartments, + absoluteModuleSpecifier: moduleSpecifier, + packageLocation, + }); + if (record) { + return record; } } diff --git a/packages/compartment-mapper/src/link.js b/packages/compartment-mapper/src/link.js index 92bc8b3438..79b8c0fdc4 100644 --- a/packages/compartment-mapper/src/link.js +++ b/packages/compartment-mapper/src/link.js @@ -8,22 +8,27 @@ // @ts-check -/** @import {ImportNowHook} from 'ses' */ -/** @import {ModuleMapHook} from 'ses' */ -/** @import {ImportNowHookMaker, ModuleTransform, ParseFnAsync, SyncModuleTransform} from './types.js' */ -/** @import {LinkResult} from './types.js' */ -/** @import {ParseFn} from './types.js' */ -/** @import {ParserForLanguage} from './types.js' */ -/** @import {SyncLinkOptions} from './types.js' */ -/** @import {SyncModuleTransforms} from './types.js' */ -/** @import {ParserImplementation} from './types.js' */ -/** @import {ShouldDeferError} from './types.js' */ -/** @import {ModuleTransforms} from './types.js' */ -/** @import {LanguageForExtension} from './types.js' */ -/** @import {ModuleDescriptor} from './types.js' */ -/** @import {CompartmentDescriptor} from './types.js' */ -/** @import {CompartmentMapDescriptor} from './types.js' */ -/** @import {LinkOptions} from './types.js' */ +/** @import {ImportNowHook, ModuleMapHook} from 'ses' */ +/** + * @import { + * CompartmentDescriptor, + * CompartmentMapDescriptor, + * ImportNowHookMaker, + * ImportNowHookMakerParams, + * LanguageForExtension, + * LinkOptions, + * LinkResult, + * ModuleDescriptor, + * ModuleTransforms, + * ParseFn, + * ParseFnAsync, + * ParserForLanguage, + * ParserImplementation, + * ShouldDeferError, + * SyncLinkOptions, + * SyncModuleTransforms + * } from './types.js' + */ /** @import {ERef} from '@endo/eventual-send' */ import { mapParsers } from './map-parser.js'; @@ -297,23 +302,26 @@ export const link = ( Compartment = defaultCompartment, } = options; - const isSync = isSyncOptions(options); - const syncModuleTransforms = isSync - ? options.syncModuleTransforms - : undefined; - const moduleTransforms = isSync - ? undefined - : /** @type {ModuleTransforms|undefined} */ ({ - ...options.syncModuleTransforms, - ...options.moduleTransforms, - }); - - const makeImportNowHook = isSync ? options.makeImportNowHook : undefined; - ``; + /** @type {SyncModuleTransforms|undefined} */ + let syncModuleTransforms; + /** @type {ModuleTransforms|undefined} */ + let moduleTransforms; + /** @type {ImportNowHookMaker|undefined} */ + let makeImportNowHook; + + if (isSyncOptions(options)) { + syncModuleTransforms = options.syncModuleTransforms; + makeImportNowHook = options.makeImportNowHook; + } else { + // we can fold syncModuleTransforms into moduleTransforms because + // async supports sync, but not vice-versa + moduleTransforms = /** @type {ModuleTransforms|undefined} */ ({ + ...options.syncModuleTransforms, + ...options.moduleTransforms, + }); + } const { compartment: entryCompartmentName } = entry; - const entryCompartmentDescriptor = - compartmentDescriptors[entryCompartmentName]; /** @type {Record} */ const compartments = create(null); @@ -408,24 +416,21 @@ export const link = ( /** @type {ImportNowHook | undefined} */ let importNowHook; - if (isSync) { - if (entryCompartmentDescriptor !== compartmentDescriptor) { - const syncParse = /** @type {ParseFn} */ (parse); - importNowHook = /** @type {ImportNowHookMaker} */ (makeImportNowHook)({ - entryCompartmentDescriptor, - packageLocation: location, - packageName: name, - parse: syncParse, - compartments, - }); - } else { - // should never happen - importNowHook = () => { - throw new Error( - 'importNowHook should not be called by entry compartment', - ); - }; - } + if (makeImportNowHook) { + // Note: using the LHS of this statement as the value for `params.parse` + // below causes a `no-object-shorthand` error. afaict there is no such + // configuration for this rule which would allow the below type assertion + // to be used. + const syncParse = /** @type {ParseFn} */ (parse); + + /** @type {ImportNowHookMakerParams} */ + const params = { + packageLocation: location, + packageName: name, + parse: syncParse, + compartments, + }; + importNowHook = makeImportNowHook(params); } else { importNowHook = () => { throw new Error('Provided read powers do not support dynamic requires'); diff --git a/packages/compartment-mapper/src/policy.js b/packages/compartment-mapper/src/policy.js index a5463ff205..29401c5ede 100644 --- a/packages/compartment-mapper/src/policy.js +++ b/packages/compartment-mapper/src/policy.js @@ -3,7 +3,7 @@ // @ts-check -/** @import {Policy} from './types.js' */ +/** @import {FindRedirectParams, Policy} from './types.js' */ /** @import {PackagePolicy} from './types.js' */ /** @import {AttenuationDefinition} from './types.js' */ /** @import {PackageNamingKit} from './types.js' */ @@ -11,6 +11,7 @@ /** @import {CompartmentDescriptor} from './types.js' */ /** @import {Attenuator} from './types.js' */ /** @import {SomePolicy} from './types.js' */ +/** @import {RedirectStaticModuleInterface} from 'ses' */ import { getAttenuatorFromDefinition, diff --git a/packages/compartment-mapper/src/types.js b/packages/compartment-mapper/src/types.js index 9d3db7f282..aedfc4566d 100644 --- a/packages/compartment-mapper/src/types.js +++ b/packages/compartment-mapper/src/types.js @@ -299,7 +299,6 @@ export {}; /** * @typedef {object} ImportNowHookMakerParams - * @property {CompartmentDescriptor} entryCompartmentDescriptor * @property {string} packageLocation * @property {string} packageName * @property {ParseFn} parse @@ -885,10 +884,9 @@ export {}; * @property {CompartmentSources} packageSources Sources * @property {ReadPowers|ReadFn} readPowers Powers * @property {SourceMapHook} [sourceMapHook] Source map hook - * @property {(compartmentName: string) => Set} - * strictlyRequiredForCompartment Function returning a set of module names - * (scoped to the compartment) whose parser is not using heuristics to determine - * imports. + * @property {(compartmentName: string) => Set} strictlyRequiredForCompartment Function + * returning a set of module names (scoped to the compartment) whose parser is not using + * heuristics to determine imports. */ /** @@ -932,3 +930,14 @@ export {}; * ReturnType} * CreateStaticModuleTypeYieldables */ + +/** + * Parameters for `findRedirect()`. + * + * @typedef FindRedirectParams + * @property {CompartmentDescriptor} compartmentDescriptor + * @property {Record} compartmentDescriptors + * @property {Record} compartments + * @property {string} absoluteModuleSpecifier A module specifier which is an absolute path + * @property {string} packageLocation + */ diff --git a/packages/compartment-mapper/test/dynamic-require.test.js b/packages/compartment-mapper/test/dynamic-require.test.js index 39d8f5fdee..a1a5df07fb 100644 --- a/packages/compartment-mapper/test/dynamic-require.test.js +++ b/packages/compartment-mapper/test/dynamic-require.test.js @@ -151,15 +151,13 @@ test('intra-package dynamic require with inter-package absolute path works witho t.is(importNowHookCallCount, 0); }); -test('intra-package dynamic require with arbitrary absolute path works when invoking the exitModuleImportNowHook', async t => { - t.plan(2); +test('intra-package dynamic require with arbitrary absolute path fails', async t => { const fixture = new URL( 'fixtures-dynamic/node_modules/broken-app/index.js', import.meta.url, ).toString(); - let importNowHookCallCount = 0; + /** @type {ExitModuleImportNowHook} */ const importNowHook = (specifier, packageLocation) => { - importNowHookCallCount += 1; const require = Module.createRequire( readPowers.fileURLToPath(packageLocation), ); @@ -190,48 +188,13 @@ test('intra-package dynamic require with arbitrary absolute path works when invo }, }; - const { namespace } = await importLocation(readPowers, fixture, { - policy, - importNowHook, - }); - - t.deepEqual( - { - default: { - isOk: 1, - }, - isOk: 1, - }, - { ...namespace }, - ); - t.is(importNowHookCallCount, 1); -}); - -test('intra-package dynamic require with arbitrary absolute does not path works when no exitModuleImportNowHook provided', async t => { - const fixture = new URL( - 'fixtures-dynamic/node_modules/broken-app/index.js', - import.meta.url, - ).toString(); - /** @type {Policy} */ - const policy = { - entry: { - packages: 'any', - }, - resources: { - badsprunt: { - packages: { - 'node-tammy-build': true, - }, - }, - }, - }; - await t.throwsAsync( importLocation(readPowers, fixture, { policy, + importNowHook, }), { - message: /try providing an importNowHook/, + message: /Could not import unknown module/, }, ); }); @@ -297,8 +260,8 @@ test('dynamic require fails without isAbsolute & fileURLToPath in read powers', ); }); -test('inter-pkg and exit module dynamic require works', async t => { - t.plan(2); +test('inter-package and exit module dynamic require works', async t => { + t.plan(3); const fixture = new URL( 'fixtures-dynamic/node_modules/hooked-app/index.js', @@ -307,10 +270,13 @@ test('inter-pkg and exit module dynamic require works', async t => { // number of times the `importNowHook` got called let importNowHookCallCount = 0; + /** @type {string[]} */ + const importNowHookSpecifiers = []; /** @type {ExitModuleImportNowHook} */ const importNowHook = (specifier, packageLocation) => { importNowHookCallCount += 1; + importNowHookSpecifiers.push(specifier); const require = Module.createRequire( readPowers.fileURLToPath(packageLocation), ); @@ -343,6 +309,11 @@ test('inter-pkg and exit module dynamic require works', async t => { cluster: true, }, }, + 'hooked>dynamic': { + packages: { + 'is-ok': true, + }, + }, }, }, }); @@ -356,7 +327,9 @@ test('inter-pkg and exit module dynamic require works', async t => { }, { ...namespace }, ); + t.is(importNowHookCallCount, 1); + t.deepEqual(importNowHookSpecifiers, ['cluster']); }); test('sync module transforms work with dynamic require support', async t => { diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/README.md b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/README.md index 626281f97c..4865a0b086 100644 --- a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/README.md +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/README.md @@ -1 +1 @@ -This package provides `node-tammy-build` an absolute path to a file that is _not_ in its compartment map. `node-tammy-build` will happily `require()` that path. This should invoke the "exit module import now hook". \ No newline at end of file +This package calls the function returned by `node-tammy-build` with an absolute path to a file that is _not_ in its compartment map. This should fail. \ No newline at end of file diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/package.json b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/package.json index 5c7825cb24..cab79816cf 100644 --- a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/package.json +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/badsprunt/package.json @@ -5,7 +5,8 @@ "preinstall": "echo DO NOT INSTALL TEST FIXTURES; exit -1" }, "dependencies": { - "node-tammy-build": "^1.0.0" + "node-tammy-build": "^1.0.0", + "sprunt": "^1.0.0" }, "main": "index.js" } diff --git a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js index a643fe8e0b..f64a83d77a 100644 --- a/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js +++ b/packages/compartment-mapper/test/fixtures-dynamic/node_modules/hooked-app/index.js @@ -1 +1,5 @@ -exports.isOk = require('hooked').isOk; \ No newline at end of file +exports.isOk = require('hooked').isOk; + +const builtin = 'cluster'; + +require(builtin); \ No newline at end of file