fix(ses): Prevent hypothetical stack bumping to get unsafe eval
Fixes
MODIFY
packages/ses/src/make-safe-evaluator.js
packages/ses/src/make-safe-evaluator.js
@@ -35,13 +35,13 @@ export const makeSafeEvaluator = ({
sloppyGlobalsMode = false,
knownScopeProxies = new WeakSet(),
} = {}) => {
- const {
- scopeHandler,
- admitOneUnsafeEvalNext,
- resetOneUnsafeEvalNext,
- } = createScopeHandler(globalObject, globalLexicals, {
+ const { scopeHandler, scopeController } = createScopeHandler(
+ globalObject,
+ globalLexicals,
+ {
sloppyGlobalsMode,
- });
+ },
+ );
const { proxy: scopeProxy, revoke: revokeScopeProxy } = proxyRevocable(
immutableObject,
scopeHandler,
@@ -76,7 +76,7 @@ export const makeSafeEvaluator = ({
mandatoryTransforms,
]);
- admitOneUnsafeEvalNext();
+ scopeController.allowNextEvalToBeUnsafe = true;
let err;
try {
// Ensure that "this" resolves to the safe global.
@@ -86,13 +86,15 @@ export const makeSafeEvaluator = ({
err = e;
throw e;
} finally {
- if (resetOneUnsafeEvalNext()) {
+ const unsafeEvalWasStillExposed = scopeController.allowNextEvalToBeUnsafe;
+ scopeController.allowNextEvalToBeUnsafe = false;
+ if (unsafeEvalWasStillExposed) {
// Barring a defect in the SES shim, the scope proxy 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 our call to
- // `admitOneUnsafeEvalNext()` and before `evaluate` calls `eval`
- // internally.
+ // A defect in the SES shim could throw an exception after we set
+ // `scopeController.allowNextEvalToBeUnsafe` 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.

MODIFY
packages/ses/src/scope-handler.js
packages/ses/src/scope-handler.js
@@ -12,6 +12,7 @@ import {
objectHasOwnProperty,
reflectGet,
reflectSet,
+ seal,
} from './commons.js';
import { assert } from './error/assert.js';
@@ -59,17 +60,15 @@ export const createScopeHandler = (
// 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.
- let allowNextEvalToBeUnsafe = false;
-
- const admitOneUnsafeEvalNext = () => {
- allowNextEvalToBeUnsafe = true;
- };
-
- const resetOneUnsafeEvalNext = () => {
- const wasSet = allowNextEvalToBeUnsafe;
- allowNextEvalToBeUnsafe = false;
- return wasSet;
+ // 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) {
@@ -82,9 +81,9 @@ export const createScopeHandler = (
// the 'with' context.
if (prop === 'eval') {
// test that it is true rather than merely truthy
- if (allowNextEvalToBeUnsafe === true) {
+ if (scopeController.allowNextEvalToBeUnsafe === true) {
// revoke before use
- allowNextEvalToBeUnsafe = false;
+ scopeController.allowNextEvalToBeUnsafe = false;
return FERAL_EVAL;
}
// fall through
@@ -165,7 +164,7 @@ export const createScopeHandler = (
// !!!!! WARNING: DANGER ZONE !!!!!!
return (
sloppyGlobalsMode ||
- (allowNextEvalToBeUnsafe && prop === 'eval') ||
+ (scopeController.allowNextEvalToBeUnsafe && prop === 'eval') ||
prop in globalLexicals ||
prop in globalObject ||
prop in globalThis
@@ -206,8 +205,7 @@ export const createScopeHandler = (
);
return {
- admitOneUnsafeEvalNext,
- resetOneUnsafeEvalNext,
+ scopeController,
scopeHandler,
};
};

MODIFY
packages/ses/test/test-scope-handler.js
packages/ses/test/test-scope-handler.js
@@ -89,16 +89,15 @@ test('scopeHandler - has trap guards eval with its life', t => {
const {
scopeHandler: handler,
- resetOneUnsafeEvalNext,
- admitOneUnsafeEvalNext,
+ scopeController: controller,
} = createScopeHandler(globalObject);
- admitOneUnsafeEvalNext();
+ controller.allowNextEvalToBeUnsafe = true;
guardDown = true;
t.is(handler.has(null, 'eval'), true);
t.is(handler.get(null, 'eval'), FERAL_EVAL);
- resetOneUnsafeEvalNext();
+ 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);
@@ -200,23 +199,22 @@ test('scopeHandler - set trap', t => {
delete globalThis.bar;
});
-test('scopeHandler - get trap - reset allow next unsafe eval', t => {
+test('scopeHandler - get trap - clear allow next unsafe eval', t => {
t.plan(7);
const globalObject = { eval: {} };
const {
scopeHandler: handler,
- resetOneUnsafeEvalNext,
- admitOneUnsafeEvalNext,
+ scopeController: controller,
} = createScopeHandler(globalObject);
- t.is(resetOneUnsafeEvalNext(), false);
+ t.is(controller.allowNextEvalToBeUnsafe, false);
t.is(handler.get(null, 'eval'), globalObject.eval);
t.is(handler.get(null, 'eval'), globalObject.eval); // repeat
- admitOneUnsafeEvalNext();
+ controller.allowNextEvalToBeUnsafe = true;
t.is(handler.get(null, 'eval'), FERAL_EVAL);
- t.is(resetOneUnsafeEvalNext(), false);
+ t.is(controller.allowNextEvalToBeUnsafe, false);
t.is(handler.get(null, 'eval'), globalObject.eval);
t.is(handler.get(null, 'eval'), globalObject.eval); // repeat
});

fix(ses): Prevent hypothetical stack bumping to get unsafe eval

Fixes #956