deps: cherry-pick 0bcb1d6f from upstream V8

[MylesBorins]

This has landed in 6.5, but we shouldn't have to wait!

Original commit message:

Introduce --disallow-code-generation-from-strings

Exposing the existing Context::AllowCodeGenerationFromStrings(false) API
to the command line.

Bug: v8:7134
Change-Id: I062ccff0b03c5bcf6878c41c455c0ded37a1d743
Reviewed-on: https://chromium-review.googlesource.com/809631
Reviewed-by: Michael Starzinger <[email protected]>
Commit-Queue: Jakob Kummerow <[email protected]>
Cr-Commit-Position: refs/heads/master@{#49911}

Refs: v8/v8@0bcb1d6

[MylesBorins]

CI: https://ci.nodejs.org/job/node-test-pull-request/12591/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1166/

[jasnell]

It should be noted that this flag has no impact on vm.runInThisContext() (and the other variants). This is notable in that vm.runInThisContext() can be easily used as a workaround for a lack of eval

const eval = vm.runInThisContext;
n = 1;
eval('n++');
// n == 2

[MylesBorins]

@jasnell do you think it might make sense to create a node specific flag that enables not only this flag but stops the ability to execute code in other fashions as well?

[bengl]

@MylesBorins If that were done, there would need to be some other way of loading code for require, or some kind of whitelist, or a Symbol passed in or something to give the require logic the ability to vm.runInThisContext regardless of the flag.

https://github.com/nodejs/node/blob/master/lib/module.js#L628

[jasnell]

Yep, given our internal reliance on vm.runInThisContext() etc, we need to be careful here. If nothing else, we land this with some documentation around it that it's not 100% effective.

[MylesBorins]

@jasnell "this" as in this specific PR or a future PR?

[jasnell]

Perhaps a second commit in this PR? Not sure exactly where that should go. Maybe in the cli.md or in vm.md.

[hashseed]

I faintly remember that this would also disable wasm. Do you want to make sure? Does that match your expectation?

[mcollina]

Would this block new Function() as well?
There are valid use cases for this.

I prefer not to spread bad information that running with this flag is the way to go to improve security.

[bnoordhuis]

given our internal reliance on vm.runInThisContext()

That shouldn't be affected, v8::ScriptCompiler is a different mechanism. But that's why this flag shouldn't be promoted as a security mechanism - it's too easy to subvert with require('vm').runInThisContext(...).

Aside: node --prof-process is probably broken with this flag on, it uses eval().

[hashseed]

Would this block new Function() as well?

Yes. Otherwise you could just use that to bypass eval.

[jasnell]

But that's why this flag shouldn't be promoted as a security mechanism

Absolutely agree. It's a good flag to have, but promoting it as a security mechanism is just a facade. Like locking the front door while leaving all the windows and the back door open with signs that say "Free Cookies Inside"

[BridgeAR]

Where do we stand here? There is also #18453 that probably already included this as well?

[MylesBorins]

I'm not sure what we should do regarding documentation of this feature. Would it make sense to wait for 6.5? I was thinking it might make sense to cherry-pick to 8.x in a future semver-minor. This is the beginning of a larger story around locking down eval.

should we land this as is?
should we block on docs?
should we wait for 6.5?

[MylesBorins]

CI: https://ci.nodejs.org/job/node-test-pull-request/12946/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1198/

[MylesBorins]

CI is green and I have 3 LGTMs. Are people ok with this landing without any documentation?

[bnoordhuis]

Can you update lib/internal/v8_prof_processor.js to use vm.runInThisContext() instead of eval()?

[MylesBorins]

@bnoordhuis 99d693d has landed on master

@jasnell thoughts re: skipping docs?

[MylesBorins]

ping @bnoordhuis and @jasnell

[jasnell]

I signed off. I'd still prefer the doc addition.

[MylesBorins]

@jasnell more than happy to add the doc addition but struggling to figure out the right place to do it, do you have any suggestions?

[MylesBorins]

since i'm not seeing anything actionable on where to add the docs I'm going to go ahead and land this.
Landed in: 8a54f4f