From d66db7a67fab6bdd36d18a931c6a9163e842c3fe Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Wed, 19 Oct 2022 17:18:29 -0700 Subject: [PATCH 01/19] fix(ses): Typo in compartmentEvaluate --- packages/ses/src/compartment-evaluate.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ses/src/compartment-evaluate.js b/packages/ses/src/compartment-evaluate.js index e766c9347c..895a57fd02 100644 --- a/packages/ses/src/compartment-evaluate.js +++ b/packages/ses/src/compartment-evaluate.js @@ -66,7 +66,7 @@ export const provideCompartmentEvaluator = (compartmentFields, options) => { }; export const compartmentEvaluate = (compartmentFields, source, options) => { - // Perform this check first to avoid unecessary sanitizing. + // Perform this check first to avoid unnecessary sanitizing. // TODO Maybe relax string check and coerce instead: // /~https://github.com/tc39/proposal-dynamic-code-brand-checks if (typeof source !== 'string') { From a4ee1ea5e54d894ad3a3ce0c5e42507f026199e6 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Wed, 19 Oct 2022 17:19:15 -0700 Subject: [PATCH 02/19] fix(ses): Typo in scope-constants --- packages/ses/src/scope-constants.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ses/src/scope-constants.js b/packages/ses/src/scope-constants.js index 51ae433022..2c6773eaa2 100644 --- a/packages/ses/src/scope-constants.js +++ b/packages/ses/src/scope-constants.js @@ -77,7 +77,7 @@ const keywords = [ /** * identifierPattern - * Simplified validation of indentifier names: may only contain alphanumeric + * Simplified validation of identifier names: may only contain alphanumeric * characters (or "$" or "_"), and may not start with a digit. This is safe * and does not reduces the compatibility of the shim. The motivation for * this limitation was to decrease the complexity of the implementation, From eb509028692e1922c0cd55b4637523b7396dfb40 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Mon, 17 Oct 2022 21:29:29 -0700 Subject: [PATCH 03/19] test(ses): Remove property prototype pollution test Per #1329, we will no longer attempt to defend against poorly-vetted shims that generally break property descriptors. --- .../ses/test/test-scope-handler-pollution.js | 52 ------------------- 1 file changed, 52 deletions(-) delete mode 100644 packages/ses/test/test-scope-handler-pollution.js diff --git a/packages/ses/test/test-scope-handler-pollution.js b/packages/ses/test/test-scope-handler-pollution.js deleted file mode 100644 index a50ce3b26e..0000000000 --- a/packages/ses/test/test-scope-handler-pollution.js +++ /dev/null @@ -1,52 +0,0 @@ -import test from 'ava'; -import { objectHasOwnProperty } from '../src/commons.js'; -import { createScopeHandler } from '../src/scope-handler.js'; - -// Pollute the object prototype such to trick 'value' in propertyDescriptor. -// eslint-disable-next-line no-extend-native -Object.prototype.value = null; - -test('scopeHandler - defends against prototype pollution of property descriptors', t => { - // This verifies that in the face of a polluted 'value' property on - // Object.prototype, the scope handler is sensitive only to an owned 'value' - // property on a property-descriptor to behave correctly. - // The globalLexicals object should not leak to the receiver on the - // globalLexicals getter or setter. - - const globalObject = {}; - let gotcha; - let receiver; - - const globalLexicals = { - get gotcha() { - receiver = this; - return gotcha; - }, - set gotcha(value) { - receiver = this; - gotcha = value; - }, - }; - - // Verify our assumptions. - t.assert('value' in {}); - const prop = Object.getOwnPropertyDescriptor(globalLexicals, 'gotcha'); - t.assert(objectHasOwnProperty(prop, 'get')); - t.assert(objectHasOwnProperty(prop, 'set')); - t.assert(!objectHasOwnProperty(prop, 'value')); - t.assert('value' in prop); // Due to pollution - - const { scopeHandler: handler } = createScopeHandler( - globalObject, - globalLexicals, - ); - - handler.set(globalObject, 'gotcha', 42); - t.is(globalObject, receiver); - - t.is(42, handler.get(globalObject, 'gotcha')); - t.is(globalObject, receiver); - - t.is(42, gotcha); - t.is(undefined, globalObject.gotcha); -}); From 37c4b4a22996e33dd4b2a48c67ce649ba88e5528 Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 21 Sep 2022 11:39:45 -1000 Subject: [PATCH 04/19] refactor(ses)!: Divide scope proxy into four layers *BREAKING CHANGE*: By breaking the scope proxy into four separate layers, SES will no longer leak scope proxies and consequently has removed the knownScopeProxies experimental feature. --- packages/ses/NEWS.md | 10 + packages/ses/src/compartment-evaluate.js | 7 +- packages/ses/src/compartment-shim.js | 11 - packages/ses/src/eval-scope.js | 20 ++ packages/ses/src/make-evaluate-factory.js | 20 +- packages/ses/src/make-safe-evaluator.js | 66 +++--- packages/ses/src/scope-constants.js | 2 +- packages/ses/src/scope-handler.js | 211 ------------------ .../src/sloppy-globals-scope-terminator.js | 53 +++++ packages/ses/src/strict-scope-terminator.js | 91 ++++++++ packages/ses/src/whitelist.js | 1 - 11 files changed, 228 insertions(+), 264 deletions(-) create mode 100644 packages/ses/src/eval-scope.js delete mode 100644 packages/ses/src/scope-handler.js create mode 100644 packages/ses/src/sloppy-globals-scope-terminator.js create mode 100644 packages/ses/src/strict-scope-terminator.js diff --git a/packages/ses/NEWS.md b/packages/ses/NEWS.md index 708cf38b70..3f0d4eda3c 100644 --- a/packages/ses/NEWS.md +++ b/packages/ses/NEWS.md @@ -1,5 +1,15 @@ User-visible changes in SES: +# Next release + +- Previous versions of SES would leak the proxy used to isolate evaluated + code to functions added to the global object by guest code. + The value of `this` in such functions should be `undefined`, but that is not + possible to emulate in this shim. + This version changes the value of `this` in such functions to be the same as + `globalThis` of the compartment, as would be correct in sloppy mode. +- Removes experimental support for "known scope proxies". + # v0.16.0 (2022-10-19) - When hardening a typed array, detects and locks down properties named as diff --git a/packages/ses/src/compartment-evaluate.js b/packages/ses/src/compartment-evaluate.js index 895a57fd02..253aaf9136 100644 --- a/packages/ses/src/compartment-evaluate.js +++ b/packages/ses/src/compartment-evaluate.js @@ -28,11 +28,7 @@ export const provideCompartmentEvaluator = (compartmentFields, options) => { // shared evaluator so we need to build a new one let { globalTransforms } = compartmentFields; - const { - globalObject, - globalLexicals, - knownScopeProxies, - } = compartmentFields; + const { globalObject, globalLexicals } = compartmentFields; let localObject = globalLexicals; if (__moduleShimLexicals__ !== undefined) { @@ -58,7 +54,6 @@ export const provideCompartmentEvaluator = (compartmentFields, options) => { globalLexicals: localObject, globalTransforms, sloppyGlobalsMode, - knownScopeProxies, })); } diff --git a/packages/ses/src/compartment-shim.js b/packages/ses/src/compartment-shim.js index 7a2390f92e..64b2062905 100644 --- a/packages/ses/src/compartment-shim.js +++ b/packages/ses/src/compartment-shim.js @@ -7,7 +7,6 @@ import { ReferenceError, TypeError, WeakMap, - WeakSet, arrayFilter, arrayJoin, assign, @@ -18,7 +17,6 @@ import { promiseThen, weakmapGet, weakmapSet, - weaksetHas, } from './commons.js'; import { setGlobalObjectConstantProperties, @@ -119,12 +117,6 @@ export const CompartmentPrototype = { return '[object Compartment]'; }, - /* eslint-disable-next-line no-underscore-dangle */ - __isKnownScopeProxy__(value) { - const { knownScopeProxies } = weakmapGet(privateFields, this); - return weaksetHas(knownScopeProxies, value); - }, - module(specifier) { if (typeof specifier !== 'string') { throw new TypeError('first argument of module() must be a string'); @@ -284,13 +276,11 @@ export const makeCompartmentConstructor = ( // evaluator is no longer eagerly created setGlobalObjectConstantProperties(globalObject); - const knownScopeProxies = new WeakSet(); const { safeEvaluate } = makeSafeEvaluator({ globalObject, globalLexicals, globalTransforms, sloppyGlobalsMode: false, - knownScopeProxies, }); setGlobalObjectMutableProperties(globalObject, { @@ -313,7 +303,6 @@ export const makeCompartmentConstructor = ( name: `${name}`, globalTransforms, globalObject, - knownScopeProxies, globalLexicals, safeEvaluate, resolveHook, diff --git a/packages/ses/src/eval-scope.js b/packages/ses/src/eval-scope.js new file mode 100644 index 0000000000..8cbd205598 --- /dev/null +++ b/packages/ses/src/eval-scope.js @@ -0,0 +1,20 @@ +import { FERAL_EVAL, create, freeze } from './commons.js'; + +export const createEvalScope = () => { + const evalScope = create(null); + const oneTimeEvalProperties = freeze({ + eval: { + get() { + delete evalScope.eval; + return FERAL_EVAL; + }, + enumerable: false, + configurable: true, + }, + }); + + return { + evalScope, + oneTimeEvalProperties, + }; +}; diff --git a/packages/ses/src/make-evaluate-factory.js b/packages/ses/src/make-evaluate-factory.js index ed155eec40..f9b59f8596 100644 --- a/packages/ses/src/make-evaluate-factory.js +++ b/packages/ses/src/make-evaluate-factory.js @@ -12,7 +12,7 @@ function buildOptimizer(constants) { if (constants.length === 0) return ''; // Use 'this' to avoid going through the scope proxy, which is unecessary // since the optimizer only needs references to the safe global. - return `const {${arrayJoin(constants, ',')}} = this;`; + return `const {${arrayJoin(constants, ',')}} = this.optimizerObject;`; } /** @@ -60,12 +60,18 @@ export const makeEvaluateFactory = (constants = []) => { // We could probably both move the optimizer into the inner function // and we could also simplify makeEvaluateFactory to simply evaluate. return FERAL_FUNCTION(` - with (this) { - ${optimizer} - return function() { - 'use strict'; - return eval(arguments[0]); - }; + with (this.scopeTerminator) { + with (this.globalObject) { + with (this.globalLexicals) { + with (this.evalScope) { + ${optimizer} + return function() { + 'use strict'; + return eval(arguments[0]); + }; + } + } + } } `); }; diff --git a/packages/ses/src/make-safe-evaluator.js b/packages/ses/src/make-safe-evaluator.js index 505554c240..0100e7fbfe 100644 --- a/packages/ses/src/make-safe-evaluator.js +++ b/packages/ses/src/make-safe-evaluator.js @@ -2,14 +2,17 @@ // /~https://github.com/v8/v8/blob/master/src/builtins/builtins-function.cc import { - WeakSet, apply, - immutableObject, - proxyRevocable, - weaksetAdd, + create, + assign, + freeze, + defineProperties, + getOwnPropertyDescriptors, } from './commons.js'; import { getScopeConstants } from './scope-constants.js'; -import { createScopeHandler } from './scope-handler.js'; +import { strictScopeTerminator } from './strict-scope-terminator.js'; +import { createSloppyGlobalsScopeTerminator } from './sloppy-globals-scope-terminator.js'; +import { createEvalScope } from './eval-scope.js'; import { applyTransforms, mandatoryTransforms } from './transforms.js'; import { makeEvaluateFactory } from './make-evaluate-factory.js'; import { assert } from './error/assert.js'; @@ -26,27 +29,17 @@ const { details: d } = assert; * @param {Object} [options.globalLexicals] * @param {Array} [options.globalTransforms] * @param {bool} [options.sloppyGlobalsMode] - * @param {WeakSet} [options.knownScopeProxies] */ export const makeSafeEvaluator = ({ globalObject, globalLexicals = {}, globalTransforms = [], sloppyGlobalsMode = false, - knownScopeProxies = new WeakSet(), } = {}) => { - const { scopeHandler, scopeController } = createScopeHandler( - globalObject, - globalLexicals, - { - sloppyGlobalsMode, - }, - ); - const { proxy: scopeProxy, revoke: revokeScopeProxy } = proxyRevocable( - immutableObject, - scopeHandler, - ); - weaksetAdd(knownScopeProxies, scopeProxy); + const scopeTerminator = sloppyGlobalsMode + ? createSloppyGlobalsScopeTerminator(globalObject) + : strictScopeTerminator; + const { evalScope, oneTimeEvalProperties } = createEvalScope(); // Defer creating the actual evaluator to first use. // Creating a compartment should be possible in no-eval environments @@ -56,7 +49,24 @@ export const makeSafeEvaluator = ({ if (!evaluate) { const constants = getScopeConstants(globalObject, globalLexicals); const evaluateFactory = makeEvaluateFactory(constants); - evaluate = apply(evaluateFactory, scopeProxy, []); + const optimizerObject = defineProperties( + create(null), + assign( + getOwnPropertyDescriptors(globalObject), + getOwnPropertyDescriptors(globalLexicals), + ), + ); + evaluate = apply( + evaluateFactory, + freeze({ + optimizerObject, + evalScope, + globalLexicals, + globalObject, + scopeTerminator, + }), + [], + ); } }; @@ -76,7 +86,11 @@ export const makeSafeEvaluator = ({ mandatoryTransforms, ]); - scopeController.allowNextEvalToBeUnsafe = true; + // Allow next reference to eval produce the unsafe FERAL_EVAL. + // We avoid defineProperty because it consumes an extra stack frame taming + // its return value. + defineProperties(evalScope, oneTimeEvalProperties); + let err; try { // Ensure that "this" resolves to the safe global. @@ -86,15 +100,14 @@ export const makeSafeEvaluator = ({ err = e; throw e; } finally { - const unsafeEvalWasStillExposed = scopeController.allowNextEvalToBeUnsafe; - scopeController.allowNextEvalToBeUnsafe = false; + const unsafeEvalWasStillExposed = 'eval' in evalScope; + delete evalScope.eval; if (unsafeEvalWasStillExposed) { - // Barring a defect in the SES shim, the scope proxy should allow the + // Barring a defect in the SES shim, the evalScope should allow the // powerful, unsafe `eval` to be used by `evaluate` exactly once, as the // very first name that it attempts to access from the lexical scope. // A defect in the SES shim could throw an exception after we set - // `scopeController.allowNextEvalToBeUnsafe` and before `evaluate` - // calls `eval` internally. + // `evalScope.eval` and before `evaluate` calls `eval` internally. // If we get here, SES is very broken. // This condition is one where this vat is now hopelessly confused, and // the vat as a whole should be aborted. @@ -104,7 +117,6 @@ export const makeSafeEvaluator = ({ // variable resolution via the scopeHandler, and throw an error with // diagnostic info including the thrown error if any from evaluating the // source code. - revokeScopeProxy(); // TODO A GOOD PLACE TO PANIC(), i.e., kill the vat incarnation. // See /~https://github.com/Agoric/SES-shim/issues/490 // eslint-disable-next-line @endo/no-polymorphic-call diff --git a/packages/ses/src/scope-constants.js b/packages/ses/src/scope-constants.js index 2c6773eaa2..237d2bd81c 100644 --- a/packages/ses/src/scope-constants.js +++ b/packages/ses/src/scope-constants.js @@ -160,7 +160,7 @@ export const getScopeConstants = (globalObject, localObject = {}) => { ); // Collect all valid & immutable identifiers from the global that - // are also not present in the endwoments (immutable or not). + // are also not present in the endowments (immutable or not). const globalConstants = arrayFilter( globalNames, name => diff --git a/packages/ses/src/scope-handler.js b/packages/ses/src/scope-handler.js deleted file mode 100644 index 5547539191..0000000000 --- a/packages/ses/src/scope-handler.js +++ /dev/null @@ -1,211 +0,0 @@ -import { - FERAL_EVAL, - Proxy, - String, - TypeError, - create, - freeze, - getOwnPropertyDescriptor, - getOwnPropertyDescriptors, - globalThis, - immutableObject, - objectHasOwnProperty, - reflectGet, - reflectSet, - seal, -} from './commons.js'; -import { assert } from './error/assert.js'; - -const { details: d, quote: q } = assert; - -/** - * alwaysThrowHandler - * This is an object that throws if any property is called. It's used as - * a proxy handler which throws on any trap called. - * It's made from a proxy with a get trap that throws. It's safe to - * create one and share it between all scopeHandlers. - */ -const alwaysThrowHandler = new Proxy( - immutableObject, - freeze({ - get(_shadow, prop) { - // eslint-disable-next-line @endo/no-polymorphic-call - assert.fail( - d`Please report unexpected scope handler trap: ${q(String(prop))}`, - ); - }, - }), -); - -/* - * createScopeHandler() - * ScopeHandler manages a Proxy which serves as the global scope for the - * safeEvaluate operation (the Proxy is the argument of a 'with' binding). - * As described in createSafeEvaluator(), it has several functions: - * - allow the very first (and only the very first) use of 'eval' to map to - * the real (unsafe) eval function, so it acts as a 'direct eval' and can - * access its lexical scope (which maps to the 'with' binding, which the - * ScopeHandler also controls). - * - ensure that all subsequent uses of 'eval' map to the safeEvaluator, - * which lives as the 'eval' property of the safeGlobal. - * - route all other property lookups at the safeGlobal. - * - hide the unsafeGlobal which lives on the scope chain above the 'with'. - * - ensure the Proxy invariants despite some global properties being frozen. - */ -export const createScopeHandler = ( - globalObject, - globalLexicals = {}, - { sloppyGlobalsMode = false } = {}, -) => { - // This flag allow us to determine if the eval() call is an done by the - // compartment's code or if it is user-land invocation, so we can react - // differently. - // Using a flag on an object with a single mutable property allows a safe - // evaluator to signal to the scope proxy without consuming a stack frame. - // Consuming a stack frame could possibly allow an attacker to control the - // stack depth before calling `evaluate` to cause a RangeError before this - // flag can be reset, leaving the unsafe evaluator available. - const scopeController = { - allowNextEvalToBeUnsafe: false, - }; - seal(scopeController); - - const scopeProxyHandlerProperties = { - get(_shadow, prop) { - if (typeof prop === 'symbol') { - return undefined; - } - - // Special treatment for eval. The very first lookup of 'eval' gets the - // unsafe (real direct) eval, so it will get the lexical scope that uses - // the 'with' context. - if (prop === 'eval') { - // test that it is true rather than merely truthy - if (scopeController.allowNextEvalToBeUnsafe === true) { - // revoke before use - scopeController.allowNextEvalToBeUnsafe = false; - return FERAL_EVAL; - } - // fall through - } - - // Properties of the globalLexicals. - if (prop in globalLexicals) { - // Use reflect to defeat accessors that could be present on the - // globalLexicals object itself as `this`. - // This is done out of an overabundance of caution, as the SES shim - // only use the globalLexicals carry globalLexicals and live binding - // traps. - // The globalLexicals are captured as a snapshot of what's passed to - // the Compartment constructor, wherein all accessors and setters are - // eliminated and the result frozen. - // The live binding traps do use accessors, and none of those accessors - // make use of their receiver. - // Live binding traps provide no avenue for user code to observe the - // receiver. - return reflectGet(globalLexicals, prop, globalObject); - } - - // Properties of the global. - return reflectGet(globalObject, prop); - }, - - set(_shadow, prop, value) { - // Properties of the globalLexicals. - if (prop in globalLexicals) { - const desc = getOwnPropertyDescriptor(globalLexicals, prop); - if (objectHasOwnProperty(desc, 'value')) { - // Work around a peculiar behavior in the specs, where - // value properties are defined on the receiver. - return reflectSet(globalLexicals, prop, value); - } - // Ensure that the 'this' value on setters resolves - // to the safeGlobal, not to the globalLexicals object. - return reflectSet(globalLexicals, prop, value, globalObject); - } - - // Properties of the global. - return reflectSet(globalObject, prop, value); - }, - - // we need has() to return false for some names to prevent the lookup from - // climbing the scope chain and eventually reaching the unsafeGlobal - // object (globalThis), which is bad. - - // todo: we'd like to just have has() return true for everything, and then - // use get() to raise a ReferenceError for anything not on the safe global. - // But we want to be compatible with ReferenceError in the normal case and - // the lack of ReferenceError in the 'typeof' case. Must either reliably - // distinguish these two cases (the trap behavior might be different), or - // we rely on a mandatory source-to-source transform to change 'typeof abc' - // to XXX. We already need a mandatory parse to prevent the 'import', - // since it's a special form instead of merely being a global variable/ - - // note: if we make has() return true always, then we must implement a - // set() trap to avoid subverting the protection of strict mode (it would - // accept assignments to undefined globals, when it ought to throw - // ReferenceError for such assignments) - - has(_shadow, prop) { - // unsafeGlobal: hide all properties of the current global - // at the expense of 'typeof' being wrong for those properties. For - // example, in the browser, evaluating 'document = 3', will add - // a property to globalObject instead of throwing a ReferenceError. - - // !!!!! WARNING: DANGER ZONE !!!!!! - // The order of the conditions in the `||` expression below is of the - // utmost importance. Under no circumstances should `eval` be checked - // after `globalObject`. The prototype of the global object is under - // full control of user code and may be replaced by a proxy with a - // `has` trap. If we allow that trap to trigger while the - // `allowNextEvalToBeUnsafe` flag is down, it could allow user code - // to get a hold of `FERAL_EVAL`, resulting in a complete escape of - // the compartment. - // !!!!! WARNING: DANGER ZONE !!!!!! - return ( - sloppyGlobalsMode || - (scopeController.allowNextEvalToBeUnsafe && prop === 'eval') || - prop in globalLexicals || - prop in globalObject || - prop in globalThis - ); - }, - - // note: this is likely a bug of safari - // https://bugs.webkit.org/show_bug.cgi?id=195534 - - getPrototypeOf() { - return null; - }, - - // Chip has seen this happen single stepping under the Chrome/v8 debugger. - // TODO record how to reliably reproduce, and to test if this fix helps. - // TODO report as bug to v8 or Chrome, and record issue link here. - - getOwnPropertyDescriptor(_target, prop) { - // Coerce with `String` in case prop is a symbol. - const quotedProp = q(String(prop)); - // eslint-disable-next-line @endo/no-polymorphic-call - console.warn( - `getOwnPropertyDescriptor trap on scopeHandler for ${quotedProp}`, - new TypeError().stack, - ); - return undefined; - }, - }; - - // The scope handler's prototype is a proxy that throws if any trap other - // than get/set/has are run (like getOwnPropertyDescriptors, apply, - // getPrototypeOf). - const scopeHandler = freeze( - create( - alwaysThrowHandler, - getOwnPropertyDescriptors(scopeProxyHandlerProperties), - ), - ); - - return { - scopeController, - scopeHandler, - }; -}; diff --git a/packages/ses/src/sloppy-globals-scope-terminator.js b/packages/ses/src/sloppy-globals-scope-terminator.js new file mode 100644 index 0000000000..9b9465e39b --- /dev/null +++ b/packages/ses/src/sloppy-globals-scope-terminator.js @@ -0,0 +1,53 @@ +import { + Proxy, + create, + freeze, + getOwnPropertyDescriptors, + immutableObject, + reflectSet, +} from './commons.js'; +import { + strictScopeTerminatorHandler, + alwaysThrowHandler, +} from './strict-scope-terminator.js'; + +/* + * createSloppyGlobalsScopeTerminator() + * strictScopeTerminatorHandler manages a scopeTerminator Proxy which serves as + * the final scope boundary that will always return "undefined" in order + * to prevent access to "start compartment globals". When "sloppyGlobalsMode" + * is true, the Proxy will perform sets on the "globalObject". + */ +export const createSloppyGlobalsScopeTerminator = globalObject => { + const scopeProxyHandlerProperties = { + // inherit scopeTerminator behavior + ...strictScopeTerminatorHandler, + + // Redirect set properties to the globalObject. + set(_shadow, prop, value) { + return reflectSet(globalObject, prop, value); + }, + + // Always claim to have a potential property in order to be the recipient of a set + has(_shadow, _prop) { + return true; + }, + }; + + // The scope handler's prototype is a proxy that throws if any trap other + // than get/set/has are run (like getOwnPropertyDescriptors, apply, + // getPrototypeOf). + const sloppyGlobalsScopeTerminatorHandler = freeze( + create( + alwaysThrowHandler, + getOwnPropertyDescriptors(scopeProxyHandlerProperties), + ), + ); + + const sloppyGlobalsScopeTerminator = new Proxy( + immutableObject, + sloppyGlobalsScopeTerminatorHandler, + ); + + return sloppyGlobalsScopeTerminator; +}; diff --git a/packages/ses/src/strict-scope-terminator.js b/packages/ses/src/strict-scope-terminator.js new file mode 100644 index 0000000000..33646fdfe0 --- /dev/null +++ b/packages/ses/src/strict-scope-terminator.js @@ -0,0 +1,91 @@ +import { + Proxy, + String, + TypeError, + ReferenceError, + create, + freeze, + getOwnPropertyDescriptors, + globalThis, + immutableObject, +} from './commons.js'; +import { assert } from './error/assert.js'; + +const { details: d, quote: q } = assert; + +/** + * alwaysThrowHandler + * This is an object that throws if any property is called. It's used as + * a proxy handler which throws on any trap called. + * It's made from a proxy with a get trap that throws. It's safe to + * create one and share it between all Proxy handlers. + */ +export const alwaysThrowHandler = new Proxy( + immutableObject, + freeze({ + get(_shadow, prop) { + // eslint-disable-next-line @endo/no-polymorphic-call + assert.fail( + d`Please report unexpected scope handler trap: ${q(String(prop))}`, + ); + }, + }), +); + +/* + * scopeProxyHandlerProperties + * scopeTerminatorHandler manages a strictScopeTerminator Proxy which serves as + * the final scope boundary that will always return "undefined" in order + * to prevent access to "start compartment globals". + */ +const scopeProxyHandlerProperties = { + get(_shadow, _prop) { + return undefined; + }, + + set(_shadow, prop, _value) { + // We should only hit this if the has() hook returned true matches the v8 + // ReferenceError message "Uncaught ReferenceError: xyz is not defined" + throw new ReferenceError(`${String(prop)} is not defined`); + }, + + has(_shadow, prop) { + // we must at least return true for all properties on the realm globalThis + return prop in globalThis; + }, + + // note: this is likely a bug of safari + // https://bugs.webkit.org/show_bug.cgi?id=195534 + getPrototypeOf() { + return null; + }, + + // Chip has seen this happen single stepping under the Chrome/v8 debugger. + // TODO record how to reliably reproduce, and to test if this fix helps. + // TODO report as bug to v8 or Chrome, and record issue link here. + getOwnPropertyDescriptor(_target, prop) { + // Coerce with `String` in case prop is a symbol. + const quotedProp = q(String(prop)); + // eslint-disable-next-line @endo/no-polymorphic-call + console.warn( + `getOwnPropertyDescriptor trap on scopeTerminatorHandler for ${quotedProp}`, + new TypeError().stack, + ); + return undefined; + }, +}; + +// The scope handler's prototype is a proxy that throws if any trap other +// than get/set/has are run (like getOwnPropertyDescriptors, apply, +// getPrototypeOf). +export const strictScopeTerminatorHandler = freeze( + create( + alwaysThrowHandler, + getOwnPropertyDescriptors(scopeProxyHandlerProperties), + ), +); + +export const strictScopeTerminator = new Proxy( + immutableObject, + strictScopeTerminatorHandler, +); diff --git a/packages/ses/src/whitelist.js b/packages/ses/src/whitelist.js index e085d88793..840ad7075c 100644 --- a/packages/ses/src/whitelist.js +++ b/packages/ses/src/whitelist.js @@ -1361,7 +1361,6 @@ export const whitelist = { name: getter, // Should this be proposed? toString: fn, - __isKnownScopeProxy__: fn, import: asyncFn, load: asyncFn, importNow: fn, From ec21beb9b89f8bc69c9e865a32bc8322fd89d3b3 Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:16:02 -0700 Subject: [PATCH 05/19] test(ses): Remove obsolete known scope proxies tests --- .../ses/test/test-compartment-instance.js | 1 - .../test-compartment-known-scope-proxy.js | 106 ------------------ .../ses/test/test-compartment-prototype.js | 1 - 3 files changed, 108 deletions(-) delete mode 100644 packages/ses/test/test-compartment-known-scope-proxy.js diff --git a/packages/ses/test/test-compartment-instance.js b/packages/ses/test/test-compartment-instance.js index 93e2125ef7..8c90cf7788 100644 --- a/packages/ses/test/test-compartment-instance.js +++ b/packages/ses/test/test-compartment-instance.js @@ -28,7 +28,6 @@ test('Compartment instance', t => { t.deepEqual( Reflect.ownKeys(Object.getPrototypeOf(c)).sort(), [ - '__isKnownScopeProxy__', 'constructor', 'evaluate', 'globalThis', diff --git a/packages/ses/test/test-compartment-known-scope-proxy.js b/packages/ses/test/test-compartment-known-scope-proxy.js deleted file mode 100644 index ac838361f7..0000000000 --- a/packages/ses/test/test-compartment-known-scope-proxy.js +++ /dev/null @@ -1,106 +0,0 @@ -/* eslint-disable no-underscore-dangle */ -import test from 'ava'; -import '../index.js'; - -lockdown(); - -function leakScopeProxy() { - return this; -} - -test('SES compartment recognizes its own scopeProxies', t => { - const c = new Compartment({ leakScopeProxy }); - const scopeProxy1 = c.evaluate('leakScopeProxy()'); - const scopeProxy2 = c.evaluate('leakScopeProxy()', { - // Force a new evaluate to be created - __moduleShimLexicals__: {}, - }); - const scopeProxyEval = c.globalThis.eval('leakScopeProxy()'); - const scopeProxyFunction = new c.globalThis.Function( - 'return leakScopeProxy()', - )(); - t.is(1, 1); - t.not(scopeProxy1, scopeProxy2); - // c.evaluate, globalThis.Function and globalThis.eval share evaluator - t.is(scopeProxy1, scopeProxyEval); - t.is(scopeProxy1, scopeProxyFunction); - t.is(typeof scopeProxy1, 'object'); - t.is(typeof scopeProxy2, 'object'); - t.is(typeof scopeProxyEval, 'object'); - t.is(typeof scopeProxyFunction, 'object'); - t.is(c.__isKnownScopeProxy__(scopeProxy1), true); - t.is(c.__isKnownScopeProxy__(scopeProxy2), true); - t.is(c.__isKnownScopeProxy__(scopeProxyEval), true); - t.is(c.__isKnownScopeProxy__(scopeProxyFunction), true); - t.is(c.__isKnownScopeProxy__({}), false); -}); - -test('SES compartment does not recognize other scopeProxies', t => { - const c1 = new Compartment({ leakScopeProxy }); - const c2 = new Compartment({ leakScopeProxy }); - const scopeProxy1 = c1.evaluate('leakScopeProxy()'); - const scopeProxy2 = c2.evaluate('leakScopeProxy()'); - t.is(1, 1); - t.not(scopeProxy1, scopeProxy2); - t.is(typeof scopeProxy1, 'object'); - t.is(typeof scopeProxy2, 'object'); - t.is(c1.__isKnownScopeProxy__(scopeProxy1), true); - t.is(c2.__isKnownScopeProxy__(scopeProxy2), true); - t.is(c1.__isKnownScopeProxy__(scopeProxy2), false); - t.is(c2.__isKnownScopeProxy__(scopeProxy1), false); -}); - -test('scope proxy leak workaround usecase', t => { - function createFunctionWrapper(sourceValue, unwrapTest, unwrapTo) { - const newValue = function functionWrapper(...args) { - if (new.target) { - // handle constructor calls - return Reflect.construct(sourceValue, args, new.target); - } else { - // handle function calls - // replace the "this" value if the unwrapTest returns truthy - const thisRef = unwrapTest(this) ? unwrapTo : this; - return Reflect.apply(sourceValue, thisRef, args); - } - }; - Object.defineProperties( - newValue, - Object.getOwnPropertyDescriptors(sourceValue), - ); - return newValue; - } - - function fixProxyLeak(compartment) { - Object.entries(Object.getOwnPropertyDescriptors(compartment.globalThis)) - // for now, for simplicity, apply only to configurable non-getter function values - .filter( - ([_, propDesc]) => - 'value' in propDesc && - typeof propDesc.value === 'function' && - propDesc.configurable, - ) - // redefine with workaround - .forEach(([key, propDesc]) => { - const newFn = createFunctionWrapper( - propDesc.value, - /* eslint-disable-next-line no-underscore-dangle */ - value => compartment.__isKnownScopeProxy__(value), - compartment.globalThis, - ); - const newPropDesc = { ...propDesc, value: newFn }; - Reflect.defineProperty(compartment.globalThis, key, newPropDesc); - }); - } - - function getThisValue() { - return this; - } - - const compartment = new Compartment({ getThisValue }); - const valueBeforeFix = compartment.evaluate('getThisValue()'); - fixProxyLeak(compartment); - const valueAfterFix = compartment.evaluate('getThisValue()'); - - t.not(valueBeforeFix, compartment.globalThis); - t.is(valueAfterFix, compartment.globalThis); -}); diff --git a/packages/ses/test/test-compartment-prototype.js b/packages/ses/test/test-compartment-prototype.js index ebc7854ffd..7dae54ce0c 100644 --- a/packages/ses/test/test-compartment-prototype.js +++ b/packages/ses/test/test-compartment-prototype.js @@ -15,7 +15,6 @@ test('Compartment prototype', t => { t.deepEqual( Reflect.ownKeys(Compartment.prototype).sort(), [ - '__isKnownScopeProxy__', 'constructor', 'evaluate', 'globalThis', From 8eff0129c43200edc36461b939a9b107bbf4b326 Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:18:21 -0700 Subject: [PATCH 06/19] test(ses): Add scope terminator tests --- .../test-sloppy-globals-scope-terminator.js | 82 ++++++++++++++++++ .../ses/test/test-strict-scope-terminator.js | 83 +++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 packages/ses/test/test-sloppy-globals-scope-terminator.js create mode 100644 packages/ses/test/test-strict-scope-terminator.js diff --git a/packages/ses/test/test-sloppy-globals-scope-terminator.js b/packages/ses/test/test-sloppy-globals-scope-terminator.js new file mode 100644 index 0000000000..43c5e66152 --- /dev/null +++ b/packages/ses/test/test-sloppy-globals-scope-terminator.js @@ -0,0 +1,82 @@ +import test from 'ava'; +import { createSloppyGlobalsScopeTerminator } from '../src/sloppy-globals-scope-terminator.js'; + +test('sloppyGlobalsScopeTerminator/get - always has properties but they are undefined', t => { + t.plan(4); + + const globalObject = {}; + const scopeTerminator = createSloppyGlobalsScopeTerminator(globalObject); + + t.is(Reflect.has(scopeTerminator, 'eval'), true); + t.is(Reflect.get(scopeTerminator, 'eval'), undefined); + + t.is(Reflect.has(scopeTerminator, 'xyz'), true); + t.is(Reflect.get(scopeTerminator, 'xyz'), undefined); +}); + +test('sloppyGlobalsScopeTerminator/set - sets happen on the globalObject', t => { + t.plan(6); + + const globalObject = {}; + const scopeTerminator = createSloppyGlobalsScopeTerminator(globalObject); + + t.is(Reflect.set(scopeTerminator, 'eval', 42), true); + t.is(Reflect.get(scopeTerminator, 'eval'), undefined); + t.is(Reflect.get(globalObject, 'eval'), 42); + + t.is(Reflect.set(scopeTerminator, 'xyz', 42), true); + t.is(Reflect.get(scopeTerminator, 'xyz'), undefined); + t.is(Reflect.get(globalObject, 'xyz'), 42); +}); + +test('sloppyGlobalsScopeTerminator/getPrototypeOf - has null prototype', t => { + t.plan(1); + + const globalObject = {}; + const scopeTerminator = createSloppyGlobalsScopeTerminator(globalObject); + + t.is(Reflect.getPrototypeOf(scopeTerminator), null); +}); + +test('sloppyGlobalsScopeTerminator/getOwnPropertyDescriptor - always has start compartment properties but provides no prop desc', t => { + t.plan(4); + + const globalObject = {}; + const scopeTerminator = createSloppyGlobalsScopeTerminator(globalObject); + + // Mute warnings + const originalWarn = console.warn; + t.teardown(() => { + console.warn = originalWarn; + }); + console.warn = () => {}; + + t.is(Reflect.has(scopeTerminator, 'eval'), true); + t.is(Reflect.getOwnPropertyDescriptor(scopeTerminator, 'eval'), undefined); + t.is(Reflect.has(scopeTerminator, 'xyz'), true); + t.is(Reflect.getOwnPropertyDescriptor(scopeTerminator, 'xyz'), undefined); +}); + +test('sloppyGlobalsScopeTerminator/etc - all other handlers throw errors', t => { + t.plan(8); + + const globalObject = {}; + const scopeTerminator = createSloppyGlobalsScopeTerminator(globalObject); + + t.throws(() => Reflect.apply(scopeTerminator), { instanceOf: Error }); + t.throws(() => Reflect.construct(scopeTerminator), { instanceOf: Error }); + t.throws(() => Reflect.defineProperty(scopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.deleteProperty(scopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.isExtensible(scopeTerminator), { instanceOf: Error }); + t.throws(() => Reflect.ownKeys(scopeTerminator), { instanceOf: Error }); + t.throws(() => Reflect.preventExtensions(scopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.setPrototypeOf(scopeTerminator), { + instanceOf: Error, + }); +}); diff --git a/packages/ses/test/test-strict-scope-terminator.js b/packages/ses/test/test-strict-scope-terminator.js new file mode 100644 index 0000000000..4999649a91 --- /dev/null +++ b/packages/ses/test/test-strict-scope-terminator.js @@ -0,0 +1,83 @@ +import test from 'ava'; +import { strictScopeTerminator } from '../src/strict-scope-terminator.js'; + +test('strictScopeTerminator/get - always has start compartment properties but they are undefined', t => { + t.plan(4); + + t.is(Reflect.has(strictScopeTerminator, 'eval'), true); + t.is(Reflect.get(strictScopeTerminator, 'eval'), undefined); + + t.is(Reflect.has(strictScopeTerminator, 'xyz'), false); + t.is(Reflect.get(strictScopeTerminator, 'xyz'), undefined); +}); + +test('strictScopeTerminator/set - always disallows sets', t => { + t.plan(2); + + // disallows set because eval exists on the global object + t.throws(() => Reflect.set(strictScopeTerminator, 'eval', 42), { + instanceOf: ReferenceError, + }); + // disallows set because xyz does not exist on the global object + t.throws(() => Reflect.set(strictScopeTerminator, 'xyz', 42), { + instanceOf: ReferenceError, + }); +}); + +test('strictScopeTerminator/getPrototypeOf - has null prototype', t => { + t.plan(1); + + t.is(Reflect.getPrototypeOf(strictScopeTerminator), null); +}); + +test('strictScopeTerminator/getOwnPropertyDescriptor - always has start compartment properties but provides no prop desc', t => { + t.plan(8); + + const originalWarn = console.warn; + let didWarn = 0; + console.warn = () => { + didWarn += 1; + }; + + t.is(Reflect.has(strictScopeTerminator, 'eval'), true); + t.is(didWarn, 0); + t.is( + Reflect.getOwnPropertyDescriptor(strictScopeTerminator, 'eval'), + undefined, + ); + t.is(didWarn, 1); + t.is(Reflect.has(strictScopeTerminator, 'xyz'), false); + t.is(didWarn, 1); + t.is( + Reflect.getOwnPropertyDescriptor(strictScopeTerminator, 'xyz'), + undefined, + ); + t.is(didWarn, 2); + + console.warn = originalWarn; +}); + +test('strictScopeTerminator/etc - all other handlers throw errors', t => { + t.plan(8); + + t.throws(() => Reflect.apply(strictScopeTerminator), { instanceOf: Error }); + t.throws(() => Reflect.construct(strictScopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.defineProperty(strictScopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.deleteProperty(strictScopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.isExtensible(strictScopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.ownKeys(strictScopeTerminator), { instanceOf: Error }); + t.throws(() => Reflect.preventExtensions(strictScopeTerminator), { + instanceOf: Error, + }); + t.throws(() => Reflect.setPrototypeOf(strictScopeTerminator), { + instanceOf: Error, + }); +}); From a3df403b0d07b5a30bdeeebb05b201aa8aed407a Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:19:10 -0700 Subject: [PATCH 07/19] test(ses): Test evaluator evalScope --- packages/ses/test/test-eval-scope.js | 37 ++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 packages/ses/test/test-eval-scope.js diff --git a/packages/ses/test/test-eval-scope.js b/packages/ses/test/test-eval-scope.js new file mode 100644 index 0000000000..ef104f4e9b --- /dev/null +++ b/packages/ses/test/test-eval-scope.js @@ -0,0 +1,37 @@ +import test from 'ava'; +import { createEvalScope } from '../src/eval-scope.js'; + +// The original unsafe untamed eval function, which must not escape. +// Sample at module initialization time, which is before lockdown can +// repair it. Use it only to build powerless abstractions. +// eslint-disable-next-line no-eval +const FERAL_EVAL = eval; + +test('evalScope - is not created with eval exposed', t => { + t.plan(2); + + const { evalScope, oneTimeEvalProperties } = createEvalScope(); + + t.is(Reflect.has(evalScope, 'eval'), false); + + Object.defineProperties(evalScope, oneTimeEvalProperties); + + t.is(Reflect.has(evalScope, 'eval'), true); +}); + +test('evalScope - getting eval removes it from evalScope', t => { + t.plan(5); + + const { evalScope, oneTimeEvalProperties } = createEvalScope(); + + t.is(Reflect.has(evalScope, 'eval'), false); + + Object.defineProperties(evalScope, oneTimeEvalProperties); + + t.is(Reflect.has(evalScope, 'eval'), true); + + t.is(Reflect.get(evalScope, 'eval'), FERAL_EVAL); + + t.is(Reflect.get(evalScope, 'eval'), undefined); + t.is(Reflect.has(evalScope, 'eval'), false); +}); From 2b202f7b50a7e8837aa8165d9e779968cb356ab0 Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:22:05 -0700 Subject: [PATCH 08/19] test(ses): Adjust expectations of fidelity for global unscopables --- packages/ses/test/test-confinement.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/ses/test/test-confinement.js b/packages/ses/test/test-confinement.js index d851e53169..cb478b6d99 100644 --- a/packages/ses/test/test-confinement.js +++ b/packages/ses/test/test-confinement.js @@ -72,9 +72,11 @@ test('confinement evaluation Symbol.unscopables with-statement escape', t => { const c = new Compartment({ flag: 'safe' }); t.is(c.evaluate('Symbol.unscopables'), Symbol.unscopables); + // This modification causes a loss of shim fidelity, but not a loss of + // security. t.is( c.evaluate('globalThis[Symbol.unscopables] = { flag: true }; flag'), - 'safe', + undefined, ); delete globalThis.flag; From dd6ea0b97424f5a668fe9303c9328ad8bd4693f1 Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:23:05 -0700 Subject: [PATCH 09/19] test(ses): Include more with blocks in expected shape of magic lines --- packages/ses/test/test-make-evaluate-factory.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ses/test/test-make-evaluate-factory.js b/packages/ses/test/test-make-evaluate-factory.js index bf7f91edb4..3e0b09cf76 100644 --- a/packages/ses/test/test-make-evaluate-factory.js +++ b/packages/ses/test/test-make-evaluate-factory.js @@ -9,7 +9,7 @@ test('Intrinsics - values', t => { .toString() .replace(/\s/g, ' ') .replace(/ +/g, ' '), - "function anonymous( ) { with (this) { return function() { 'use strict'; return eval(arguments[0]); }; } }", + "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", ); t.is( @@ -17,6 +17,6 @@ test('Intrinsics - values', t => { .toString() .replace(/\s/g, ' ') .replace(/ +/g, ' '), - "function anonymous( ) { with (this) { const {foot} = this; return function() { 'use strict'; return eval(arguments[0]); }; } }", + "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {foot} = this.optimizerObject; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", ); }); From 53958d937578706bc7231ade750abc25a984f0b4 Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:23:58 -0700 Subject: [PATCH 10/19] test(ses): Adjust expectations about scope behaviors --- packages/ses/test/test-make-eval-function.js | 2 +- packages/ses/test/test-scope.js | 95 ++++++++++++-------- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/packages/ses/test/test-make-eval-function.js b/packages/ses/test/test-make-eval-function.js index 78c80ee357..1eb449aaec 100644 --- a/packages/ses/test/test-make-eval-function.js +++ b/packages/ses/test/test-make-eval-function.js @@ -21,7 +21,7 @@ test('makeEvalFunction - leak', t => { t.is(safeEval('none'), undefined); t.is(safeEval('this.none'), undefined); - safeEval('none = 8'); + safeEval('this.none = 8'); t.is(safeEval('none'), 8); t.is(safeEval('this.none'), 8); diff --git a/packages/ses/test/test-scope.js b/packages/ses/test/test-scope.js index 2a792a0d4b..3b01310c72 100644 --- a/packages/ses/test/test-scope.js +++ b/packages/ses/test/test-scope.js @@ -57,7 +57,7 @@ test('scope behavior - lookup in sloppyGlobalsMode', t => { }); test('scope behavior - this-value', t => { - t.plan(17); + t.plan(16); let globalObjectProtoSetterValue; let globalObjectSetterValue; @@ -169,39 +169,34 @@ test('scope behavior - this-value', t => { }, }); - const knownScopeProxies = new WeakSet(); const { safeEvaluate: evaluate } = makeSafeEvaluator({ globalObject, globalLexicals, - knownScopeProxies, }); // Known compromise in fidelity of the emulated script environment (all tests): t.is(evaluate('globalObjectProtoGetter'), globalObject); t.is(evaluate('globalObjectGetter'), globalObject); - t.is(evaluate('globalLexicalsProtoGetter'), globalObject); - t.is(evaluate('globalLexicalsGetter'), globalObject); + t.is(evaluate('globalLexicalsProtoGetter'), globalLexicals); + t.is(evaluate('globalLexicalsGetter'), globalLexicals); evaluate('globalObjectProtoSetter = 123'); t.is(globalObjectProtoSetterValue, globalObject); evaluate('globalObjectSetter = 123'); t.is(globalObjectSetterValue, globalObject); - // bug: properties in prototype of globalLexicals error on set - t.throws(() => evaluate('globalLexicalsProtoSetter = 123'), { - instanceOf: Error, - }); - t.is(globalLexicalsProtoSetterValue, undefined); + evaluate('globalLexicalsProtoSetter = 123'); + t.is(globalLexicalsProtoSetterValue, globalLexicals); evaluate('globalLexicalsSetter = 123'); - t.is(globalLexicalsSetterValue, globalObject); + t.is(globalLexicalsSetterValue, globalLexicals); - t.true(knownScopeProxies.has(evaluate('globalObjectProtoFn()'))); - t.true(knownScopeProxies.has(evaluate('globalObjectFn()'))); - t.true(knownScopeProxies.has(evaluate('globalObjectProtoFnImmutable()'))); + t.is(evaluate('globalObjectProtoFn()'), globalObject); + t.is(evaluate('globalObjectFn()'), globalObject); + t.is(evaluate('globalObjectProtoFnImmutable()'), globalObject); t.is(evaluate('globalObjectFnImmutable()'), undefined); - t.true(knownScopeProxies.has(evaluate('globalLexicalsProtoFn()'))); - t.true(knownScopeProxies.has(evaluate('globalLexicalsFn()'))); - t.true(knownScopeProxies.has(evaluate('globalLexicalsProtoFnImmutable()'))); + t.is(evaluate('globalLexicalsProtoFn()'), globalLexicals); + t.is(evaluate('globalLexicalsFn()'), globalLexicals); + t.is(evaluate('globalLexicalsProtoFnImmutable()'), globalLexicals); t.is(evaluate('globalLexicalsFnImmutable()'), undefined); }); @@ -274,8 +269,9 @@ test('scope behavior - strict vs sloppy locally non-existing global set', t => { delete globalThis.realmGlobalProp; }); - // Known compromise in fidelity of the emulated script environment: - t.notThrows(() => evaluateStrict('realmGlobalProp = 123')); + t.throws(() => evaluateStrict('realmGlobalProp = 123'), { + instanceOf: ReferenceError, + }); t.throws(() => evaluateStrict('missingRealmGlobalProp = 123'), { instanceOf: ReferenceError, }); @@ -315,7 +311,7 @@ test('scope behavior - realm globalThis property info leak', t => { }); test('scope behavior - Symbol.unscopables fidelity test', t => { - t.plan(32); + t.plan(33); const globalObject = { Symbol, @@ -329,12 +325,16 @@ test('scope behavior - Symbol.unscopables fidelity test', t => { globalObject, }); - t.is(evaluate('typeof localProp'), 'object'); + // Known compromise in fidelity of the emulated script environment: + t.is(evaluate('typeof localProp'), 'undefined'); t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined'); t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined'); t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined'); - t.is(evaluate('localProp'), globalObject.localProp); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('localProp'), { + instanceOf: ReferenceError, + }); t.throws(() => evaluate('eventuallyAssignedLocalProp'), { instanceOf: ReferenceError, }); @@ -350,12 +350,16 @@ test('scope behavior - Symbol.unscopables fidelity test', t => { delete globalThis.eventuallyAssignedRealmGlobalProp; }); - t.is(evaluate('typeof localProp'), 'object'); + // Known compromise in fidelity of the emulated script environment: + t.is(evaluate('typeof localProp'), 'undefined'); t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined'); t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined'); t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined'); - t.is(evaluate('localProp'), globalObject.localProp); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('localProp'), { + instanceOf: ReferenceError, + }); t.throws(() => evaluate('eventuallyAssignedLocalProp'), { instanceOf: ReferenceError, }); @@ -371,18 +375,26 @@ test('scope behavior - Symbol.unscopables fidelity test', t => { // after property is created on globalObject, assignment is evaluated to // test if it is affected by the Symbol.unscopables configuration globalObject.eventuallyAssignedLocalProp = null; - evaluate('eventuallyAssignedLocalProp = {}'); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('eventuallyAssignedLocalProp = {}'), { + instanceOf: ReferenceError, + }); - t.is(evaluate('typeof localProp'), 'object'); - t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'object'); + // Known compromise in fidelity of the emulated script environment: + t.is(evaluate('typeof localProp'), 'undefined'); + // Known compromise in fidelity of the emulated script environment: + t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined'); t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined'); t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined'); - t.is(evaluate('localProp'), globalObject.localProp); - t.is( - evaluate('eventuallyAssignedLocalProp'), - globalObject.eventuallyAssignedLocalProp, - ); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('localProp'), { + instanceOf: ReferenceError, + }); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('eventuallyAssignedLocalProp'), { + instanceOf: ReferenceError, + }); t.throws(() => evaluate('missingRealmGlobalProp'), { instanceOf: ReferenceError, }); @@ -398,16 +410,21 @@ test('scope behavior - Symbol.unscopables fidelity test', t => { eventuallyAssignedLocalProp: true, }; - t.is(evaluate('typeof localProp'), 'object'); - t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'object'); + // Known compromise in fidelity of the emulated script environment: + t.is(evaluate('typeof localProp'), 'undefined'); + // Known compromise in fidelity of the emulated script environment: + t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined'); t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined'); t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined'); - t.is(evaluate('localProp'), globalObject.localProp); - t.is( - evaluate('eventuallyAssignedLocalProp'), - globalObject.eventuallyAssignedLocalProp, - ); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('localProp'), { + instanceOf: ReferenceError, + }); + // Known compromise in fidelity of the emulated script environment: + t.throws(() => evaluate('eventuallyAssignedLocalProp'), { + instanceOf: ReferenceError, + }); t.throws(() => evaluate('missingRealmGlobalProp'), { instanceOf: ReferenceError, }); From 3d9da897ef5598326ee198b6b21443debfad1552 Mon Sep 17 00:00:00 2001 From: kumavis Date: Mon, 17 Oct 2022 21:24:12 -0700 Subject: [PATCH 11/19] test(ses): Remove obsolete scope handler tests --- packages/ses/test/test-scope-handler.js | 251 ------------------------ 1 file changed, 251 deletions(-) delete mode 100644 packages/ses/test/test-scope-handler.js diff --git a/packages/ses/test/test-scope-handler.js b/packages/ses/test/test-scope-handler.js deleted file mode 100644 index 775df64f54..0000000000 --- a/packages/ses/test/test-scope-handler.js +++ /dev/null @@ -1,251 +0,0 @@ -/* global globalThis */ - -import test from 'ava'; -import sinon from 'sinon'; -import { createScopeHandler } from '../src/scope-handler.js'; - -// The original unsafe untamed eval function, which must not escape. -// Sample at module initialization time, which is before lockdown can -// repair it. Use it only to build powerless abstractions. -// eslint-disable-next-line no-eval -const FERAL_EVAL = eval; - -test('scopeHandler - has trap', t => { - t.plan(7); - - globalThis.bar = {}; - - const globalObject = { foo: {} }; - const endowments = { foobar: {} }; - const { scopeHandler: handler } = createScopeHandler( - globalObject, - endowments, - ); - - t.is(handler.has(null, Symbol.unscopables), false); - t.is(handler.has(null, 'arguments'), false); - - t.is(handler.has(null, 'eval'), true); - t.is(handler.has(null, 'foo'), true); - t.is(handler.has(null, 'bar'), true); - t.is(handler.has(null, 'foobar'), true); - t.is(handler.has(null, 'dummy'), false); - - delete globalThis.bar; -}); - -test('scopeHandler - has trap in sloppyGlobalsMode', t => { - t.plan(7); - - const globalObject = {}; - const endowments = {}; - const options = { sloppyGlobalsMode: true }; - const { scopeHandler: handler } = createScopeHandler( - globalObject, - endowments, - options, - ); - - globalThis.bar = {}; - - t.is(handler.has(null, Symbol.unscopables), true); - t.is(handler.has(null, 'arguments'), true); - - t.is(handler.has(null, 'eval'), true); - t.is(handler.has(null, 'foo'), true); - t.is(handler.has(null, 'bar'), true); - t.is(handler.has(null, 'foobar'), true); - t.is(handler.has(null, 'dummy'), true); - - delete globalThis.bar; -}); - -test('scopeHandler - has trap guards eval with its life', t => { - t.plan(5); - let guardDown = false; - let lookedUpGlobal = false; - - // eslint-disable-next-line no-eval - const originalEval = globalThis.eval; - delete globalThis.eval; // eslint-disable-line no-eval - - const globalObject = { - __proto__: new Proxy( - {}, - { - has(...args) { - if (args[1] === 'eval') { - if (guardDown) { - t.fail('Scope Handler let globalObject catch eval'); - } else { - lookedUpGlobal = true; - } - } - return Reflect.has(...args); - }, - }, - ), - }; - - const { - scopeHandler: handler, - scopeController: controller, - } = createScopeHandler(globalObject); - - controller.allowNextEvalToBeUnsafe = true; - guardDown = true; - t.is(handler.has(null, 'eval'), true); - t.is(handler.get(null, 'eval'), FERAL_EVAL); - - controller.allowNextEvalToBeUnsafe = false; - guardDown = false; - t.is(handler.has(null, 'eval'), false, `global object doesn't have eval`); - t.is(handler.get(null, 'eval'), undefined); - t.true(lookedUpGlobal, 'Looked up `eval` on global object'); - - globalThis.eval = originalEval; // eslint-disable-line no-eval -}); - -test('scopeHandler - get trap', t => { - t.plan(7); - - const globalObject = { foo: {} }; - const endowments = { foobar: {} }; - const { scopeHandler: handler } = createScopeHandler( - globalObject, - endowments, - ); - - globalThis.bar = {}; - - t.is(handler.get(null, Symbol.unscopables), undefined); - t.is(handler.get(null, 'arguments'), undefined); - - t.is(handler.get(null, 'eval'), undefined); - t.is(handler.get(null, 'foo'), globalObject.foo); - t.is(handler.get(null, 'bar'), undefined); - t.is(handler.get(null, 'foobar'), endowments.foobar); - t.is(handler.get(null, 'dummy'), undefined); - - delete globalThis.bar; -}); - -test('scopeHandler - get trap - accessors on endowments', t => { - t.plan(2); - - const globalObject = { foo: {} }; - const endowments = {}; - const { scopeHandler: handler } = createScopeHandler( - globalObject, - endowments, - ); - - Object.defineProperties(endowments, { - foo: { - get() { - return this; - }, - }, - }); - - t.not(handler.get(null, 'foo'), endowments); - t.is(handler.get(null, 'foo'), globalObject); -}); - -test('scopeHandler - set trap', t => { - t.plan(13); - - const globalObject = { foo: {} }; - const endowments = { foobar: {} }; - const { scopeHandler: handler } = createScopeHandler( - globalObject, - endowments, - ); - - globalThis.bar = {}; - - const evil = {}; - // eslint-disable-next-line no-eval - const originalEval = globalThis.eval; - handler.set(null, 'eval', evil); - t.is(globalObject.eval, evil); - // eslint-disable-next-line no-eval - t.is(globalThis.eval, originalEval); - - const bar = {}; - const originalBar = globalThis.bar; - handler.set(null, 'bar', bar); - t.is(globalObject.bar, bar); - t.is(globalThis.bar, originalBar); - - const foo = {}; - const originalFoo = globalObject.for; - handler.set(null, 'foo', foo); - t.is(globalObject.foo, foo); - t.not(globalObject.foo, originalFoo); - t.is(globalThis.foo, undefined); - - const foobar = {}; - const originalFoobar = endowments.foobar; - handler.set(null, 'foobar', foobar); - t.is(endowments.foobar, foobar); - t.not(globalObject.foo, originalFoobar); - t.is(globalObject.foobar, undefined); - t.is(globalThis.foobar, undefined); - - t.is(Object.keys(globalObject).length, 3); - t.is(Object.keys(endowments).length, 1); - - delete globalThis.bar; -}); - -test('scopeHandler - get trap - clear allow next unsafe eval', t => { - t.plan(7); - - const globalObject = { eval: {} }; - const { - scopeHandler: handler, - scopeController: controller, - } = createScopeHandler(globalObject); - - t.is(controller.allowNextEvalToBeUnsafe, false); - t.is(handler.get(null, 'eval'), globalObject.eval); - t.is(handler.get(null, 'eval'), globalObject.eval); // repeat - - controller.allowNextEvalToBeUnsafe = true; - t.is(handler.get(null, 'eval'), FERAL_EVAL); - t.is(controller.allowNextEvalToBeUnsafe, false); - t.is(handler.get(null, 'eval'), globalObject.eval); - t.is(handler.get(null, 'eval'), globalObject.eval); // repeat -}); - -test('scopeHandler - throw only for unsupported traps', t => { - t.plan(13); - - sinon.stub(console, 'error').callsFake(); - - const globalObject = {}; - const { scopeHandler: handler } = createScopeHandler(globalObject); - - ['has', 'get', 'set', 'getPrototypeOf'].forEach(trap => - t.notThrows(() => handler[trap]), - ); - - [ - 'apply', - 'construct', - 'defineProperty', - 'delteProperty', - 'getOwnProperty', - 'isExtensible', - 'ownKeys', - 'preventExtensions', - 'setPrototypeOf', - ].forEach(trap => - t.throws(() => handler[trap], { - message: /Please report unexpected scope handler trap:/, - }), - ); - - sinon.restore(); -}); From f70d0464badcf3f67ec2d44d9ad5f46c9893fe98 Mon Sep 17 00:00:00 2001 From: naugtur Date: Fri, 30 Sep 2022 09:35:21 +0200 Subject: [PATCH 12/19] refactor(ses): Colocate logic of exposing FERAL_EVAL inside eval-scope module --- packages/ses/src/eval-scope.js | 9 +++++++-- packages/ses/src/make-safe-evaluator.js | 6 ++---- packages/ses/test/test-eval-scope.js | 8 ++++---- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/ses/src/eval-scope.js b/packages/ses/src/eval-scope.js index 8cbd205598..2555072c84 100644 --- a/packages/ses/src/eval-scope.js +++ b/packages/ses/src/eval-scope.js @@ -1,4 +1,4 @@ -import { FERAL_EVAL, create, freeze } from './commons.js'; +import { FERAL_EVAL, create, freeze, defineProperties } from './commons.js'; export const createEvalScope = () => { const evalScope = create(null); @@ -15,6 +15,11 @@ export const createEvalScope = () => { return { evalScope, - oneTimeEvalProperties, + allowNextEvalToBeUnsafe: () => { + // Allow next reference to eval produce the unsafe FERAL_EVAL. + // We avoid defineProperty because it consumes an extra stack frame taming + // its return value. + defineProperties(evalScope, oneTimeEvalProperties); + }, }; }; diff --git a/packages/ses/src/make-safe-evaluator.js b/packages/ses/src/make-safe-evaluator.js index 0100e7fbfe..fcebd2e971 100644 --- a/packages/ses/src/make-safe-evaluator.js +++ b/packages/ses/src/make-safe-evaluator.js @@ -39,7 +39,7 @@ export const makeSafeEvaluator = ({ const scopeTerminator = sloppyGlobalsMode ? createSloppyGlobalsScopeTerminator(globalObject) : strictScopeTerminator; - const { evalScope, oneTimeEvalProperties } = createEvalScope(); + const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); // Defer creating the actual evaluator to first use. // Creating a compartment should be possible in no-eval environments @@ -87,9 +87,7 @@ export const makeSafeEvaluator = ({ ]); // Allow next reference to eval produce the unsafe FERAL_EVAL. - // We avoid defineProperty because it consumes an extra stack frame taming - // its return value. - defineProperties(evalScope, oneTimeEvalProperties); + allowNextEvalToBeUnsafe(); let err; try { diff --git a/packages/ses/test/test-eval-scope.js b/packages/ses/test/test-eval-scope.js index ef104f4e9b..fd3681d07f 100644 --- a/packages/ses/test/test-eval-scope.js +++ b/packages/ses/test/test-eval-scope.js @@ -10,11 +10,11 @@ const FERAL_EVAL = eval; test('evalScope - is not created with eval exposed', t => { t.plan(2); - const { evalScope, oneTimeEvalProperties } = createEvalScope(); + const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); t.is(Reflect.has(evalScope, 'eval'), false); - Object.defineProperties(evalScope, oneTimeEvalProperties); + allowNextEvalToBeUnsafe(); t.is(Reflect.has(evalScope, 'eval'), true); }); @@ -22,11 +22,11 @@ test('evalScope - is not created with eval exposed', t => { test('evalScope - getting eval removes it from evalScope', t => { t.plan(5); - const { evalScope, oneTimeEvalProperties } = createEvalScope(); + const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); t.is(Reflect.has(evalScope, 'eval'), false); - Object.defineProperties(evalScope, oneTimeEvalProperties); + allowNextEvalToBeUnsafe(); t.is(Reflect.has(evalScope, 'eval'), true); From 83873f65d887fa8b5f451af86f81ea7b42789af4 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Mon, 17 Oct 2022 18:11:43 -0700 Subject: [PATCH 13/19] refactor(ses): Divide scope optimizer --- packages/ses/src/make-evaluate-factory.js | 20 ++++- packages/ses/src/make-safe-evaluator.js | 25 ++---- packages/ses/src/scope-constants.js | 26 +++--- .../ses/test/test-make-evaluate-factory.js | 20 ++++- packages/ses/test/test-scope-constants.js | 90 ++++++++++--------- 5 files changed, 106 insertions(+), 75 deletions(-) diff --git a/packages/ses/src/make-evaluate-factory.js b/packages/ses/src/make-evaluate-factory.js index f9b59f8596..05be063c73 100644 --- a/packages/ses/src/make-evaluate-factory.js +++ b/packages/ses/src/make-evaluate-factory.js @@ -20,10 +20,21 @@ function buildOptimizer(constants) { * The factory create 'evaluate' functions with the correct optimizer * inserted. * - * @param {Array} [constants] + * @param {Array} [globalObjectConstants] + * @param {Array} [globalLexicalConstants] */ -export const makeEvaluateFactory = (constants = []) => { - const optimizer = buildOptimizer(constants); +export const makeEvaluateFactory = ( + globalObjectConstants = [], + globalLexicalConstants = [], +) => { + const globalObjectOptimizer = buildOptimizer( + globalObjectConstants, + 'globalObject', + ); + const globalLexicalOptimizer = buildOptimizer( + globalLexicalConstants, + 'globalLexicals', + ); // Create a function in sloppy mode, so that we can use 'with'. It returns // a function in strict mode that evaluates the provided code using direct @@ -64,7 +75,8 @@ export const makeEvaluateFactory = (constants = []) => { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { - ${optimizer} + ${globalObjectOptimizer} + ${globalLexicalOptimizer} return function() { 'use strict'; return eval(arguments[0]); diff --git a/packages/ses/src/make-safe-evaluator.js b/packages/ses/src/make-safe-evaluator.js index fcebd2e971..ad83aa8e17 100644 --- a/packages/ses/src/make-safe-evaluator.js +++ b/packages/ses/src/make-safe-evaluator.js @@ -1,14 +1,7 @@ // Portions adapted from V8 - Copyright 2016 the V8 project authors. // /~https://github.com/v8/v8/blob/master/src/builtins/builtins-function.cc -import { - apply, - create, - assign, - freeze, - defineProperties, - getOwnPropertyDescriptors, -} from './commons.js'; +import { apply, freeze } from './commons.js'; import { getScopeConstants } from './scope-constants.js'; import { strictScopeTerminator } from './strict-scope-terminator.js'; import { createSloppyGlobalsScopeTerminator } from './sloppy-globals-scope-terminator.js'; @@ -47,19 +40,17 @@ export const makeSafeEvaluator = ({ let evaluate; const makeEvaluate = () => { if (!evaluate) { - const constants = getScopeConstants(globalObject, globalLexicals); - const evaluateFactory = makeEvaluateFactory(constants); - const optimizerObject = defineProperties( - create(null), - assign( - getOwnPropertyDescriptors(globalObject), - getOwnPropertyDescriptors(globalLexicals), - ), + const { + globalObjectConstants, + globalLexicalConstants, + } = getScopeConstants(globalObject, globalLexicals); + const evaluateFactory = makeEvaluateFactory( + globalObjectConstants, + globalLexicalConstants, ); evaluate = apply( evaluateFactory, freeze({ - optimizerObject, evalScope, globalLexicals, globalObject, diff --git a/packages/ses/src/scope-constants.js b/packages/ses/src/scope-constants.js index 237d2bd81c..93c7bfd296 100644 --- a/packages/ses/src/scope-constants.js +++ b/packages/ses/src/scope-constants.js @@ -144,32 +144,36 @@ function isImmutableDataProperty(obj, name) { * safe and only prevent performance optimization. * * @param {Object} globalObject - * @param {Object} localObject + * @param {Object} globalLexicals */ -export const getScopeConstants = (globalObject, localObject = {}) => { +export const getScopeConstants = (globalObject, globalLexicals = {}) => { // getOwnPropertyNames() does ignore Symbols so we don't need to // filter them out. - const globalNames = getOwnPropertyNames(globalObject); - const localNames = getOwnPropertyNames(localObject); + const globalObjectNames = getOwnPropertyNames(globalObject); + const globalLexicalNames = getOwnPropertyNames(globalLexicals); // Collect all valid & immutable identifiers from the endowments. - const localConstants = arrayFilter( - localNames, + const globalLexicalConstants = arrayFilter( + globalLexicalNames, name => - isValidIdentifierName(name) && isImmutableDataProperty(localObject, name), + isValidIdentifierName(name) && + isImmutableDataProperty(globalLexicals, name), ); // Collect all valid & immutable identifiers from the global that // are also not present in the endowments (immutable or not). - const globalConstants = arrayFilter( - globalNames, + const globalObjectConstants = arrayFilter( + globalObjectNames, name => // Can't define a constant: it would prevent a // lookup on the endowments. - !arrayIncludes(localNames, name) && + !arrayIncludes(globalLexicalNames, name) && isValidIdentifierName(name) && isImmutableDataProperty(globalObject, name), ); - return [...globalConstants, ...localConstants]; + return { + globalObjectConstants, + globalLexicalConstants, + }; }; diff --git a/packages/ses/test/test-make-evaluate-factory.js b/packages/ses/test/test-make-evaluate-factory.js index 3e0b09cf76..78aa5b0fc8 100644 --- a/packages/ses/test/test-make-evaluate-factory.js +++ b/packages/ses/test/test-make-evaluate-factory.js @@ -2,7 +2,7 @@ import test from 'ava'; import { makeEvaluateFactory } from '../src/make-evaluate-factory.js'; test('Intrinsics - values', t => { - t.plan(2); + t.plan(4); t.is( makeEvaluateFactory() @@ -17,6 +17,22 @@ test('Intrinsics - values', t => { .toString() .replace(/\s/g, ' ') .replace(/ +/g, ' '), - "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {foot} = this.optimizerObject; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", + "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {foot} = this.globalObject; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", + ); + + t.is( + makeEvaluateFactory([], ['bart']) + .toString() + .replace(/\s/g, ' ') + .replace(/ +/g, ' '), + "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {bart} = this.globalLexicals; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", + ); + + t.is( + makeEvaluateFactory(['foot'], ['bart']) + .toString() + .replace(/\s/g, ' ') + .replace(/ +/g, ' '), + "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {foot} = this.globalObject; const {bart} = this.globalLexicals; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", ); }); diff --git a/packages/ses/test/test-scope-constants.js b/packages/ses/test/test-scope-constants.js index f61b2e60b1..8c68779471 100644 --- a/packages/ses/test/test-scope-constants.js +++ b/packages/ses/test/test-scope-constants.js @@ -4,52 +4,56 @@ import { getScopeConstants } from '../src/scope-constants.js'; test('getScopeConstants - global object', t => { t.plan(20); - t.deepEqual(getScopeConstants({}), [], 'should return empty if no global'); + t.deepEqual( + getScopeConstants({}), + { globalLexicalConstants: [], globalObjectConstants: [] }, + 'should return empty if no global', + ); t.deepEqual( getScopeConstants({ foo: true }), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject configurable & writable', ); t.deepEqual( getScopeConstants(Object.create(null, { foo: { value: true } })), - ['foo'], + { globalObjectConstants: ['foo'], globalLexicalConstants: [] }, 'should return non configurable & non writable', ); t.deepEqual( getScopeConstants( Object.create(null, { foo: { value: true, configurable: true } }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject configurable', ); t.deepEqual( getScopeConstants( Object.create(null, { foo: { value: true, writable: true } }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject writable', ); t.deepEqual( getScopeConstants(Object.create(null, { foo: { get: () => true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject getter', ); t.deepEqual( getScopeConstants(Object.create(null, { foo: { set: () => true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject setter', ); t.deepEqual( getScopeConstants(Object.create(null, { eval: { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject eval', ); t.deepEqual( getScopeConstants(Object.create(null, { const: { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject reserved keyword', ); t.deepEqual( @@ -60,7 +64,7 @@ test('getScopeConstants - global object', t => { false: { value: true }, }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject literals (reserved)', ); t.deepEqual( @@ -70,46 +74,46 @@ test('getScopeConstants - global object', t => { arguments: { value: true }, }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject this and arguments', ); t.deepEqual( getScopeConstants( Object.create(null, { [Symbol.iterator]: { value: true } }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject symbols', ); t.deepEqual( getScopeConstants(Object.create(null, { 123: { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject leading digit', ); t.deepEqual( getScopeConstants(Object.create(null, { '-123': { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject leading dash', ); t.deepEqual( getScopeConstants(Object.create(null, { _123: { value: true } })), - ['_123'], + { globalObjectConstants: ['_123'], globalLexicalConstants: [] }, 'should return leading underscore', ); t.deepEqual( getScopeConstants(Object.create(null, { $123: { value: true } })), - ['$123'], + { globalObjectConstants: ['$123'], globalLexicalConstants: [] }, 'should return leading underscore', ); t.deepEqual( getScopeConstants(Object.create(null, { a123: { value: true } })), - ['a123'], + { globalObjectConstants: ['a123'], globalLexicalConstants: [] }, 'should return leading lowercase', ); t.deepEqual( getScopeConstants(Object.create(null, { A123: { value: true } })), - ['A123'], + { globalObjectConstants: ['A123'], globalLexicalConstants: [] }, 'should return leading uppercase', ); @@ -117,7 +121,7 @@ test('getScopeConstants - global object', t => { getScopeConstants( Object.create(null, { foo: { value: true }, bar: { value: true } }), ), - ['foo', 'bar'], + { globalObjectConstants: ['foo', 'bar'], globalLexicalConstants: [] }, 'should return all non configurable & non writable', ); t.deepEqual( @@ -127,7 +131,7 @@ test('getScopeConstants - global object', t => { bar: { value: true, configurable: true }, }), ), - ['foo'], + { globalObjectConstants: ['foo'], globalLexicalConstants: [] }, 'should return only non configurable & non writable', ); }); @@ -135,16 +139,20 @@ test('getScopeConstants - global object', t => { test('getScopeConstants - local object (endownments)', t => { t.plan(20); - t.deepEqual(getScopeConstants({}, {}), [], 'should return empty if no local'); + t.deepEqual( + getScopeConstants({}, {}), + { globalObjectConstants: [], globalLexicalConstants: [] }, + 'should return empty if no local', + ); t.deepEqual( getScopeConstants({}, { foo: true }), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject configurable & writable', ); t.deepEqual( getScopeConstants({}, Object.create(null, { foo: { value: true } })), - ['foo'], + { globalObjectConstants: [], globalLexicalConstants: ['foo'] }, 'should return non configurable & non writable', ); t.deepEqual( @@ -152,7 +160,7 @@ test('getScopeConstants - local object (endownments)', t => { {}, Object.create(null, { foo: { value: true, configurable: true } }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject configurable', ); t.deepEqual( @@ -160,29 +168,29 @@ test('getScopeConstants - local object (endownments)', t => { {}, Object.create(null, { foo: { value: true, writable: true } }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject writable', ); t.deepEqual( getScopeConstants({}, Object.create(null, { foo: { get: () => true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject getter', ); t.deepEqual( getScopeConstants({}, Object.create(null, { foo: { set: () => true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject setter', ); t.deepEqual( getScopeConstants({}, Object.create(null, { eval: { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject eval', ); t.deepEqual( getScopeConstants({}, Object.create(null, { const: { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject reserved keyword', ); t.deepEqual( @@ -194,7 +202,7 @@ test('getScopeConstants - local object (endownments)', t => { false: { value: true }, }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject literals (reserved)', ); t.deepEqual( @@ -205,7 +213,7 @@ test('getScopeConstants - local object (endownments)', t => { arguments: { value: true }, }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject this and arguments', ); t.deepEqual( @@ -213,39 +221,39 @@ test('getScopeConstants - local object (endownments)', t => { {}, Object.create(null, { [Symbol.iterator]: { value: true } }), ), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject symbols', ); t.deepEqual( getScopeConstants({}, Object.create(null, { 123: { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject leading digit', ); t.deepEqual( getScopeConstants({}, Object.create(null, { '-123': { value: true } })), - [], + { globalObjectConstants: [], globalLexicalConstants: [] }, 'should reject leading dash', ); t.deepEqual( getScopeConstants({}, Object.create(null, { _123: { value: true } })), - ['_123'], + { globalObjectConstants: [], globalLexicalConstants: ['_123'] }, 'should return leading underscore', ); t.deepEqual( getScopeConstants({}, Object.create(null, { $123: { value: true } })), - ['$123'], + { globalObjectConstants: [], globalLexicalConstants: ['$123'] }, 'should return leading underscore', ); t.deepEqual( getScopeConstants({}, Object.create(null, { a123: { value: true } })), - ['a123'], + { globalObjectConstants: [], globalLexicalConstants: ['a123'] }, 'should return leading lowercase', ); t.deepEqual( getScopeConstants({}, Object.create(null, { A123: { value: true } })), - ['A123'], + { globalObjectConstants: [], globalLexicalConstants: ['A123'] }, 'should return leading uppercase', ); @@ -254,7 +262,7 @@ test('getScopeConstants - local object (endownments)', t => { {}, Object.create(null, { foo: { value: true }, bar: { value: true } }), ), - ['foo', 'bar'], + { globalObjectConstants: [], globalLexicalConstants: ['foo', 'bar'] }, 'should return all non configurable & non writable', ); t.deepEqual( @@ -265,7 +273,7 @@ test('getScopeConstants - local object (endownments)', t => { bar: { value: true, configurable: true }, }), ), - ['foo'], + { globalObjectConstants: [], globalLexicalConstants: ['foo'] }, 'should return only non configurable & non writable', ); }); @@ -278,7 +286,7 @@ test('getScopeConstants - global and local object', t => { Object.create(null, { foo: { value: true }, bar: { value: true } }), { foo: false }, ), - ['bar'], + { globalObjectConstants: ['bar'], globalLexicalConstants: [] }, 'should only return global contants not hidden by local', ); }); From 32b42eaa59bdfa104732814b9fc306b7e5a65c28 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Thu, 13 Oct 2022 00:39:17 +0000 Subject: [PATCH 14/19] refactor(ses): introduce makeEvaluate --- ...e-evaluate-factory.js => make-evaluate.js} | 22 ++++++++++++- packages/ses/src/make-safe-evaluator.js | 33 +++++++------------ .../ses/test/test-make-evaluate-factory.js | 2 +- ...aluator.js => test-make-safe-evaluator.js} | 0 4 files changed, 33 insertions(+), 24 deletions(-) rename packages/ses/src/{make-evaluate-factory.js => make-evaluate.js} (84%) rename packages/ses/test/{test-make-evaluator.js => test-make-safe-evaluator.js} (100%) diff --git a/packages/ses/src/make-evaluate-factory.js b/packages/ses/src/make-evaluate.js similarity index 84% rename from packages/ses/src/make-evaluate-factory.js rename to packages/ses/src/make-evaluate.js index 05be063c73..5a8bbf0dcd 100644 --- a/packages/ses/src/make-evaluate-factory.js +++ b/packages/ses/src/make-evaluate.js @@ -1,4 +1,5 @@ -import { FERAL_FUNCTION, arrayJoin } from './commons.js'; +import { FERAL_FUNCTION, arrayJoin, apply } from './commons.js'; +import { getScopeConstants } from './scope-constants.js'; /** * buildOptimizer() @@ -87,3 +88,22 @@ export const makeEvaluateFactory = ( } `); }; + +/** + * @param {object} context + * @param {object} context.evalScope + * @param {object} context.globalLexicals + * @param {object} context.globalObject + * @param {object} context.scopeTerminator + */ +export const makeEvaluate = context => { + const { globalObjectConstants, globalLexicalConstants } = getScopeConstants( + context.globalObject, + context.globalLexicals, + ); + const evaluateFactory = makeEvaluateFactory( + globalObjectConstants, + globalLexicalConstants, + ); + return apply(evaluateFactory, context, []); +}; diff --git a/packages/ses/src/make-safe-evaluator.js b/packages/ses/src/make-safe-evaluator.js index ad83aa8e17..f489eaa902 100644 --- a/packages/ses/src/make-safe-evaluator.js +++ b/packages/ses/src/make-safe-evaluator.js @@ -2,12 +2,11 @@ // /~https://github.com/v8/v8/blob/master/src/builtins/builtins-function.cc import { apply, freeze } from './commons.js'; -import { getScopeConstants } from './scope-constants.js'; import { strictScopeTerminator } from './strict-scope-terminator.js'; import { createSloppyGlobalsScopeTerminator } from './sloppy-globals-scope-terminator.js'; import { createEvalScope } from './eval-scope.js'; import { applyTransforms, mandatoryTransforms } from './transforms.js'; -import { makeEvaluateFactory } from './make-evaluate-factory.js'; +import { makeEvaluate } from './make-evaluate.js'; import { assert } from './error/assert.js'; const { details: d } = assert; @@ -34,30 +33,20 @@ export const makeSafeEvaluator = ({ : strictScopeTerminator; const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + const evaluateContext = freeze({ + evalScope, + globalLexicals, + globalObject, + scopeTerminator, + }); + // Defer creating the actual evaluator to first use. // Creating a compartment should be possible in no-eval environments // It also allows more global constants to be captured by the optimizer let evaluate; - const makeEvaluate = () => { + const provideEvaluate = () => { if (!evaluate) { - const { - globalObjectConstants, - globalLexicalConstants, - } = getScopeConstants(globalObject, globalLexicals); - const evaluateFactory = makeEvaluateFactory( - globalObjectConstants, - globalLexicalConstants, - ); - evaluate = apply( - evaluateFactory, - freeze({ - evalScope, - globalLexicals, - globalObject, - scopeTerminator, - }), - [], - ); + evaluate = makeEvaluate(evaluateContext); } }; @@ -67,7 +56,7 @@ export const makeSafeEvaluator = ({ * @param {Array} [options.localTransforms] */ const safeEvaluate = (source, { localTransforms = [] } = {}) => { - makeEvaluate(); + provideEvaluate(); // Execute the mandatory transforms last to ensure that any rewritten code // meets those mandatory requirements. diff --git a/packages/ses/test/test-make-evaluate-factory.js b/packages/ses/test/test-make-evaluate-factory.js index 78aa5b0fc8..08b5eda3d5 100644 --- a/packages/ses/test/test-make-evaluate-factory.js +++ b/packages/ses/test/test-make-evaluate-factory.js @@ -1,5 +1,5 @@ import test from 'ava'; -import { makeEvaluateFactory } from '../src/make-evaluate-factory.js'; +import { makeEvaluateFactory } from '../src/make-evaluate.js'; test('Intrinsics - values', t => { t.plan(4); diff --git a/packages/ses/test/test-make-evaluator.js b/packages/ses/test/test-make-safe-evaluator.js similarity index 100% rename from packages/ses/test/test-make-evaluator.js rename to packages/ses/test/test-make-safe-evaluator.js From eb4a2fd466d8dd0c3d12907ab27b91d4ead309bf Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Thu, 13 Oct 2022 01:20:48 +0000 Subject: [PATCH 15/19] refactor(ses): remove makeEvaluateFactory remnants Rewrite tests to verify functionality of evaluate instead asserting the string output of the factory. --- packages/ses/src/make-evaluate.js | 42 ++++------ .../ses/test/test-make-evaluate-factory.js | 38 --------- packages/ses/test/test-make-evaluate.js | 78 +++++++++++++++++++ 3 files changed, 93 insertions(+), 65 deletions(-) delete mode 100644 packages/ses/test/test-make-evaluate-factory.js create mode 100644 packages/ses/test/test-make-evaluate.js diff --git a/packages/ses/src/make-evaluate.js b/packages/ses/src/make-evaluate.js index 5a8bbf0dcd..493bc1d662 100644 --- a/packages/ses/src/make-evaluate.js +++ b/packages/ses/src/make-evaluate.js @@ -1,3 +1,5 @@ +// @ts-check + import { FERAL_FUNCTION, arrayJoin, apply } from './commons.js'; import { getScopeConstants } from './scope-constants.js'; @@ -17,17 +19,20 @@ function buildOptimizer(constants) { } /** - * makeEvaluateFactory() - * The factory create 'evaluate' functions with the correct optimizer - * inserted. + * makeEvaluate() + * Create an 'evaluate' function with the correct optimizer inserted. * - * @param {Array} [globalObjectConstants] - * @param {Array} [globalLexicalConstants] + * @param {object} context + * @param {object} context.evalScope + * @param {object} context.globalLexicals + * @param {object} context.globalObject + * @param {object} context.scopeTerminator */ -export const makeEvaluateFactory = ( - globalObjectConstants = [], - globalLexicalConstants = [], -) => { +export const makeEvaluate = context => { + const { globalObjectConstants, globalLexicalConstants } = getScopeConstants( + context.globalObject, + context.globalLexicals, + ); const globalObjectOptimizer = buildOptimizer( globalObjectConstants, 'globalObject', @@ -71,7 +76,7 @@ export const makeEvaluateFactory = ( // reused for multiple evaluations, but in practice we have no such calls. // We could probably both move the optimizer into the inner function // and we could also simplify makeEvaluateFactory to simply evaluate. - return FERAL_FUNCTION(` + const evaluateFactory = FERAL_FUNCTION(` with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { @@ -87,23 +92,6 @@ export const makeEvaluateFactory = ( } } `); -}; -/** - * @param {object} context - * @param {object} context.evalScope - * @param {object} context.globalLexicals - * @param {object} context.globalObject - * @param {object} context.scopeTerminator - */ -export const makeEvaluate = context => { - const { globalObjectConstants, globalLexicalConstants } = getScopeConstants( - context.globalObject, - context.globalLexicals, - ); - const evaluateFactory = makeEvaluateFactory( - globalObjectConstants, - globalLexicalConstants, - ); return apply(evaluateFactory, context, []); }; diff --git a/packages/ses/test/test-make-evaluate-factory.js b/packages/ses/test/test-make-evaluate-factory.js deleted file mode 100644 index 08b5eda3d5..0000000000 --- a/packages/ses/test/test-make-evaluate-factory.js +++ /dev/null @@ -1,38 +0,0 @@ -import test from 'ava'; -import { makeEvaluateFactory } from '../src/make-evaluate.js'; - -test('Intrinsics - values', t => { - t.plan(4); - - t.is( - makeEvaluateFactory() - .toString() - .replace(/\s/g, ' ') - .replace(/ +/g, ' '), - "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", - ); - - t.is( - makeEvaluateFactory(['foot']) - .toString() - .replace(/\s/g, ' ') - .replace(/ +/g, ' '), - "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {foot} = this.globalObject; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", - ); - - t.is( - makeEvaluateFactory([], ['bart']) - .toString() - .replace(/\s/g, ' ') - .replace(/ +/g, ' '), - "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {bart} = this.globalLexicals; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", - ); - - t.is( - makeEvaluateFactory(['foot'], ['bart']) - .toString() - .replace(/\s/g, ' ') - .replace(/ +/g, ' '), - "function anonymous( ) { with (this.scopeTerminator) { with (this.globalObject) { with (this.globalLexicals) { with (this.evalScope) { const {foot} = this.globalObject; const {bart} = this.globalLexicals; return function() { 'use strict'; return eval(arguments[0]); }; } } } } }", - ); -}); diff --git a/packages/ses/test/test-make-evaluate.js b/packages/ses/test/test-make-evaluate.js new file mode 100644 index 0000000000..efe066ac97 --- /dev/null +++ b/packages/ses/test/test-make-evaluate.js @@ -0,0 +1,78 @@ +import test from 'ava'; +import { apply, freeze } from '../src/commons.js'; +import { makeEvaluate } from '../src/make-evaluate.js'; +import { strictScopeTerminator } from '../src/strict-scope-terminator.js'; +import { createEvalScope } from '../src/eval-scope.js'; + +const makeObservingProxy = target => { + const ops = []; + const proxy = new Proxy(target, { + get(_target, prop, _receiver) { + if (prop !== Symbol.unscopables) { + ops.push(['get', prop]); + } + return Reflect.get(target, prop); + }, + }); + + return [proxy, ops]; +}; + +test('makeEvaluate - optimizer', t => { + t.plan(5); + + const globalObjectTarget = Object.create(null, { + foo: { value: true }, + bar: { value: true }, + baz: { value: true, writable: true }, + }); + const globalLexicalsTarget = Object.create(null, { foo: { value: false } }); + + const [globalObject, globalObjectOps] = makeObservingProxy( + globalObjectTarget, + ); + const [globalLexicals, globalLexicalsOps] = makeObservingProxy( + globalLexicalsTarget, + ); + + const scopeTerminator = strictScopeTerminator; + const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + + const evaluate = makeEvaluate( + freeze({ scopeTerminator, globalObject, globalLexicals, evalScope }), + ); + + t.deepEqual(globalObjectOps, [['get', 'bar']]); + t.deepEqual(globalLexicalsOps, [['get', 'foo']]); + + globalObjectOps.length = 0; + globalLexicalsOps.length = 0; + + allowNextEvalToBeUnsafe(); + + const result = apply(evaluate, globalObject, [`!foo && bar && baz`]); + + t.is(result, true); + t.deepEqual(globalObjectOps, [['get', 'baz']]); + t.deepEqual(globalLexicalsOps, []); +}); + +test('makeEvaluate - strict-mode', t => { + t.plan(2); + + const globalObject = Object.create(null); + const globalLexicals = Object.create(null); + + const scopeTerminator = strictScopeTerminator; + const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + + const evaluate = makeEvaluate( + freeze({ scopeTerminator, globalObject, globalLexicals, evalScope }), + ); + + allowNextEvalToBeUnsafe(); + t.throws(() => apply(evaluate, globalObject, [`foo = 42`])); + + allowNextEvalToBeUnsafe(); + t.throws(() => apply(evaluate, globalObject, [`with({}) {}`])); +}); From e8f5889bc17ef81f1dac3ab2bb5131c72f252722 Mon Sep 17 00:00:00 2001 From: Mathieu Hofman Date: Thu, 13 Oct 2022 02:15:48 +0000 Subject: [PATCH 16/19] chore(ses): Update comments of makeEvaluate to reflect reality --- packages/ses/src/make-evaluate.js | 61 +++++++++++++++++++------------ 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/packages/ses/src/make-evaluate.js b/packages/ses/src/make-evaluate.js index 493bc1d662..39fb34eef8 100644 --- a/packages/ses/src/make-evaluate.js +++ b/packages/ses/src/make-evaluate.js @@ -5,17 +5,19 @@ import { getScopeConstants } from './scope-constants.js'; /** * buildOptimizer() - * Given an array of indentifier, the optimizer return a `const` declaration - * destructring `this`. + * Given an array of identifiers, the optimizer returns a `const` declaration + * destructuring `this.${name}`. * * @param {Array} constants + * @param {string} name */ -function buildOptimizer(constants) { +function buildOptimizer(constants, name) { // No need to build an optimizer when there are no constants. if (constants.length === 0) return ''; - // Use 'this' to avoid going through the scope proxy, which is unecessary + // Use 'this' to avoid going through the scope proxy, which is unnecessary // since the optimizer only needs references to the safe global. - return `const {${arrayJoin(constants, ',')}} = this.optimizerObject;`; + // Destructure the constants from the target scope object. + return `const {${arrayJoin(constants, ',')}} = this.${name};`; } /** @@ -47,35 +49,46 @@ export const makeEvaluate = context => { // eval, and thus in strict mode in the same scope. We must be very careful // to not create new names in this scope - // 1: we use 'with' (around a Proxy) to catch all free variable names. The - // `this` value holds the Proxy which safely wraps the safeGlobal - // 2: 'optimizer' catches constant variable names for speed. - // 3: The inner strict function is effectively passed two parameters: + // 1: we use multiple nested 'with' to catch all free variable names. The + // `this` value of the outer sloppy function holds the different scope + // layers, from inner to outer: + // a) `evalScope` which must release the `FERAL_EVAL` as 'eval' once for + // every invocation of the inner `evaluate` function in order to + // trigger direct eval. The direct eval semantics is what allows the + // evaluated code to lookup free variable names on the other scope + // objects and not in global scope. + // b) `globalLexicals` which provide a way to introduce free variables + // that are not available on the globalObject. + // c) `globalObject` is the global scope object of the evaluator, aka the + // Compartment's `globalThis`. + // d) `scopeTerminator` is a proxy object which prevents free variable + // lookups to escape to the start compartment's global object. + // 2: `optimizer`s catch constant variable names for speed. + // 3: The inner strict `evaluate` function should be passed two parameters: // a) its arguments[0] is the source to be directly evaluated. // b) its 'this' is the this binding seen by the code being // directly evaluated (the globalObject). - // 4: The outer sloppy function is passed one parameter, the scope proxy. - // as the `this` parameter. // Notes: - // - everything in the 'optimizer' string is looked up in the proxy - // (including an 'arguments[0]', which points at the Proxy). - // - keywords like 'function' which are reserved keywords, and cannot be - // used as a variable, so they are not part of the optimizer. - // - when 'eval' is looked up in the proxy, and it's the first time it is - // looked up after allowNextEvalToBeUnsafe is turned on, the proxy returns - // the powerful, unsafe eval intrinsic, and flips allowNextEvalToBeUnsafe - // back to false. Any reference to 'eval' in that string will get the tamed - // evaluator. + // - The `optimizer` strings only lookup values on the `globalObject` and + // `globalLexicals` objects by construct. Keywords like 'function' are + // reserved and cannot be used as a variable, so they are excluded from the + // optimizer. Furthermore to prevent shadowing 'eval', while a valid + // identifier, that name is also explicitly excluded. + // - when 'eval' is looked up in the `evalScope`, the powerful unsafe eval + // intrinsic is returned after automatically removing it from the + // `evalScope`. Any further reference to 'eval' in the evaluate string will + // get the tamed evaluator from the `globalObject`, if any. // TODO /~https://github.com/endojs/endo/issues/816 // The optimizer currently runs under sloppy mode, and although we doubt that // there is any vulnerability introduced just by running the optimizer // sloppy, we are much more confident in the semantics of strict mode. - // The motivation for having the optimizer in sloppy mode is that it can be - // reused for multiple evaluations, but in practice we have no such calls. - // We could probably both move the optimizer into the inner function - // and we could also simplify makeEvaluateFactory to simply evaluate. + // The `evaluate` function can be and is reused across multiple evaluations. + // Since the optimizer should not be re-evaluated every time, it cannot be + // inside the `evaluate` closure. While we could potentially nest an + // intermediate layer of `() => {'use strict'; ${optimizers}; ...`, it + // doesn't seem worth the overhead and complexity. const evaluateFactory = FERAL_FUNCTION(` with (this.scopeTerminator) { with (this.globalObject) { From dcb8f5da2a453ac72b7f2cc3208f591a9c298402 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Fri, 14 Oct 2022 16:35:45 -0700 Subject: [PATCH 17/19] fix(ses)!: Prevent surprising global unscopables behavior *BREAKING CHANGE*: Adding a Symbol.unscopables property to globalThis in a compartment will now throw an error, as a preferable outcome to proceding under the false assumption that Symbol.unscopables do not affect global scope, as this is not a behavior of proper JavaScript that the shim can emulate. --- packages/ses/src/commons.js | 1 + packages/ses/src/compartment-shim.js | 3 ++ packages/ses/src/global-object.js | 41 ++++++++++++++++++++++++++- packages/ses/test/test-confinement.js | 13 ++++----- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/packages/ses/src/commons.js b/packages/ses/src/commons.js index 5b7329871c..b970dc5bf7 100644 --- a/packages/ses/src/commons.js +++ b/packages/ses/src/commons.js @@ -74,6 +74,7 @@ export const { toStringTag: toStringTagSymbol, iterator: iteratorSymbol, matchAll: matchAllSymbol, + unscopables: unscopablesSymbol, keyFor: symbolKeyFor, for: symbolFor, } = Symbol; diff --git a/packages/ses/src/compartment-shim.js b/packages/ses/src/compartment-shim.js index 64b2062905..8e2a1b037a 100644 --- a/packages/ses/src/compartment-shim.js +++ b/packages/ses/src/compartment-shim.js @@ -19,6 +19,7 @@ import { weakmapSet, } from './commons.js'; import { + setGlobalObjectSymbolUnscopables, setGlobalObjectConstantProperties, setGlobalObjectMutableProperties, setGlobalObjectEvaluators, @@ -269,6 +270,8 @@ export const makeCompartmentConstructor = ( const globalObject = {}; + setGlobalObjectSymbolUnscopables(globalObject); + // We must initialize all constant properties first because // `makeSafeEvaluator` may use them to create optimized bindings // in the evaluator. diff --git a/packages/ses/src/global-object.js b/packages/ses/src/global-object.js index 1a387d4291..94185cfefd 100644 --- a/packages/ses/src/global-object.js +++ b/packages/ses/src/global-object.js @@ -1,8 +1,47 @@ -import { defineProperty, objectHasOwnProperty, entries } from './commons.js'; +import { + TypeError, + assign, + create, + defineProperty, + entries, + freeze, + objectHasOwnProperty, + unscopablesSymbol, +} from './commons.js'; import { makeEvalFunction } from './make-eval-function.js'; import { makeFunctionConstructor } from './make-function-constructor.js'; import { constantProperties, universalPropertyNames } from './whitelist.js'; +/** + * The host's ordinary global object is not provided by a `with` block, so + * assigning to Symbol.unscopables has no effect. + * Since this shim uses `with` blocks to create a confined lexical scope for + * 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 + * this false assumption. + * + * @param {Object} globalObject + */ +export const setGlobalObjectSymbolUnscopables = globalObject => { + defineProperty( + globalObject, + unscopablesSymbol, + freeze( + assign(create(null), { + set: freeze(() => { + throw new TypeError( + `Cannot set Symbol.unscopables of a Compartment's globalThis`, + ); + }), + enumerable: false, + configurable: false, + }), + ), + ); +}; + /** * setGlobalObjectConstantProperties() * Initializes a new global object using a process similar to ECMA specifications diff --git a/packages/ses/test/test-confinement.js b/packages/ses/test/test-confinement.js index cb478b6d99..6dfa913398 100644 --- a/packages/ses/test/test-confinement.js +++ b/packages/ses/test/test-confinement.js @@ -65,19 +65,16 @@ test('confinement evaluation eval', t => { }); test('confinement evaluation Symbol.unscopables with-statement escape', t => { - t.plan(2); + t.plan(1); + // Give the terminal scope proxy reason to claim that unsafe exists, + // by leaving its shadow on the actual global scope. globalThis.flag = 'unsafe'; const c = new Compartment({ flag: 'safe' }); - t.is(c.evaluate('Symbol.unscopables'), Symbol.unscopables); - // This modification causes a loss of shim fidelity, but not a loss of - // security. - t.is( - c.evaluate('globalThis[Symbol.unscopables] = { flag: true }; flag'), - undefined, - ); + // Known loss of fidility of emulating a proper host: + t.throws(() => c.evaluate('Symbol.unscopables = { flag: true };')); delete globalThis.flag; }); From 0187d1e3532cfc2f052619c46a8a6331a8c15ae8 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Wed, 19 Oct 2022 18:08:42 -0700 Subject: [PATCH 18/19] feat(ses): Revocable evalScope --- packages/ses/src/eval-scope.js | 73 +++++++++++++++++++++++-- packages/ses/src/make-safe-evaluator.js | 9 ++- packages/ses/test/test-eval-scope.js | 12 ++-- packages/ses/test/test-make-evaluate.js | 14 +++-- 4 files changed, 90 insertions(+), 18 deletions(-) diff --git a/packages/ses/src/eval-scope.js b/packages/ses/src/eval-scope.js index 2555072c84..b753ac45e9 100644 --- a/packages/ses/src/eval-scope.js +++ b/packages/ses/src/eval-scope.js @@ -1,6 +1,62 @@ -import { FERAL_EVAL, create, freeze, defineProperties } from './commons.js'; +import { FERAL_EVAL, create, defineProperties, freeze } from './commons.js'; -export const createEvalScope = () => { +import { assert } from './error/assert.js'; + +const { details: d } = assert; + +// We attempt to frustrate stack bumping attacks on the safe evaluator +// (`make-safe-evaluator.js`). +// A stack bumping attack forces an API call to throw a stack overflow +// `RangeError` at an inopportune time. +// The attacker arranges for the stack to be sufficiently deep that the API +// consumes exactly enough stack frames to throw an exception. +// +// For the safe evaluator, an exception thrown between adding and then deleting +// `eval` on `evalScope` could leak the real `eval` to an attacker's lexical +// scope. +// This would be sufficiently disastrous that we guard against it twice. +// First, we delete `eval` from `evalScope` immediately before rendering it to +// the guest program's lexical scope. +// +// If the attacker manages to arrange for `eval` to throw an exception after we +// call `allowNextEvalToBeUnsafe` but before the guest program accesses `eval`, +// it would be able to access `eval` once more in its own code. +// Although they could do no harm with a direct `eval`, they would be able to +// escape to the true global scope with an indirect `eval`. +// +// prepareStack(depth, () => { +// (eval)(''); +// }); +// const unsafeEval = (eval); +// const safeEval = (eval); +// const realGlobal = unsafeEval('globalThis'); +// +// To protect against that case, we also delete `eval` from the `evalScope` in +// a `finally` block surrounding the call to the safe evaluator. +// The only way to reach this case is if `eval` remains on `evalScope` due to +// an attack, so we assume that attack would have have invalided our isolation +// and revoke all future access to the evaluator. +// +// To defeat a stack bumping attack, we must use fewer stack frames to recover +// in that `finally` block than we used in the `try` block. +// We have no reliable guarantees about how many stack frames a block of +// JavaScript will consume. +// Function inlining, tail-call optimization, variations in the size of a stack +// frame, and block scopes may affect the depth of the stack. +// The only number of acceptable stack frames to use in the finally block is +// zero. +// We only use property assignment and deletion in the safe evaluator's +// `finally` block. +// We use `delete evalScope.eval` to withhold the evaluator. +// We assign an envelope object over `evalScopeKit.revoked` to revoke the +// evaluator. +// +// This is why we supply a meaningfully named function for +// `allowNextEvalToBeUnsafe` but do not provide a corresponding +// `revokeAccessToUnsafeEval` or even simply `revoke`. +// These recovery routines are expressed inline in the safe evaluator. + +export const makeEvalScopeKit = () => { const evalScope = create(null); const oneTimeEvalProperties = freeze({ eval: { @@ -13,13 +69,22 @@ export const createEvalScope = () => { }, }); - return { + const evalScopeKit = { evalScope, - allowNextEvalToBeUnsafe: () => { + allowNextEvalToBeUnsafe() { + if (evalScopeKit.revoked !== null) { + // eslint-disable-next-line @endo/no-polymorphic-call + assert.fail( + d`a handler did not reset allowNextEvalToBeUnsafe ${this.revoked.err}`, + ); + } // Allow next reference to eval produce the unsafe FERAL_EVAL. // We avoid defineProperty because it consumes an extra stack frame taming // its return value. defineProperties(evalScope, oneTimeEvalProperties); }, + revoked: null, }; + + return evalScopeKit; }; diff --git a/packages/ses/src/make-safe-evaluator.js b/packages/ses/src/make-safe-evaluator.js index f489eaa902..1757e651c3 100644 --- a/packages/ses/src/make-safe-evaluator.js +++ b/packages/ses/src/make-safe-evaluator.js @@ -4,7 +4,7 @@ import { apply, freeze } from './commons.js'; import { strictScopeTerminator } from './strict-scope-terminator.js'; import { createSloppyGlobalsScopeTerminator } from './sloppy-globals-scope-terminator.js'; -import { createEvalScope } from './eval-scope.js'; +import { makeEvalScopeKit } from './eval-scope.js'; import { applyTransforms, mandatoryTransforms } from './transforms.js'; import { makeEvaluate } from './make-evaluate.js'; import { assert } from './error/assert.js'; @@ -31,7 +31,8 @@ export const makeSafeEvaluator = ({ const scopeTerminator = sloppyGlobalsMode ? createSloppyGlobalsScopeTerminator(globalObject) : strictScopeTerminator; - const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + const evalScopeKit = makeEvalScopeKit(); + const { evalScope } = evalScopeKit; const evaluateContext = freeze({ evalScope, @@ -67,7 +68,8 @@ export const makeSafeEvaluator = ({ ]); // Allow next reference to eval produce the unsafe FERAL_EVAL. - allowNextEvalToBeUnsafe(); + // eslint-disable-next-line @endo/no-polymorphic-call + evalScopeKit.allowNextEvalToBeUnsafe(); let err; try { @@ -95,6 +97,7 @@ export const makeSafeEvaluator = ({ // variable resolution via the scopeHandler, and throw an error with // diagnostic info including the thrown error if any from evaluating the // source code. + evalScopeKit.revoked = { err }; // TODO A GOOD PLACE TO PANIC(), i.e., kill the vat incarnation. // See /~https://github.com/Agoric/SES-shim/issues/490 // eslint-disable-next-line @endo/no-polymorphic-call diff --git a/packages/ses/test/test-eval-scope.js b/packages/ses/test/test-eval-scope.js index fd3681d07f..54f1249ae5 100644 --- a/packages/ses/test/test-eval-scope.js +++ b/packages/ses/test/test-eval-scope.js @@ -1,5 +1,5 @@ import test from 'ava'; -import { createEvalScope } from '../src/eval-scope.js'; +import { makeEvalScopeKit } from '../src/eval-scope.js'; // The original unsafe untamed eval function, which must not escape. // Sample at module initialization time, which is before lockdown can @@ -10,11 +10,12 @@ const FERAL_EVAL = eval; test('evalScope - is not created with eval exposed', t => { t.plan(2); - const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + const evalScopeKit = makeEvalScopeKit(); + const { evalScope } = evalScopeKit; t.is(Reflect.has(evalScope, 'eval'), false); - allowNextEvalToBeUnsafe(); + evalScopeKit.allowNextEvalToBeUnsafe(); t.is(Reflect.has(evalScope, 'eval'), true); }); @@ -22,11 +23,12 @@ test('evalScope - is not created with eval exposed', t => { test('evalScope - getting eval removes it from evalScope', t => { t.plan(5); - const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + const evalScopeKit = makeEvalScopeKit(); + const { evalScope } = evalScopeKit; t.is(Reflect.has(evalScope, 'eval'), false); - allowNextEvalToBeUnsafe(); + evalScopeKit.allowNextEvalToBeUnsafe(); t.is(Reflect.has(evalScope, 'eval'), true); diff --git a/packages/ses/test/test-make-evaluate.js b/packages/ses/test/test-make-evaluate.js index efe066ac97..c79d99bd48 100644 --- a/packages/ses/test/test-make-evaluate.js +++ b/packages/ses/test/test-make-evaluate.js @@ -2,7 +2,7 @@ import test from 'ava'; import { apply, freeze } from '../src/commons.js'; import { makeEvaluate } from '../src/make-evaluate.js'; import { strictScopeTerminator } from '../src/strict-scope-terminator.js'; -import { createEvalScope } from '../src/eval-scope.js'; +import { makeEvalScopeKit } from '../src/eval-scope.js'; const makeObservingProxy = target => { const ops = []; @@ -36,7 +36,8 @@ test('makeEvaluate - optimizer', t => { ); const scopeTerminator = strictScopeTerminator; - const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + const evalScopeKit = makeEvalScopeKit(); + const { evalScope } = evalScopeKit; const evaluate = makeEvaluate( freeze({ scopeTerminator, globalObject, globalLexicals, evalScope }), @@ -48,7 +49,7 @@ test('makeEvaluate - optimizer', t => { globalObjectOps.length = 0; globalLexicalsOps.length = 0; - allowNextEvalToBeUnsafe(); + evalScopeKit.allowNextEvalToBeUnsafe(); const result = apply(evaluate, globalObject, [`!foo && bar && baz`]); @@ -64,15 +65,16 @@ test('makeEvaluate - strict-mode', t => { const globalLexicals = Object.create(null); const scopeTerminator = strictScopeTerminator; - const { evalScope, allowNextEvalToBeUnsafe } = createEvalScope(); + const evalScopeKit = makeEvalScopeKit(); + const { evalScope } = evalScopeKit; const evaluate = makeEvaluate( freeze({ scopeTerminator, globalObject, globalLexicals, evalScope }), ); - allowNextEvalToBeUnsafe(); + evalScopeKit.allowNextEvalToBeUnsafe(); t.throws(() => apply(evaluate, globalObject, [`foo = 42`])); - allowNextEvalToBeUnsafe(); + evalScopeKit.allowNextEvalToBeUnsafe(); t.throws(() => apply(evaluate, globalObject, [`with({}) {}`])); }); From 3d022b1e1b34ff9b86cf6af236c499bfbe291298 Mon Sep 17 00:00:00 2001 From: Kris Kowal Date: Thu, 20 Oct 2022 17:53:36 -0700 Subject: [PATCH 19/19] fix(ses): Protect necessary eval admission before it has been admitted --- packages/ses/src/make-safe-evaluator.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ses/src/make-safe-evaluator.js b/packages/ses/src/make-safe-evaluator.js index 1757e651c3..2ef1afe05c 100644 --- a/packages/ses/src/make-safe-evaluator.js +++ b/packages/ses/src/make-safe-evaluator.js @@ -67,12 +67,12 @@ export const makeSafeEvaluator = ({ mandatoryTransforms, ]); - // Allow next reference to eval produce the unsafe FERAL_EVAL. - // eslint-disable-next-line @endo/no-polymorphic-call - evalScopeKit.allowNextEvalToBeUnsafe(); - let err; try { + // Allow next reference to eval produce the unsafe FERAL_EVAL. + // eslint-disable-next-line @endo/no-polymorphic-call + evalScopeKit.allowNextEvalToBeUnsafe(); + // Ensure that "this" resolves to the safe global. return apply(evaluate, globalObject, [source]); } catch (e) {