refactor(ses)!: Divide safe evaluator scope stack into four layers #1293

kumavis · 2022-09-21T21:40:10Z

multilayered scope stack, or "the four backflips of the apotheosis" 🏇 🤸 🧙

description

instead of with(complicatedScopeProxy){ ... }, we now have the following scope stack (outermost to innermost, lowest takes precedence):

scopeTerminator {security, fidelity} (always return undefined, if sloppyGlobalsMode is true, sets happen on globalObject)
globalObject {fidelity} (bare)
globalLexicals {fidelity} (bare)
evalScope {security} (obj with FERAL_EVAL exposed as eval only once)

behavioural changes

this refactor introduces some changed behavior:

globalLexicals object can be leaked if a special lexical is crafted (should not be craftable by evaluated code)
setting a correctly formatted Symbol.unscopables property on the evaluator globalObject will cause the globals configured by Symbol.unscopables to not be in scope. this is a shim fidelity issue and not a security issue as the scopeTerminator is not affected.
fixes a bug where sloppyGlobal assignment is allowed in non-sloppyGlobalsMode when a property does not exist on the compartment but does exist on the realm globalThis

other notes

attempts to adapt test-scope-handler test to use makeSafeEvaluator and test the whole scope stack, but its not a great match and a lot had to be changed
adds a failing test test-scope-handler-pollution because the test's prototype pollution broke the makeSafeEvaluator internals
changed a couple tests to match behavioral changes

things to revisit after this PR

leak of presence of properties on globalThis with strict-scope-terminator Revisit behavior of has proxy trap in safe evaluator’s terminal scope #1305
consider adding another lexical layer to differentiate between moduleScope and globalLexicals to limit impact of a leak Module shims lexicals leak internal live bindings object #912

packages/ses/src/make-safe-evaluator.js

packages/ses/test/test-scope-handler.js

mhofman

Some preliminary observations.

I'm having a hard time reviewing the tests because of the combined rename + modifications past the diff threshold. Would appreciate a minimal rename and separate modification to the tests

packages/ses/src/make-safe-evaluator.js

packages/ses/src/scope-terminator.js

kumavis · 2022-09-23T00:05:42Z

my biggest regret is that the diff is a bit difficult to review, but I think the result is easy to review and audit

@mhofman the test-scope.js file (previously test-scope-handler.js) was changed pretty dramatically bc it tested behavior about the full scope stack but in very low level ways. I did my best to rework the tests against the safeEvaluator level but observing scope behavior at this level meant different checks and results. The new test file is somewhat haphazard and very different from the original so reviewing a diff from the previous test is perhaps not particularly useful. However I still thought having a test tracking the behavior was useful. Additionally all observed changes to behavior are enumerated in the top comment of this pr.

kumavis · 2022-09-23T03:10:14Z

split the scopeTerminator into a reuseable strict and per-evaluator sloppyGlobals version

packages/ses/src/make-safe-evaluator.js

packages/ses/src/strict-scope-terminator.js

naugtur · 2022-09-26T14:29:46Z

packages/ses/test/test-confinement.js

  t.is(
    c.evaluate('globalThis[Symbol.unscopables] = { flag: true }; flag'),
-    'safe',
+    undefined,


What's preventing us from adding a noop setter on globalThis[Symbol.unscopables] ?
Would that be too much intervention?

hmm we could do that

I think it’s worth the loss in fidelity to prevent leaking the terminal scope proxy. What do you think, @erights?

there is no known way to leak the scope terminator proxy. @naugtur's suggestion just prevents the "compartment global var not in scope" fidelity issue. from the pr top comment:

setting a correctly formatted Symbol.unscopables property on the evaluator globalObject will cause the therein configured globals to not be in scope. this is a shim fidelity issue and not a security issue as the scopeTerminator is not affected.

What loss of fidelity are we talking about? The

noop setter on globalThis[Symbol.unscopables] ?

?
And in exchange we would no longer leak a scope proxy? Yes, worth it!

What loss of fidelity are we talking about?

setting a correctly formatted Symbol.unscopables property on the evaluator globalObject will cause the globals configured by Symbol.unscopables to not be in scope. this is a shim fidelity issue and not a security issue as the scopeTerminator is not affected.

example:

globalThis.xyz = 123 xyz // => 123 globalThis[Symbol.unscopables] = { xyz: true } xyz //=> undefined

And in exchange we would no longer leak a scope proxy? Yes, worth it!

there is no known way to leak a scope proxy in this PR's implementation

I reread the conversation and understand that my previous response did not make sense.

I think the fidelity issue here cuts both ways, with no choice being strictly better fidelity than the other. The fidelity issue is also minor, as it is unlikely to be encountered by accident, and it is unlikely to be exploitable by an attacker. When faced with such a choice, the next question is: Which lack of fidelity fails safe?

I think giving the global object a Symbol.unscopables non-configurable accessor without a setter, or whose setter always throws, means that any program that does not cause an error to be thrown will work the same under the shim and under a native implementation.

Unresolving.

If we decide to install a Symbol.unscopables on the globalThis, it should be a configurable (to allow a program to change it through defineProperty if really needed) inert accessor (to avoid assignment errors). I don't believe it should be a non-writable undefined value, or a throwing setter, as programs with such assignments would end up throwing in strict mode.

I have erred in my judgement, and have now come around to @erights's suggestion that we install a property with { configurable: false, enumerable: false, set() { assert.fail('Some meaningful message'); } }

Appended commit fc8f84b

kumavis · 2022-09-26T22:10:12Z

I'm having a hard time reviewing the tests because of the combined rename + modifications past the diff threshold. Would appreciate a minimal rename and separate modification to the tests

@mhofman makes sense - though im not sure how to accomplish this. can someone provide some suggestions on how to improve the legibility of the diff? my gitfoo is not amazing. and i dont think its possible to make a minimal modification to the test-scope.js tests

kriskowal · 2022-09-26T23:13:16Z

@kumavis One way to satisfy @mhofman’s concern would be to either create or refer to tests that cover all the security behaviors of the collective scopes that pass both before and after this refactor, since the unit tests cut too closely to the particulars of the implementations before and after. If that requires new tests, we could land them in another PR before so we’re sure they passed before this change, and also highlight any deliberate behavior changes in this PR.

packages/ses/test/test-scope.js

packages/ses/src/make-safe-evaluator.js

packages/ses/test/test-scope.js

mhofman

Intermediate review

packages/ses/src/eval-scope.js

packages/ses/src/make-safe-evaluator.js

packages/ses/src/make-evaluate-factory.js

kriskowal · 2022-09-29T20:50:34Z

@mhofman @erights I’d like to land this without the fifth with block, without fixing the global lexicals leak (#912), and then proceed to work on that fix myself after this has landed.

erights · 2022-09-29T20:52:54Z

@mhofman @erights I’d like to land this without the fifth with block, without fixing the global lexicals leak (#912), and then proceed to work on that fix myself after this has landed.

Fine with me.

mhofman · 2022-09-29T20:56:02Z

Since the leak is no worse than what we have today, find by me too.

erights

While I did not spot anything alarming, I did not give it a careful enough review to approve it on that basis alone. However, it's gotten good examination that I've followed and two approvals. I'm happy to see it merge on that basis, and the only way I can withdraw my "Request changes" is to approve.

So I approve. Good improvements! Onward!

erights · 2022-10-19T01:47:56Z

packages/ses/src/make-evaluate.js

+
+/**
+ * buildOptimizer()
+ * Given an array of indentifier, the optimizer returns a `const` declaration


Suggested change

* Given an array of indentifier, the optimizer returns a `const` declaration

* Given an array of identifiers, the optimizer returns a `const` declaration

erights · 2022-10-19T01:49:16Z

packages/ses/src/make-evaluate.js

+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


Suggested change

// Use 'this' to avoid going through the scope proxy, which is unecessary

// Use 'this' to avoid going through the scope proxy, which is unnecessary

erights · 2022-10-19T01:57:58Z

packages/ses/src/make-safe-evaluator.js

@@ -104,7 +106,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();


Wondering if we should neuter the evaluator in this case? Maybe something like replacing allowNextEvalToBeUnsafe with a throwing function?

I like that. Unless there's something surprising, I'd say that's worth doing.

kriskowal · 2022-10-20T01:12:51Z

@erights I’ve appended a commit, 79e77b9, that makes the evalScope revocable. I chose to use a property of the evalScopeKit to communicate the revocation bit (and a wrapper object to guard against throw null) for reasons I explain in a new comment. I’m asking for a review of just this change.

erights

The "Revocable evalScope" commit LGTM

erights · 2022-10-20T03:15:16Z

packages/ses/src/eval-scope.js

+  // In makeSafeEvaluator, we ensure that we consume at least as many stack
+  // frames when granting access to FERAL_EVAL than we consume when we are


Suggested change

// In makeSafeEvaluator, we ensure that we consume at least as many stack

// frames when granting access to FERAL_EVAL than we consume when we are

// In makeSafeEvaluator, we ensure that we consume at least as many stack

// frames when granting access to FERAL_EVAL as we consume when we are

erights · 2022-10-20T03:18:34Z

packages/ses/src/eval-scope.js

+  // In this case, there is a plausible but very narrow window of opportunity
+  // between defining `oneTimeEvalProperties` on the `evalScope` and when the
+  // confined program reaches for the eval property on the `evalScope` through
+  // a lexical reference where a program could induce a RangeError that would


Suggested change

// In this case, there is a plausible but very narrow window of opportunity

// between defining `oneTimeEvalProperties` on the `evalScope` and when the

// confined program reaches for the eval property on the `evalScope` through

// a lexical reference where a program could induce a RangeError that would

// In this case, there is a plausible but very narrow window of

// opportunity---between defining `oneTimeEvalProperties` on the `evalScope` and when the

// confined program reaches for the eval property on the `evalScope` through

// a lexical reference---where a program could induce a RangeError that would

erights · 2022-10-20T03:20:43Z

packages/ses/src/eval-scope.js

+  // between defining `oneTimeEvalProperties` on the `evalScope` and when the
+  // confined program reaches for the eval property on the `evalScope` through
+  // a lexical reference where a program could induce a RangeError that would
+  // prevent the program from reaching `delete evalScope.eval` either here or


Suggested change

// prevent the program from reaching `delete evalScope.eval` either here or

// prevent the program from reaching `delete evalScope.eval`, either here or

Even with the extra punctuation, this is an amazing run-on sentence. Better to rephrase using more smaller sentences ;)

erights · 2022-10-20T03:22:15Z

packages/ses/src/eval-scope.js

+  // confined program reaches for the eval property on the `evalScope` through
+  // a lexical reference where a program could induce a RangeError that would
+  // prevent the program from reaching `delete evalScope.eval` either here or
+  // in the fallback position in `makeSafeEvaluator` where we attempt to delete


Suggested change

// in the fallback position in `makeSafeEvaluator` where we attempt to delete

// in the fallback position in `makeSafeEvaluator`, where we attempt to delete

How long is that sentence? ;)

erights · 2022-10-20T03:30:03Z

packages/ses/src/eval-scope.js

+  // it again in the finally clause.
+  // By using `delete` and simple property assignment for
+  // `evalScopeKit.revoked`, we avoid pushing to the stack while withdrawing
+  // access to the `FERAL_EVAL`.


Worth noting the non-guaranteed implementation properties we assume in this reasoning that may well be wrong on particular platforms:

That if stack A has more frames than stack B, where none of these frames are unusual, then stack B will not exceed the stack if stack A did not. May be false depending on the sizes of each frame.

That function calls in the semantics of the language is an adequate predictor of stack frames. May be false when some of these function calls are inlined or otherwise optimized away --- as we know happens with JITs.

Have we reexamined our frame-count reasoning in light of the possibility of implementation tail-calling? The ECMAScript std mandates it and JSC actually implements it.

The reason we're ok with this fragile reasoning is that it is only about a backstop that we'd need if all the other relevant mechanisms failed. Nothing will protect against any possible bug we overlooked. Rather, we're only trying for defense in depth, so that multiple improbably things would need to go wrong to create an exploitable vulnerability.

erights · 2022-10-20T03:36:27Z

packages/ses/src/make-safe-evaluator.js

@@ -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 };


Very nice communicating the error through the revocation channel!

Per #1329, we will no longer attempt to defend against poorly-vetted shims that generally break property descriptors.

*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.

…e module

Rewrite tests to verify functionality of evaluate instead asserting the string output of the factory.

*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.

kumavis commented Sep 21, 2022

View reviewed changes

packages/ses/src/make-safe-evaluator.js Outdated Show resolved Hide resolved

kriskowal reviewed Sep 21, 2022

View reviewed changes

packages/ses/test/test-scope-handler.js Outdated Show resolved Hide resolved

kumavis marked this pull request as ready for review September 22, 2022 04:23

kumavis requested review from mhofman, erights and naugtur September 22, 2022 04:24

kumavis changed the title ~~refactor(ses): introduce evalScope~~ refactor(ses): multilayered scope stack to improved auditablitity Sep 22, 2022

mhofman reviewed Sep 22, 2022

View reviewed changes

packages/ses/src/make-safe-evaluator.js Outdated Show resolved Hide resolved

packages/ses/src/scope-terminator.js Outdated Show resolved Hide resolved

packages/ses/src/scope-terminator.js Outdated Show resolved Hide resolved

mhofman reviewed Sep 22, 2022

View reviewed changes

packages/ses/src/scope-terminator.js Outdated Show resolved Hide resolved

naugtur reviewed Sep 26, 2022

View reviewed changes

packages/ses/src/make-safe-evaluator.js Outdated Show resolved Hide resolved

naugtur reviewed Sep 26, 2022

View reviewed changes

packages/ses/src/strict-scope-terminator.js Show resolved Hide resolved

naugtur reviewed Sep 26, 2022

View reviewed changes

kumavis mentioned this pull request Sep 27, 2022

Add general scope tests #1302

Merged

mhofman self-requested a review September 28, 2022 02:34

kumavis commented Sep 28, 2022

View reviewed changes

packages/ses/test/test-scope.js Show resolved Hide resolved

kumavis commented Sep 28, 2022

View reviewed changes

packages/ses/test/test-scope.js Show resolved Hide resolved

kumavis commented Sep 28, 2022

View reviewed changes

packages/ses/src/make-safe-evaluator.js Outdated Show resolved Hide resolved

mhofman reviewed Sep 28, 2022

View reviewed changes

packages/ses/test/test-scope.js Show resolved Hide resolved

packages/ses/test/test-scope.js Outdated Show resolved Hide resolved

mhofman self-requested a review September 28, 2022 03:07

kumavis commented Sep 28, 2022

View reviewed changes

packages/ses/test/test-scope.js Outdated Show resolved Hide resolved

mhofman reviewed Sep 28, 2022

View reviewed changes

packages/ses/src/eval-scope.js Show resolved Hide resolved

packages/ses/src/make-safe-evaluator.js Outdated Show resolved Hide resolved

mhofman reviewed Sep 28, 2022

View reviewed changes

packages/ses/src/make-evaluate-factory.js Outdated Show resolved Hide resolved

kriskowal mentioned this pull request Sep 29, 2022

Revisit behavior of has proxy trap in safe evaluator’s terminal scope #1305

Open

erights approved these changes Oct 19, 2022

View reviewed changes

kriskowal force-pushed the refactor/quadruple-backflip branch 2 times, most recently from e0c0d0e to 79e77b9 Compare October 20, 2022 01:12

kriskowal requested a review from erights October 20, 2022 01:12

erights approved these changes Oct 20, 2022

View reviewed changes

kriskowal and others added 19 commits October 20, 2022 17:54

fix(ses): Typo in compartmentEvaluate

d66db7a

fix(ses): Typo in scope-constants

a4ee1ea

test(ses): Remove property prototype pollution test

eb50902

Per #1329, we will no longer attempt to defend against poorly-vetted shims that generally break property descriptors.

refactor(ses)!: Divide scope proxy into four layers

37c4b4a

*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.

test(ses): Remove obsolete known scope proxies tests

ec21beb

test(ses): Add scope terminator tests

8eff012

test(ses): Test evaluator evalScope

a3df403

test(ses): Adjust expectations of fidelity for global unscopables

2b202f7

test(ses): Include more with blocks in expected shape of magic lines

dd6ea0b

test(ses): Adjust expectations about scope behaviors

53958d9

test(ses): Remove obsolete scope handler tests

3d9da89

refactor(ses): Colocate logic of exposing FERAL_EVAL inside eval-scop…

f70d046

…e module

refactor(ses): Divide scope optimizer

83873f6

refactor(ses): introduce makeEvaluate

32b42ea

refactor(ses): remove makeEvaluateFactory remnants

eb4a2fd

Rewrite tests to verify functionality of evaluate instead asserting the string output of the factory.

chore(ses): Update comments of makeEvaluate to reflect reality

e8f5889

feat(ses): Revocable evalScope

0187d1e

fix(ses): Protect necessary eval admission before it has been admitted

3d022b1

kriskowal force-pushed the refactor/quadruple-backflip branch from 79e77b9 to 3d022b1 Compare October 21, 2022 00:54

kriskowal merged commit 174749b into master Oct 21, 2022

kriskowal deleted the refactor/quadruple-backflip branch October 21, 2022 01:04

This was referenced Oct 26, 2022

Proposal: split scope handlers into multiple with blocks #905

Closed

Scope proxy leak allow constructive access to global lexicals #904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ses)!: Divide safe evaluator scope stack into four layers #1293

refactor(ses)!: Divide safe evaluator scope stack into four layers #1293

kumavis commented Sep 21, 2022 •

edited

Loading

mhofman left a comment

kumavis commented Sep 23, 2022

kumavis commented Sep 23, 2022

naugtur Sep 26, 2022

kumavis Sep 26, 2022

kriskowal Sep 26, 2022

kumavis Sep 26, 2022 •

edited

Loading

erights Sep 26, 2022

kumavis Sep 26, 2022 •

edited

Loading

erights Oct 7, 2022 •

edited

Loading

mhofman Oct 10, 2022 •

edited

Loading

mhofman Oct 13, 2022

kriskowal Oct 14, 2022

kumavis commented Sep 26, 2022

kriskowal commented Sep 26, 2022

mhofman left a comment

kriskowal commented Sep 29, 2022

erights commented Sep 29, 2022

mhofman commented Sep 29, 2022

erights left a comment

erights Oct 19, 2022

erights Oct 19, 2022

erights Oct 19, 2022

kriskowal commented Oct 20, 2022

erights left a comment

erights Oct 20, 2022

erights Oct 20, 2022

erights Oct 20, 2022

erights Oct 20, 2022

erights Oct 20, 2022

erights Oct 20, 2022

	* Given an array of indentifier, the optimizer returns a `const` declaration
	* Given an array of identifiers, the optimizer returns a `const` declaration

	// Use 'this' to avoid going through the scope proxy, which is unecessary
	// Use 'this' to avoid going through the scope proxy, which is unnecessary

		// In makeSafeEvaluator, we ensure that we consume at least as many stack
		// frames when granting access to FERAL_EVAL than we consume when we are

	// prevent the program from reaching `delete evalScope.eval` either here or
	// prevent the program from reaching `delete evalScope.eval`, either here or

	// in the fallback position in `makeSafeEvaluator` where we attempt to delete
	// in the fallback position in `makeSafeEvaluator`, where we attempt to delete

refactor(ses)!: Divide safe evaluator scope stack into four layers #1293

refactor(ses)!: Divide safe evaluator scope stack into four layers #1293

Conversation

kumavis commented Sep 21, 2022 • edited Loading

multilayered scope stack, or "the four backflips of the apotheosis" 🏇 🤸 🧙

description

behavioural changes

other notes

things to revisit after this PR

mhofman left a comment

Choose a reason for hiding this comment

kumavis commented Sep 23, 2022

kumavis commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

erights Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

mhofman Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis commented Sep 26, 2022

kriskowal commented Sep 26, 2022

mhofman left a comment

Choose a reason for hiding this comment

kriskowal commented Sep 29, 2022

erights commented Sep 29, 2022

mhofman commented Sep 29, 2022

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Oct 20, 2022

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis commented Sep 21, 2022 •

edited

Loading

kumavis Sep 26, 2022 •

edited

Loading

kumavis Sep 26, 2022 •

edited

Loading

erights Oct 7, 2022 •

edited

Loading

mhofman Oct 10, 2022 •

edited

Loading