deps,build: disable V8 untrusted code mitigations by targos · Pull Request #19222 · nodejs/node

targos · 2018-03-08T09:36:37Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

@nodejs/v8

addaleax · 2018-03-08T09:40:18Z

Can we add documentation for users on how to undo the effects of this at runtime?

targos · 2018-03-08T09:41:42Z

Sure, where should I write it? In BUILDING.md?

vsemozhetbyt · 2018-03-08T09:43:31Z

Refs: nodejs/node-v8#40

targos · 2018-03-08T09:45:39Z

Sorry I didn't read the "runtime" part. I guess we can add it to node --help output and the man page.

addaleax · 2018-03-08T09:46:36Z

Also, does this affect WebAssembly? I might be wrong but I was under the impression that that actually does provide a reliable, full sandbox within Node now.

Sure, where should I write it? In BUILDING.md?

I guess that depends – I read the linked issue, but I’m not sure whether --untrusted-code-mitigations will work at runtime or not if we bake this into the executable?
If not, then that seems like the right place, but I’m not sure whether it’s a good idea to do this at all then. If it does, I think it might be worth explicitly documenting this as a CLI option, like we do for --abort-on-uncaught-exception.

targos · 2018-03-08T09:52:12Z

/cc @hashseed and @bmeurer.
In nodejs/node-v8#40, you said that the flag not only impacts runtime but also some things are compiled differently.
I can't find anything in V8's source related to that. It seems to me that only the runtime flag is affected:

git grep -i untrusted -- deps/v8

deps/v8/BUILD.gn:  v8_untrusted_code_mitigations = true
deps/v8/BUILD.gn:  if (!v8_untrusted_code_mitigations) {
deps/v8/BUILD.gn:    defines += [ "DISABLE_UNTRUSTED_CODE_MITIGATIONS" ]
deps/v8/gypfiles/features.gypi:    'v8_untrusted_code_mitigations%': 'true',
deps/v8/gypfiles/features.gypi:      ['v8_untrusted_code_mitigations=="false"', {
deps/v8/gypfiles/features.gypi:        'defines': ['DISABLE_UNTRUSTED_CODE_MITIGATIONS',],
deps/v8/src/compilation-info.cc:      flags_(FLAG_untrusted_code_mitigations ? kUntrustedCodeMitigations : 0),
deps/v8/src/compilation-info.h:  bool has_untrusted_code_mitigations() const {
deps/v8/src/compiler/pipeline.cc:          data->info()->has_untrusted_code_mitigations()
deps/v8/src/compiler/wasm-compiler.cc:      untrusted_code_mitigations_(FLAG_untrusted_code_mitigations),
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.cc:  if (untrusted_code_mitigations_) {
deps/v8/src/compiler/wasm-compiler.h:  const bool untrusted_code_mitigations_ = true;
deps/v8/src/flag-definitions.h:#ifdef DISABLE_UNTRUSTED_CODE_MITIGATIONS
deps/v8/src/flag-definitions.h:#define V8_DEFAULT_UNTRUSTED_CODE_MITIGATIONS false
deps/v8/src/flag-definitions.h:#define V8_DEFAULT_UNTRUSTED_CODE_MITIGATIONS true
deps/v8/src/flag-definitions.h:DEFINE_BOOL(untrusted_code_mitigations, V8_DEFAULT_UNTRUSTED_CODE_MITIGATIONS,
deps/v8/src/flag-definitions.h:#undef V8_DEFAULT_UNTRUSTED_CODE_MITIGATIONS

targos · 2018-03-08T09:52:59Z

@addaleax see my previous message. The wasm compiler is affected.

indutny

LGTM, if CI is good.

hashseed · 2018-03-08T11:33:20Z

Let me give some information so that you can make an educated decision on this.

Also, does this affect WebAssembly? I might be wrong but I was under the impression that that actually does provide a reliable, full sandbox within Node now.

Spectre affects WebAssembly just as much as JavaScript. Both are "sandboxed". But that doesn't protect you from leaking information if CPUs do that at a machine instruction level.

In nodejs/node-v8#40, you said that the flag not only impacts runtime but also some things are compiled differently.
I can't find anything in V8's source related to that. It seems to me that only the runtime flag is affected

V8 runs mksnapshot at build time to produce the startup snapshot, which includes code objects to implement builtins and bytecode handlers. The latter are affected by the flag.

We are essentially looking at four combinations of enabling / disabling the flag at build time / runtime:

Disabled at build time and disabled at runtime
This is fine, no mitigations. Performance is good. However, we do not test this configuration at V8 at this point. I don't expect this to be a major risk though.

Disabled at build time and enabled at runtime
This makes no sense, since bytecode handlers do not have the mitigations. This is untested by V8.

Enabled at build time and disabled at runtime
We test this configuration in V8, but not in Chrome. Having mitigations enabled in bytecode handlers will slow down performance of the interpreter, but peak performance will unlikely to be affected, as optimized code do not include mitigations.

Enabled at build time and enabled at runtime
We test this configuration in V8 and in Chrome, with full Canary coverage. Performance will regress, as observed. This gives as much protection against Spectre as we can get with V8 6.6.

Personally I would just disable altogether, and discourage people from enabling at runtime via command line flag.

So this change looks good to me.

natorion · 2018-03-08T11:42:10Z

Please also have a look at https://github.com/v8/v8/wiki/Untrusted-code-mitigations

bmeurer · 2018-03-08T11:44:18Z

I also strongly recommend to not offer any of these mitigations at all by default in Node. They don't make sense. If someone needs a Node with these on, they should build their own version, and better know exactly what they are doing and why. I think we might otherwise risk sending people down into false sense of security, because just passing --untrusted-code-mitigations will not protect anyone in Node land from just accessing anything on the underlying system via require('fs') etc.

bnoordhuis · 2018-03-08T14:39:10Z

Is it an idea to add a regression test that checks that --untrusted_code_mitigations in --v8-options defaults to false?

devsnek · 2018-03-08T14:51:20Z

deps/v8/gypfiles/features.gypi

is this overridden by common.gypi?

Yes. It's the 2nd commit in this PR.

Add a GYP flag similar to the one defined in BUILD.gn.

Refs: https://github.com/v8/v8/wiki/Untrusted-code-mitigations

targos · 2018-03-14T10:02:41Z

@bnoordhuis I added a test.

targos · 2018-03-14T10:05:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13672/

bmeurer

LGTM

targos · 2018-03-15T14:00:35Z

Landed in cac4da0 and c6ae8a2

Add a GYP flag similar to the one defined in BUILD.gn. PR-URL: #19222 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Refs: https://github.com/v8/v8/wiki/Untrusted-code-mitigations PR-URL: #19222 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

targos added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 8, 2018

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 8, 2018

targos changed the title ~~V8 no untrusted code mitigations~~ deps,build: disable V8 untrusted code mitigations Mar 8, 2018

indutny approved these changes Mar 8, 2018

View reviewed changes

hashseed approved these changes Mar 8, 2018

View reviewed changes

cjihrig approved these changes Mar 8, 2018

View reviewed changes

devsnek reviewed Mar 8, 2018

View reviewed changes

deps: allow disabling V8 untrusted code mitigations

b415a93

Add a GYP flag similar to the one defined in BUILD.gn.

targos added dont-land-on-v4.x labels Mar 14, 2018

build: disable V8 untrusted code mitigations

df2a907

Refs: https://github.com/v8/v8/wiki/Untrusted-code-mitigations

targos force-pushed the v8-no-untrusted-code-mitigations branch from 507c9dd to df2a907 Compare March 14, 2018 10:01

jasnell approved these changes Mar 14, 2018

View reviewed changes

bmeurer approved these changes Mar 14, 2018

View reviewed changes

devsnek approved these changes Mar 14, 2018

View reviewed changes

gibfahn approved these changes Mar 15, 2018

View reviewed changes

targos closed this Mar 15, 2018

targos deleted the v8-no-untrusted-code-mitigations branch March 15, 2018 14:00

Uh oh!

Conversation

targos commented Mar 8, 2018

Checklist

Uh oh!

addaleax commented Mar 8, 2018

Uh oh!

targos commented Mar 8, 2018

Uh oh!

vsemozhetbyt commented Mar 8, 2018

Uh oh!

targos commented Mar 8, 2018

Uh oh!

addaleax commented Mar 8, 2018

Uh oh!

targos commented Mar 8, 2018

Uh oh!

targos commented Mar 8, 2018

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

hashseed commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natorion commented Mar 8, 2018

Uh oh!

bmeurer commented Mar 8, 2018

Uh oh!

bnoordhuis commented Mar 8, 2018

Uh oh!

devsnek Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

targos Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

targos commented Mar 14, 2018

Uh oh!

targos commented Mar 14, 2018

Uh oh!

bmeurer left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Mar 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

hashseed commented Mar 8, 2018 •

edited

Loading