From 7e2b07a466b6fafec5f6e39e92624c94aa9e177c Mon Sep 17 00:00:00 2001 From: LeoTM <1881059+leotm@users.noreply.github.com> Date: Wed, 19 Feb 2025 17:23:09 +0000 Subject: [PATCH] feat(ses): Hermes eval and compartment taming --- packages/ses/src/global-object.js | 6 ++- packages/ses/src/lockdown.js | 34 +++++++++++--- packages/ses/src/make-eval-function.js | 35 +++++++++++++-- packages/ses/src/make-function-constructor.js | 7 +-- packages/ses/src/permits.js | 44 +++++++++++++------ packages/ses/types.d.ts | 7 +++ 6 files changed, 104 insertions(+), 29 deletions(-) diff --git a/packages/ses/src/global-object.js b/packages/ses/src/global-object.js index 1b2c3c038d..9ffb409ed0 100644 --- a/packages/ses/src/global-object.js +++ b/packages/ses/src/global-object.js @@ -19,7 +19,7 @@ import { constantProperties, universalPropertyNames } from './permits.js'; * guest programs, we cannot emulate the proper behavior. * With this shim, assigning Symbol.unscopables causes the given lexical * names to fall through to the terminal scope proxy. - * But, we can install this setter to prevent a program from proceding on + * But, we can install this setter to prevent a program from proceeding on * this false assumption. * * @param {object} globalObject @@ -146,14 +146,16 @@ export const setGlobalObjectMutableProperties = ( * @param {object} globalObject * @param {Function} evaluator * @param {(object) => void} markVirtualizedNativeFunction + * @param {string} [legacyHermesTaming] */ export const setGlobalObjectEvaluators = ( globalObject, evaluator, markVirtualizedNativeFunction, + legacyHermesTaming, ) => { { - const f = freeze(makeEvalFunction(evaluator)); + const f = freeze(makeEvalFunction(evaluator, legacyHermesTaming)); markVirtualizedNativeFunction(f); defineProperty(globalObject, 'eval', { value: f, diff --git a/packages/ses/src/lockdown.js b/packages/ses/src/lockdown.js index d55e8b2ebf..78a88ff638 100644 --- a/packages/ses/src/lockdown.js +++ b/packages/ses/src/lockdown.js @@ -102,6 +102,7 @@ const safeHarden = makeHardener(); const assertDirectEvalAvailable = () => { let allowed = false; + let evaluatorsBlocked = false; try { allowed = FERAL_FUNCTION( 'eval', @@ -122,13 +123,18 @@ const assertDirectEvalAvailable = () => { // We reach here if eval is outright forbidden by a Content Security Policy. // We allow this for SES usage that delegates the responsibility to isolate // guest code to production code generation. - allowed = true; + evaluatorsBlocked = true; } - if (!allowed) { + if (!allowed && !evaluatorsBlocked) { // See https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_DIRECT_EVAL.md throw TypeError( `SES cannot initialize unless 'eval' is the original intrinsic 'eval', suitable for direct-eval (dynamically scoped eval) (SES_DIRECT_EVAL)`, ); + // TODO (hermes): (legacyHermesTaming === 'safe') + // throw TypeError( + // `SES cannot initialize unless 'eval' is the original intrinsic 'eval', suitable for direct-eval (dynamically scoped eval) (SES_DIRECT_EVAL) + // See: https://github.com/facebook/hermes/issues/957`, + // ); } }; @@ -152,11 +158,11 @@ export const repairIntrinsics = (options = {}) => { // The `stackFiltering` is not a safety issue. Rather it is a tradeoff // between relevance and completeness of the stack frames shown on the // console. Setting`stackFiltering` to `'verbose'` applies no filters, providing - // the raw stack frames that can be quite versbose. Setting + // the raw stack frames that can be quite verbose. Setting // `stackFrameFiltering` to`'concise'` limits the display to the stack frame // information most likely to be relevant, eliminating distracting frames // such as those from the infrastructure. However, the bug you're trying to - // track down might be in the infrastrure, in which case the `'verbose'` setting + // track down might be in the infrastructure, in which case the `'verbose'` setting // is useful. See // [`stackFiltering` options](https://github.com/Agoric/SES-shim/blob/master/packages/ses/docs/lockdown.md#stackfiltering-options) // for an explanation. @@ -189,6 +195,9 @@ export const repairIntrinsics = (options = {}) => { /** @param {string} debugName */ debugName => debugName !== '', ), + legacyHermesTaming = /** @type { 'safe' | 'unsafe' } */ ( + getenv('LOCKDOWN_LEGACY_HERMES_TAMING', 'safe') + ), legacyRegeneratorRuntimeTaming = getenv( 'LOCKDOWN_LEGACY_REGENERATOR_RUNTIME_TAMING', 'safe', @@ -199,6 +208,10 @@ export const repairIntrinsics = (options = {}) => { ...extraOptions } = options; + legacyHermesTaming === 'safe' || + legacyHermesTaming === 'unsafe' || + Fail`lockdown(): non supported option legacyHermesTaming: ${q(legacyHermesTaming)}`; + legacyRegeneratorRuntimeTaming === 'safe' || legacyRegeneratorRuntimeTaming === 'unsafe-ignore' || Fail`lockdown(): non supported option legacyRegeneratorRuntimeTaming: ${q(legacyRegeneratorRuntimeTaming)}`; @@ -218,13 +231,11 @@ export const repairIntrinsics = (options = {}) => { const { warn } = reporter; if (dateTaming !== undefined) { - // eslint-disable-next-line no-console warn( `SES The 'dateTaming' option is deprecated and does nothing. In the future specifying it will be an error.`, ); } if (mathTaming !== undefined) { - // eslint-disable-next-line no-console warn( `SES The 'mathTaming' option is deprecated and does nothing. In the future specifying it will be an error.`, ); @@ -242,7 +253,9 @@ export const repairIntrinsics = (options = {}) => { // trace retained: priorRepairIntrinsics.stack; - assertDirectEvalAvailable(); + if (legacyHermesTaming === 'safe') { + assertDirectEvalAvailable(); + } /** * Because of packagers and bundlers, etc, multiple invocations of lockdown @@ -408,6 +421,12 @@ export const repairIntrinsics = (options = {}) => { markVirtualizedNativeFunction, }); + if (legacyHermesTaming === 'unsafe') { + globalThis.testCompartmentHooks = undefined; + // @ts-ignore Compartment does exist on globalThis + delete globalThis.Compartment; + } + if (evalTaming === 'noEval') { setGlobalObjectEvaluators( globalThis, @@ -420,6 +439,7 @@ export const repairIntrinsics = (options = {}) => { globalThis, safeEvaluate, markVirtualizedNativeFunction, + legacyHermesTaming, ); } else if (evalTaming === 'unsafeEval') { // Leave eval function and Function constructor of the initial compartment in-tact. diff --git a/packages/ses/src/make-eval-function.js b/packages/ses/src/make-eval-function.js index 1c3e77600f..6f08e1afb2 100644 --- a/packages/ses/src/make-eval-function.js +++ b/packages/ses/src/make-eval-function.js @@ -1,11 +1,15 @@ +import { TypeError } from './commons.js'; + /** * makeEvalFunction() * A safe version of the native eval function which relies on - * the safety of safeEvaluate for confinement. + * the safety of safeEvaluate for confinement, unless noEval + * is specified (then a TypeError is thrown). * - * @param {Function} safeEvaluate + * @param {Function} evaluator + * @param legacyHermesTaming */ -export const makeEvalFunction = safeEvaluate => { +export const makeEvalFunction = (evaluator, legacyHermesTaming) => { // We use the concise method syntax to create an eval without a // [[Construct]] behavior (such that the invocation "new eval()" throws // TypeError: eval is not a constructor"), but which still accepts a @@ -19,7 +23,30 @@ export const makeEvalFunction = safeEvaluate => { // rule. Track. return source; } - return safeEvaluate(source); + if (legacyHermesTaming === 'unsafe') { + throw TypeError( + `Legacy Hermes unsupported eval() with strings arguments cannot be tamed safe under legacyHermesTaming ${legacyHermesTaming} + See: https://github.com/facebook/hermes/issues/1056 + See: https://github.com/endojs/endo/issues/1561 +Did you mean evalTaming: 'unsafeEval'?`, + ); + } + // refactoring to try/catch... + // - error output still 'Uncaught' + // - SES_NO_EVAL no longer encountered first + // try { + // safeEvaluate(source); + // } catch (e) { + // // throw Error(e); // Uncaught Error: SyntaxError: 2:5:invalid statement encountered. + // throw TypeError( + // `legacy Hermes unsupported eval() with strings arguments cannot be tamed safe under legacyHermesTaming ${legacyHermesTaming} + // see: https://github.com/facebook/hermes/issues/1056 + // see: https://github.com/endojs/endo/issues/1561 + // did you mean evalTaming: 'unsafeEval'?`, + // ); + // } + // Disabling safeEvaluate is not enough, since returning the source string is not evaluating it. + return evaluator(source); }, }.eval; diff --git a/packages/ses/src/make-function-constructor.js b/packages/ses/src/make-function-constructor.js index 47b86ef794..33b534308a 100644 --- a/packages/ses/src/make-function-constructor.js +++ b/packages/ses/src/make-function-constructor.js @@ -12,9 +12,10 @@ const { Fail } = assert; /* * makeFunctionConstructor() * A safe version of the native Function which relies on - * the safety of safeEvaluate for confinement. + * the safety of safeEvaluate for confinement, unless noEval + * is specified (then a TypeError is thrown). */ -export const makeFunctionConstructor = safeEvaluate => { +export const makeFunctionConstructor = evaluator => { // Define an unused parameter to ensure Function.length === 1 const newFunction = function Function(_body) { // Sanitize all parameters at the entry point. @@ -54,7 +55,7 @@ export const makeFunctionConstructor = safeEvaluate => { // TODO: since we create an anonymous function, the 'this' value // isn't bound to the global object as per specs, but set as undefined. const src = `(function anonymous(${parameters}\n) {\n${bodyText}\n})`; - return safeEvaluate(src); + return evaluator(src); }; defineProperties(newFunction, { diff --git a/packages/ses/src/permits.js b/packages/ses/src/permits.js index a2b08455c5..bad434cc69 100644 --- a/packages/ses/src/permits.js +++ b/packages/ses/src/permits.js @@ -1,7 +1,11 @@ /* eslint-disable no-restricted-globals */ /* eslint max-lines: 0 */ -import { arrayPush, arrayForEach } from './commons.js'; +import { + arrayPush, + arrayForEach, + getOwnPropertyDescriptor, +} from './commons.js'; /** @import {GenericErrorConstructor} from '../types.js' */ @@ -316,23 +320,37 @@ const accessor = { set: fn, }; +// TODO Remove this once we no longer support Hermes. +// While all engines have a ThrowTypeError accessor for fields not permitted in strict mode, +// some (Hermes 0.12) put that accessor in unexpected places. +// We can't clean them up because they're non-configurable. +// Therefore we're checking for identity with specCompliantThrowTypeError and dynamically adding permits for those. + // eslint-disable-next-line func-names -const strict = function () { +const specCompliantThrowTypeError = (function () { 'use strict'; -}; -// TODO Remove this once we no longer support the Hermes that needed this. -arrayForEach(['caller', 'arguments'], prop => { - try { - strict[prop]; - } catch (e) { - // https://github.com/facebook/hermes/blob/main/test/hermes/function-non-strict.js - if (e.message === 'Restricted in strict mode') { - // Fixed in Static Hermes: https://github.com/facebook/hermes/issues/1582 + // eslint-disable-next-line prefer-rest-params + const desc = getOwnPropertyDescriptor(arguments, 'callee'); + return desc && desc.get; +})(); +if (specCompliantThrowTypeError) { + // eslint-disable-next-line func-names + const strict = function () { + 'use strict'; + }; + arrayForEach(['caller', 'arguments'], prop => { + const desc = getOwnPropertyDescriptor(strict, prop); + if ( + desc && + desc.configurable === false && + desc.get && + desc.get === specCompliantThrowTypeError + ) { FunctionInstance[prop] = accessor; } - } -}); + }); +} export const isAccessorPermit = permit => { return permit === getter || permit === accessor; diff --git a/packages/ses/types.d.ts b/packages/ses/types.d.ts index 1fc86f12d0..170b47cee4 100644 --- a/packages/ses/types.d.ts +++ b/packages/ses/types.d.ts @@ -40,6 +40,13 @@ export interface RepairOptions { overrideTaming?: 'moderate' | 'min' | 'severe'; overrideDebug?: Array; domainTaming?: 'safe' | 'unsafe'; + /** + * safe (default): do nothing. + * + * unsafe: skips direct-eval check and compartment. + * + */ + legacyHermesTaming?: 'safe' | 'unsafe'; /** * safe (default): do nothing. *