refactor(ses): multilayered scope stack to improved auditablitity

[kumavis]

multilayered scope stack, or "the four backflips of the apotheosis" :horse_racing: :cartwheeling: :mage:

description

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

• scopeTerminator (always return undefined, if sloppyGlobalsMode is true, sets happen on globalObject)
• globalObject (bare)
• globalLexicals (bare)
• evalScope (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 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.

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

[kumavis]

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]

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

[kumavis]

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]

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

[kriskowal]

@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]

@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]

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

[kriskowal]

@erights @mhofman This should be ready for review again. We bifurcated the optimizer at today’s SES meeting and the remaining follow-ups have tracking issues.

[erights]

adds a failing test test-scope-handler-pollution because the test's prototype pollution broke the makeSafeEvaluator internals

This sounds like a severe bug and a possible vulnerability to code (like vetted shims) that runs between repair and lockdown. Is it?

[kriskowal]

I’ve reviewed @mhofman’s 3 amendments and am convinced they’re a desirable refactor with no behavior change.

[kriskowal]

adds a failing test test-scope-handler-pollution because the test's prototype pollution broke the makeSafeEvaluator internals

This sounds like a severe bug and a possible vulnerability to code (like vetted shims) that runs between repair and lockdown. Is it?

I add a change that fixed the test by using a null prototype on a property descriptor. We don’t do this nearly enough to pervasively avoid Object.prototype.value pollution from poorly-vetted shims. Filed https://github.com/endojs/endo/issues/1329

[mhofman]

The one in safe-evaluator.js is the backup one in case something went extremely wrong and it didn't get deleted.

[kriskowal]

I'm missing something very basic. Why does delete evalScope.eval appear in both eval-scope.js and make-safe-evalutor.js ? Surely this should be done in only one place, yes?

The occurrence in evalScope is the one we’re sure will work. The one in makeSafeEvaluator should be redundant and I can’t imagine a scenario where it would do anything except delete the property after it has already been deleted, but is here as a hedge in case there’s a way for the evalScope.eval getter manages to throw before it deletes itself.

[kriskowal]

And, notably, we’re using delete evalScope.eval instead of a meaningfully named method of the “eval scope kit” in order to avoid consuming a stack frame, in order to frustrate stack bumping.

[kriskowal]

@erights I’ve appended a commit, 79e77b9fb, 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.