Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
Oct 21, 2022

Conversation

kumavis
Copy link
Member

@kumavis kumavis commented Sep 21, 2022

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

@kumavis kumavis marked this pull request as ready for review September 22, 2022 04:23
@kumavis kumavis changed the title refactor(ses): introduce evalScope refactor(ses): multilayered scope stack to improved auditablitity Sep 22, 2022
Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
@kumavis
Copy link
Member Author

kumavis commented Sep 23, 2022

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
Copy link
Member Author

kumavis commented Sep 23, 2022

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm we could do that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@kumavis kumavis Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

@kumavis kumavis Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@erights erights Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mhofman mhofman Oct 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'); } }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appended commit fc8f84b

@kumavis
Copy link
Member Author

kumavis commented Sep 26, 2022

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
Copy link
Member

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

@kumavis kumavis mentioned this pull request Sep 27, 2022
@mhofman mhofman self-requested a review September 28, 2022 02:34
packages/ses/test/test-scope.js Show resolved Hide resolved
packages/ses/test/test-scope.js Outdated Show resolved Hide resolved
@mhofman mhofman self-requested a review September 28, 2022 03:07
Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intermediate review

packages/ses/src/eval-scope.js Show resolved Hide resolved
packages/ses/src/make-safe-evaluator.js Outdated Show resolved Hide resolved
@kriskowal
Copy link
Member

@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
Copy link
Contributor

erights commented Sep 29, 2022

@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
Copy link
Contributor

mhofman commented Sep 29, 2022

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

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kriskowal force-pushed the refactor/quadruple-backflip branch 2 times, most recently from e0c0d0e to 79e77b9 Compare October 20, 2022 01:12
@kriskowal
Copy link
Member

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

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Revocable evalScope" commit LGTM

Comment on lines 8 to 9
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 16 to 19
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? ;)

// 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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice communicating the error through the revocation channel!

kriskowal and others added 19 commits October 20, 2022 17:54
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants