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

Frozen intrinsics experimental flag #25685

Closed
wants to merge 4 commits into from

Conversation

@guybedford
Copy link
Contributor

guybedford commented Jan 24, 2019

This PR implements a node --frozen-intrinsics flag which applies the deep freeze method from SES (https://github.com/Agoric/SES/blob/15ecf1520c8314f7ec29165548eccff1118e294d/src/bundle/deepFreeze.js) to all known intrinsics.

There is a more detailed write up by those involved in these security initiatives at https://docs.google.com/document/d/1h__FmXsEWRuNrzAV_l3Iw9i_z8fCXSokGfBiW8-nDNg/edit?ts=5c1adaed#heading=h.q1x6on1y6aju, I just put this together as it seems an easy low-hanging fruit to improve security in Node.js.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@guybedford guybedford force-pushed the guybedford:freeze-intrinsics branch 5 times, most recently from 6f8f222 to 496cf4d Jan 24, 2019

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jan 24, 2019

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Jan 24, 2019

@guybedford

I'd be more interested in seeing a solution where userland can still polyfill and such. There have been other proposals where node uses the original methods without needing to freeze everything.

also fwiw:

IteratorPrototype = Object.getPrototypeOf(Object.getPrototypeOf([][Symbol.iterator]()))

TypedArray = Object.getPrototypeOf(Uint8Array)

ThrowTypeError = Object.getOwnPropertyDescriptor(Function.prototype, 'caller').get

@addaleax
Copy link
Member

addaleax left a comment

  • What’s the idea behind an --experimental-... flag? Does this mean the goal is to eventually enable this by default? That seems … maybe not feasible?
  • Should this be applied in a per-Context way, i.e. also affect code inside vm.* environments? I think that would require making lib/internal/per_context.js more powerful, but it might be worth it? (/cc @joyeecheung)
Show resolved Hide resolved lib/internal/freeze_intrinsics.js Outdated
Show resolved Hide resolved test/parallel/test-freeze-intrinsics.js
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Jan 25, 2019

What’s the idea behind an --experimental-... flag? Does this mean the goal is to eventually enable this by default? That seems … maybe not feasible?

The goal would be to make it a non-experimental flag, but there are still a few things to ensure before such a stage:

  1. We probably want this spec bug fixed tc39/ecma262#1320 as it stops classes extending any of these builtins from overridding properties otherwise.
  2. It could be worth considering including in this ensuring that global.Array is itself unwritable / unconfigurable, but that seems like a next step after this PR to me
  3. The per_context handling also definitely sounds preferable. I just tried implementing this, but hit (1) causing a bug on the buffer code - class FastBuffer extends Uint8Array {}; FastBuffer.prototype.toString = .... Also per_context is too early to catch the console and timers freezing as well. We should definitely come back around on this though.

So the --experimental aspect allows as to move forward despite 1 - 3 above. Although I'm certainly open to discussion about this approach.

@devsnek thanks for filling in the remaining anonymous intrinsics there, that's a huge help.

@vdeturckheim

This comment has been minimized.

Copy link
Member

vdeturckheim commented Jan 25, 2019

Do we have numbers regarding a potential perf impact here?

Also, if I'm not mistaken this would prevent adding methods to global classes but since Global is not frozen, one could still replace the value of say String.

Also, could it be possible to make this list expendable or to expose the deep freeze method for users who might want to use it?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 25, 2019

The goal would be to make it a non-experimental flag, but there are still a few things to ensure before such a stage:

In that case I’d prefer to give the flag its desired non-experimental name and document it as experimental.

@guybedford guybedford force-pushed the guybedford:freeze-intrinsics branch from 978762c to dc2b969 Jan 26, 2019

Show resolved Hide resolved doc/api/cli.md
Show resolved Hide resolved doc/api/cli.md
@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Jan 26, 2019

In that case I’d prefer to give the flag its desired non-experimental name and document it as experimental.

@addaleax sounds good, I've renamed to --frozen-intrinsics and made it clear it is experimental. If I should note this in any other way too let me know.

Do we have numbers regarding a potential perf impact here?

In most benchmarks these days for v8, frozen objects behave identically to non-frozen ones. The time to freeze on my machine is about 20ms which is no insignificant (against a 80ms startup). But we should be able to integrate the frozen objects into the snapshot in future to avoid this.

Also, if I'm not mistaken this would prevent adding methods to global classes but since Global is not frozen, one could still replace the value of say String.

Yes exactly, I've explained this clearer in the notes. We could use a getter / setter approach to fix this, or we could look at some context approaches too.

Also, could it be possible to make this list expendable or to expose the deep freeze method for users who might want to use it?

Exposing the freeze API is something for userland, this internal API may even be optimized out in future so this is a non-goal.

@guybedford guybedford force-pushed the guybedford:freeze-intrinsics branch from dc2b969 to 9c5d7c7 Jan 26, 2019

@jdalton

This comment has been minimized.

Copy link
Member

jdalton commented Jan 28, 2019

I noticed --frozen-intrinsics breaks console creation which could be related to this note:

> new console.Console({ stdout: process.stdout, stderr: process.stderr });
Thrown:
TypeError: Cannot assign to read only property 'log' of object '#<Console>'
    at new Console (internal/console/constructor.js:112:13)
@@ -340,6 +340,10 @@ function startup() {
NativeModule.require('internal/process/report').setup();
}

if (getOptionValue('--frozen-intrinsics')) {

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 29, 2019

Member

I'd say prepareUserCodeExecution is a better place for this, as this does not make a lot of sense in use cases like node --help

This comment has been minimized.

Copy link
@guybedford

guybedford Feb 17, 2019

Author Contributor

Thanks. This is now in pre_execution.js. Let me know if that seems right.

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Jan 29, 2019

Should this be applied in a per-Context way, i.e. also affect code inside vm.* environments? I think that would require making lib/internal/per_context.js more powerful, but it might be worth it?

@addaleax How would this option get passed to NewContext? Maybe a boolean? Currently at the point per_context.js is executed the JS land cannot access the CLI options yet, also note that we need to override things like global.console by ourselves during bootstrap that is done after per_context.js

In most benchmarks these days for v8, frozen objects behave identically to non-frozen ones. The time to freeze on my machine is about 20ms which is no insignificant (against a 80ms startup). But we should be able to integrate the frozen objects into the snapshot in future to avoid this.

I recall there are still some perf issues regarding frozen objects in V8, cc @caitp who's been working on that

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Jan 29, 2019

FWIW, this looks more like a v8 flag than a Node flag to me, or this looks less overreaching if V8 provides an API for us to do this.

@caitp

This comment has been minimized.

Copy link
Contributor

caitp commented Jan 29, 2019

FWIW, this looks more like a v8 flag than a Node flag to me, or this looks less overreaching if V8 provides an API for us to do this.

It's not a v8 flag, and v8 does not currently provide an API which does this.

I recall there are still some perf issues regarding frozen objects in V8

As discussed offline, there are some well known issues regarding indexed property access to frozen objects --- however, these would generally not affect the things on the white list, currently. OTOH, the main cost would be some slight increased startup cost, since these frozen objects aren't frozen in any snapshot.

I'll be working on speeding up other cases, so please ping me on perf bugs regarding frozen object access.

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Jan 29, 2019

It's not a v8 flag, and v8 does not currently provide an API which does this.

Yes, I was talking hypothetically

But we should be able to integrate the frozen objects into the snapshot in future to avoid this.

@guybedford The freezing depends on a CLI flag, so we can't include it in a snapshot for the default startup, the best workaround I can think of is to build two snapshots and include them both by default, but I am not sure how much the size overhead would be if we do this for different combinations of CLI flags.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Jan 29, 2019

i agree that this would make sense as a feature for v8, rather than node. even if they don't come up with a magical optimization for the snapshot, it would be a lot faster than this, since they can just directly flip some flags. (and we wouldn't have to worry about stuff like new globals exposed by v8 flags, which this misses (for example, WeakRef))

@guybedford guybedford force-pushed the guybedford:freeze-intrinsics branch from 99e3a4b to 075529a Mar 3, 2019

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 3, 2019

I'd like to land this tomorrow, unless anyone has any feedback / objections further on this experimental flag?

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

@guybedford

This comment has been minimized.

Copy link
Contributor Author

guybedford commented Mar 4, 2019

Landed in b2abda9. Thanks all for feedback, and I look forward to seeing these security properties evolve in future.

@guybedford guybedford closed this Mar 4, 2019

guybedford added a commit that referenced this pull request Mar 4, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: #25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 13, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: nodejs#25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 13, 2019

This does not land cleanly on v11. Please open a manual backport for this as other PRs rely on it.

ZYSzys added a commit to zys-contribs/node that referenced this pull request Mar 14, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: nodejs#25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

ZYSzys added a commit to zys-contribs/node that referenced this pull request Mar 14, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: nodejs#25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

ZYSzys added a commit to zys-contribs/node that referenced this pull request Mar 14, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: nodejs#25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: #25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 14, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: nodejs#25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    nodejs#25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    nodejs#26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    nodejs#26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) nodejs#26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) nodejs#26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    nodejs#26527

BridgeAR added a commit that referenced this pull request Mar 14, 2019

bootstrap: experimental --frozen-intrinsics flag
PR-URL: #25685
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

2019-03-14, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527

BridgeAR added a commit that referenced this pull request Mar 15, 2019

2019-03-15, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    #25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    #26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    #26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) #26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) #26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    #26527

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 15, 2019

2019-03-15, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    nodejs#25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    nodejs#26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    nodejs#26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) nodejs#26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) nodejs#26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    nodejs#26527

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019

2019-03-15, Version 11.12.0 (Current)
Notable Changes

* bootstrap:
  * Add experimental `--frozen-intrinsics` flag (Guy Bedford)
    nodejs#25685
* build:
  * Enable v8's siphash for hash seed creation (Rod Vagg)
    nodejs#26367
* deps:
  * Upgrade openssl to 1.1.1b (Sam Roberts)
    nodejs#26327
* process:
  * Make `process[Symbol.toStringTag]` writable again
    (Ruben Bridgewater) nodejs#26488
* repl:
  * Add `util.inspect.replDefaults` to customize the writer
    (Ruben Bridgewater) nodejs#26375
* report:
  * Rename `triggerReport()` to `writeReport()` (Colin Ihrig)
    nodejs#26527

@targos targos added this to Backported in v11.x Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.