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

lib: Add option to disable __proto__ accessors #32279

Merged
merged 1 commit into from Mar 18, 2020
Merged

Conversation

@devsnek
Copy link
Member

devsnek commented Mar 15, 2020

Adds --disable-proto CLI option which can be set to delete or
throw.

Fixes #31951

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
@devsnek devsnek added the security label Mar 15, 2020
@devsnek devsnek requested review from mcollina, bmeck, jasnell and guybedford Mar 15, 2020
@devsnek devsnek force-pushed the devsnek:disable-proto branch 3 times, most recently Mar 15, 2020
lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Mar 15, 2020

Maybe clarify somewhere that this only affects the main Context?

Copy link
Member

mcollina left a comment

lgtm

Does this also impact worker threads? I think it should and we could also add an option there.

I'm also perfectly fine in landing this as this and opening an issue for worker threads etc.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Mar 15, 2020

Does this also impact worker threads? I think it should and we could also add an option there.

Worker threads inherit their execArgv from the parent thread, unless specified otherwise. I would not add a separate option for this.

@legendecas

This comment has been minimized.

Copy link
Member

legendecas commented Mar 15, 2020

Does it feasible to add an option to ignore the __proto__ field rather than throwing? If the engine does ignore the __proto__s there is no reason for users to handle these errors.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Mar 15, 2020

@legendecas How would that differ from --disable-proto=delete? Are you suggesting that a.__proto__ = {}; does nothing, including not setting __proto__ as a regular property? That would seem more surprising to me…

@legendecas

This comment has been minimized.

Copy link
Member

legendecas commented Mar 15, 2020

@addaleax I tried the PR locally, it doesn't seem to be effective on the case of delete since only Object.prototype.__proto__ is deleted in the PR:

$ node --disable-proto=delete -p '({ __proto__: Array.prototype }).push === Array.prototype.push'
true

What I'm understanding/expecting is:

$ node --disable-proto=delete -p '({ __proto__: Array.prototype }).push'
undefined
doc/api/errors.md Outdated Show resolved Hide resolved
@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Mar 15, 2020

@legendecas object literals do not use the getter/setter and are a special case of syntax. They are not problematic for assignment like the accessors and not part of the CVEs in question

Copy link
Contributor

cjihrig left a comment

This should update doc/node.1 too.

@devsnek devsnek force-pushed the devsnek:disable-proto branch Mar 15, 2020
@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Mar 15, 2020

@cjihrig can you verify i did the node.1 change correctly? i just kind of guessed based on other entries...

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Mar 15, 2020

@devsnek you can run man doc/node.1 to look at the finished product.

EDIT: It looks good at a quick glance, but I don't really know the format and need to view it with man.

@devsnek devsnek force-pushed the devsnek:disable-proto branch 2 times, most recently Mar 15, 2020
@Slayer95

This comment has been minimized.

Copy link

Slayer95 commented Mar 16, 2020

The given solution might not provide protection against the following test case:

"use strict";

const assert = require('assert');
const vm = require('vm');

(function patchProto() {
	const protoThrow = () => {throw new Error(`Illegal __proto__ access`)}
	Object.defineProperty(Object.prototype, '__proto__', {get: protoThrow, set: protoThrow, enumerable: false, configurable: false});
})();

function getCrossRealmObject() {
	const ctx = vm.createContext({});
	vm.runInContext('value = {"foreign":1}', ctx);
	return ctx.value;
}

function testProto() {
	const payload = '{"__proto__": {"evil": true}}';

	const x = getCrossRealmObject();
	try {
		Object.assign(x, JSON.parse(payload));
	} catch (err) {}
	assert(!x.evil);
}

testProto();

If it does, please add such a test!

@bmeck bmeck changed the title lib: Add option to disable __proto__ lib: Add option to disable __proto__ accessors Mar 16, 2020
doc/api/cli.md Show resolved Hide resolved
@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Mar 16, 2020

@Slayer95 See @addaleax's #32279 (comment):

this only affects the main Context

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented Mar 16, 2020

lib/internal/bootstrap/pre_execution.js Outdated Show resolved Hide resolved
@devsnek devsnek requested review from devnexen, cjihrig, jasnell and mcollina Mar 16, 2020
@devsnek devsnek force-pushed the devsnek:disable-proto branch Mar 16, 2020
@devsnek devsnek requested review from bmeck and tniessen Mar 16, 2020
@nodejs-github-bot

This comment has been minimized.

@bmeck
bmeck approved these changes Mar 17, 2020
Adds `--disable-proto` CLI option which can be set to `delete` or
`throw`.

Fixes #31951

PR-URL: #32279
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
@devsnek devsnek force-pushed the devsnek:disable-proto branch to 7a742ec Mar 18, 2020
@devsnek devsnek merged commit 7a742ec into nodejs:master Mar 18, 2020
19 of 22 checks passed
19 of 22 checks passed
ubuntu-build ubuntu-build
Details
ubuntu-build ubuntu-build
Details
build-docs
Details
build-docs
Details
build (3.8)
Details
build (3.8)
Details
build-linux
Details
build-linux
Details
build-windows
Details
build-windows
Details
build-macOS
Details
build-macOS
Details
lint-addon-docs
Details
lint-addon-docs
Details
lint-cpp
Details
lint-cpp
Details
lint-md
Details
lint-md
Details
lint-js
Details
lint-js
Details
Travis CI - Pull Request Build Errored
Details
Travis CI - Branch Build Passed
Details
@devsnek devsnek deleted the devsnek:disable-proto branch Mar 18, 2020
MylesBorins added a commit that referenced this pull request Mar 19, 2020
Adds `--disable-proto` CLI option which can be set to `delete` or
`throw`.

Fixes #31951

PR-URL: #32279
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
MylesBorins added a commit that referenced this pull request Mar 24, 2020
Adds `--disable-proto` CLI option which can be set to `delete` or
`throw`.

Fixes #31951

PR-URL: #32279
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
MylesBorins added a commit that referenced this pull request Mar 25, 2020
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 25, 2020
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * update npm to 6.14.3 (Myles Borins)
    #32368
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
MylesBorins added a commit that referenced this pull request Mar 26, 2020
macOS package notarization and a change in builder configuration:

The macOS binaries for this release, and future 13.x releases, are now
being compiled on macOS 10.15 (Catalina) with Xcode 11 to support
package notarization, a requirement for installing on .pkg files on
macOS 10.15 and later. Previous builds of Node.js 13.x were compiled on
macOS 10.11 (El Capitan) with Xcode 10. As binaries are still being
compiled to support a minimum of macOS 10.10 (Yosemite) we do not
anticipate this having a negative impact on Node.js 13.x users with
older versions of macOS.

Notable changes:

* build:
  * macOS package notarization (Rod Vagg)
    #31459
* deps:
  * upgrade npm to 6.14.4 (Ruy Adorno)
    #32495
  * update to uvwasi 0.0.6 (Colin Ihrig)
    #32309
  * upgrade to libuv 1.35.0 (Colin Ihrig)
    #32204
* lib:
  * add --disable-proto option to cli (Gus Caplan)
    #32279
* node_report:
  * move diagnostic reports to stable (Colin Ihrig)
    #32242
* worker:
  * allow URL in Worker constructor (Antoine du HAMEL)
    #31664
* util:
  * use a global symbol for `util.promisify.custom` (ExE Boss)
    #31672

PR-URL: #32376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.