feat: add evalTaming option #961

Jack-Works · 2021-12-18T06:08:27Z

This PR adds a new option evalTaming which defaults to "safeEval" (current behavior).

I introduced a new behavior "unsafeEval" and "noEval".

This option is for the environment that does not allow eval in the production mode but allows it in the development mode for webpack eval-source-map.
The current behavior will replace the eval constructor and cause the webpack bundle to fail to load.

I have documented this new option and warned any users do not use it unless they have the same situation as us.

erights

Hi @Jack-Works , very nice! Some questions:

IIUC, what you really care about is the Function and eval of the start compartment. Under the 'unsafe' setting you want these left as is. But I don't understand what you intend for the evaluate method of compartments, nor for the per-non-start-compartment setting of eval and Function.

For production use, IIUC, you want this to actually be safe if in an environment where evaluation is suppressed by other means, like CSP. However, in that environment specifically, how would it make a difference whether eval and Function were tamed or not? In fact, to better support that environment, should this PR instead have a three-settings option, perhaps with names like the following?

'noEval' -- all evaluators are disabled, achieving no-eval safety with or without CSP. This would be compat with the CSP no-eval option, but would be redundant belt and suspenders, in case the CSP was, for example, mistakenly changed under maintenance.
'safeEval' -- status quo
'unsafeEval' -- supports the development mode you're concerned about, by leaving (only?) the start compartment eval and Function untamed.

I'm hitting "Request changes" but I'm really requesting dialog ;). I'd like your reaction to these questions before completing the review. Thanks!

packages/ses/src/lockdown-shim.js

packages/ses/src/global-object.js

Jack-Works · 2021-12-19T02:34:22Z

'noEval' -- all evaluators are disabled, achieving no-eval safety with or without CSP. This would be compat with the CSP no-eval option, but would be redundant belt and suspenders, in case the CSP was, for example, mistakenly changed under maintenance.

What should happen for eval within a compartment? The compartment itself internally based on the real eval and provide a CSP-safe eval?

Jack-Works · 2021-12-19T02:46:17Z

And is it possible to have different eval behavior in the start compartment and the child compartment? For example, start compartment has noEval and the child one has safeEval? Or start compartment with unsafeEval but child has noEval

erights · 2021-12-20T08:04:28Z

'noEval' -- all evaluators are disabled, achieving no-eval safety with or without CSP. This would be compat with the CSP no-eval option, but would be redundant belt and suspenders, in case the CSP was, for example, mistakenly changed under maintenance.

What should happen for eval within a compartment? The compartment itself internally based on the real eval and provide a CSP-safe eval?

Are you asking about the 'noEval' option specifically? In that case, all evaluators should be inert (only throwing errors), including those within compartments.

erights · 2021-12-20T08:07:58Z

And is it possible to have different eval behavior in the start compartment and the child compartment? For example, start compartment has noEval and the child one has safeEval? Or start compartment with unsafeEval but child has noEval

It could be possible by other means. But I don't think appropriate for such lockdown options, since these are not per-compartment. I don't think we need to go that far in this PR.

kriskowal · 2021-12-20T17:52:14Z

I can only think of motivating uses to rationalize two modes: one where eval cannot be used in any form and one where we use eval. Which of these three modes do we not need and can we frame the remaining two in a way that makes the choice clearest to the end user?

Or: what’s the third motivating use case?

Jack-Works · 2022-01-03T04:46:50Z

Updated cc @erights @kriskowal

Jack-Works · 2022-01-16T13:55:02Z

ping @erights @kriskowal can u check out this PR?

kriskowal · 2022-01-16T17:14:31Z

I’ve given this a scan and it looks close. I will ask @erights and @mhofman to sign off on the architecture and I will send a review with suggestions to clarify the feature documentation, which will help me think through the rationale for each of the taming options. This will need a short entry in NEWS

kriskowal

This is good and close to merge-worthy, pending architecture approval from some combination of @erights and @mhofman. The code does not yet have the intended effect for the "unsafe" option.

I would like to propose the names "safe", "unsafe", and "none" for which there is precedent on other taming options.

I would like to see a test case that verifies that compartment evaluators work with the "unsafe" option.

packages/ses/src/commons.js

packages/ses/src/global-object.js

packages/ses/docs/lockdown.md

erights

Environment variable misnamed?

packages/ses/src/lockdown-shim.js

erights

LGTM modulo lint errors.

packages/ses/docs/lockdown.md

kriskowal

Now that I’ve slept on it, I agree with Mark that the taming option names should be noEval, safeEval, and unsafeEval. Mostly, none is not a good name. My apologies for making more work for you.

kriskowal

Thank you. This is looking great. I would like compartment evaluator tests for each of the modes.

It would also be good to test compartments constructed within another compartment, specifically to highlight the TODO you added regarding the propagation of evalTaming to child compartments. I expect safeEval will work fine, but noEval might not be working correctly.

Otherwise, nits.

Thank you for the drive-by cleanups.

packages/ses/docs/lockdown.md

packages/ses/src/commons.js

packages/ses/src/compartment-shim.js

packages/ses/src/lockdown-shim.js

packages/ses/test/test-evalTaming-noEval.js

packages/ses/test/test-evalTaming-default.js

packages/ses/test/test-evalTaming-unsafe.js

Jack-Works · 2022-02-16T15:31:18Z

hi @kriskowal can I get a review? this is blocking our product to adopt lockdown

kriskowal

It would be nice to add an error code doc. If you don’t, I’ll do that in follow-up. I would like to get @erights’s final stamp.

packages/ses/src/commons.js

@erights

I am satisfied and defer to @erights for final sign-off.

Jack-Works · 2022-02-16T16:40:14Z

hi is this change you're expecting? I just pushed a new commit @kriskowal

kriskowal · 2022-02-16T17:18:33Z

Yes, that will do, thank you!

…

On Wed, Feb 16, 2022 at 8:40 AM Jack Works ***@***.***> wrote: hi is this change you're expecting? I just pushed a new commit @kriskowal <https://github.com/kriskowal> — Reply to this email directly, view it on GitHub <#961 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOXBTOBW2TNPT6DL2IGNDU3PHPRANCNFSM5KKI7T6A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

mhofman

There is regression about marking the eval and Function globals as native functions.

I'm a little confused about the goal of noEval and the interaction with the Compartment shim. compartment.evaluate() is also an evaluator. Shouldn't it throw as well in the case of noEval taming? That would limit the usefulness of Compartment since the shim doesn't natively support ES modules (to replicate CSP, I would imagine only some modules would be allowed to be imported based on some policy)

packages/ses/src/global-object.js

packages/ses/docs/lockdown.md

erights

LGTM. All my comments are only on documentation.

Thanks for this enhancement!

packages/ses/docs/lockdown.md

erights

LGTM

erights · 2022-02-19T06:33:19Z

packages/ses/docs/lockdown.md

+
+```js
+const c = new Compartment()
+c.globalThis.eval = c.globalThis.Function = function() {


All the *Function constructors reachable by somePrototype.constructor are inert. They only throw. They don't do tamed evaluation.

The only *Function constructor that has ever been reachable any other way is the Function constructor itself, by name from the global object. Thus, by default, that one is a tamed evaluator.

Besides eval and Function, we have one other per-compartment global evaluator: Compartment.prototype.evaluate. If we want to turn off all evaluators in a compartment, we need to turn that one off as well.

Co-authored-by: Kris Kowal <kris@cixar.com> Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

Co-authored-by: Kris Kowal <kris@cixar.com>

Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

We have agreed out-of-band that to keep the compartment evaluator behavior as written.

Jack-Works · 2022-02-22T15:32:35Z

Cool!

Closes: #XXXX Refs: #961 #2690 #2723 ## Description #961 deviated from our general convention that lockdown option values be kebob-case, instead adding `evalTaming:` option values `safeEval`, `unsafeEval`, `noEval`. (I approved #961 at the time, apparently without noticing this discrepancy.) This PR fixes those to be `safe-eval`, `unsafe-eval`, and `no-eval`. But to avoid breaking any old usage, this PR ensure the only names continue to work for now, but always commented in the code as "deprecated". This PR does only that. Other changes to the relevant lockdown option or relevant lockdown options machinery are left to #2723 or #2690 respectively. I request that this PR go first, with those others adjusting to this one. ### Security Considerations none ### Scaling Considerations non ### Documentation Considerations This PR simply changes the documentation to use the new names without ever mentioning the deprecated old names. That seems like an appropriate simplification for the docs. ### Testing Considerations With a bit of duplication and renaming, we now test the new names and the old deprecated names. ### Compatibility Considerations To avoid breaking any old usage, this PR ensure the only names continue to work for now, but always commented in the code as "deprecated". It would be very nice to eventually be able to retire the deprecated names, but I find it hard to imagine how we'd test that it is safe to do so. ### Upgrade Considerations Nothing BREAKING, since the old deprecated names continue to work. - [x] Update `NEWS.md` for user-facing changes.

erights requested changes Dec 18, 2021

View reviewed changes

packages/ses/src/lockdown-shim.js Outdated Show resolved Hide resolved

packages/ses/src/global-object.js Show resolved Hide resolved

erights self-requested a review December 20, 2021 08:01

kriskowal requested changes Jan 23, 2022

View reviewed changes

packages/ses/src/commons.js Outdated Show resolved Hide resolved

packages/ses/src/global-object.js Outdated Show resolved Hide resolved

packages/ses/src/global-object.js Outdated Show resolved Hide resolved

packages/ses/docs/lockdown.md Outdated Show resolved Hide resolved

erights requested changes Jan 23, 2022

View reviewed changes

packages/ses/src/lockdown-shim.js Outdated Show resolved Hide resolved

erights approved these changes Jan 23, 2022

View reviewed changes

kriskowal reviewed Jan 23, 2022

View reviewed changes

Jack-Works requested a review from kriskowal January 28, 2022 16:48

kriskowal previously requested changes Jan 28, 2022

View reviewed changes

kriskowal self-requested a review February 16, 2022 16:03

kriskowal reviewed Feb 16, 2022

View reviewed changes

packages/ses/src/commons.js Show resolved Hide resolved

kriskowal approved these changes Feb 16, 2022

View reviewed changes

Jack-Works requested a review from erights February 17, 2022 03:27

kriskowal requested a review from mhofman February 17, 2022 04:21

mhofman previously requested changes Feb 17, 2022

View reviewed changes

packages/ses/src/global-object.js Outdated Show resolved Hide resolved

packages/ses/docs/lockdown.md Outdated Show resolved Hide resolved

erights approved these changes Feb 17, 2022

View reviewed changes

erights reviewed Feb 18, 2022

View reviewed changes

packages/ses/docs/lockdown.md Outdated Show resolved Hide resolved

Jack-Works commented Feb 18, 2022

View reviewed changes

packages/ses/docs/lockdown.md Outdated Show resolved Hide resolved

Jack-Works commented Feb 18, 2022

View reviewed changes

packages/ses/docs/lockdown.md Outdated Show resolved Hide resolved

Jack-Works requested review from mhofman, erights and kriskowal February 19, 2022 04:01

erights approved these changes Feb 19, 2022

View reviewed changes

Jack-Works and others added 19 commits February 22, 2022 07:06

feat: add eval taming

471580f

fix: no compartment removal

e3eee4b

chore: fix type error

71c90d3

feat: add noEval option

945623b

docs: fix doc

b9e74e5

Apply suggestions from code review

c56c02d

Co-authored-by: Kris Kowal <kris@cixar.com> Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

Apply suggestions from code review

caaf7ca

Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

fix: linter and obvious errors

3b5e872

chore: follow review

3e2def5

docs: update document

e682ad3

fix: test failed

a676525

Apply suggestions from code review

d078ecf

Co-authored-by: Kris Kowal <kris@cixar.com>

chore: add test cases

e83e4b4

docs: update docs

d2b862a

fix: evaluators not marked as native

d321ebd

Apply suggestions from code review

264f787

Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

Update packages/ses/docs/lockdown.md

6eb5cd6

Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>

docs: update docs

ed2c7fb

docs: update docs

f1e32b5

kriskowal enabled auto-merge (squash) February 22, 2022 15:10

kriskowal merged commit 735ff94 into endojs:master Feb 22, 2022

Jack-Works deleted the eval-taming branch February 22, 2022 15:32

erights mentioned this pull request Mar 12, 2025

fix(ses): lockdown options should be kebob-case #2739

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add evalTaming option #961

feat: add evalTaming option #961

Jack-Works commented Dec 18, 2021 •

edited

Loading

erights left a comment

Jack-Works commented Dec 19, 2021

Jack-Works commented Dec 19, 2021

erights commented Dec 20, 2021

erights commented Dec 20, 2021

kriskowal commented Dec 20, 2021 •

edited

Loading

Jack-Works commented Jan 3, 2022

Jack-Works commented Jan 16, 2022

kriskowal commented Jan 16, 2022

kriskowal left a comment

erights left a comment

erights left a comment

kriskowal left a comment

kriskowal left a comment

Jack-Works commented Feb 16, 2022

kriskowal left a comment

Jack-Works commented Feb 16, 2022

kriskowal commented Feb 16, 2022 via email

mhofman left a comment

erights left a comment

erights left a comment

erights Feb 19, 2022

Jack-Works commented Feb 22, 2022

feat: add evalTaming option #961

feat: add evalTaming option #961

Conversation

Jack-Works commented Dec 18, 2021 • edited Loading

erights left a comment

Choose a reason for hiding this comment

Jack-Works commented Dec 19, 2021

Jack-Works commented Dec 19, 2021

erights commented Dec 20, 2021

erights commented Dec 20, 2021

kriskowal commented Dec 20, 2021 • edited Loading

Jack-Works commented Jan 3, 2022

Jack-Works commented Jan 16, 2022

kriskowal commented Jan 16, 2022

kriskowal left a comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

Jack-Works commented Feb 16, 2022

kriskowal left a comment

Choose a reason for hiding this comment

Jack-Works commented Feb 16, 2022

kriskowal commented Feb 16, 2022 via email

mhofman left a comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

erights Feb 19, 2022

Choose a reason for hiding this comment

Jack-Works commented Feb 22, 2022

Jack-Works commented Dec 18, 2021 •

edited

Loading

kriskowal commented Dec 20, 2021 •

edited

Loading