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

Conversation

devsnek
Copy link
Member

@devsnek 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 Issues and PRs related to security. label Mar 15, 2020
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 15, 2020
@devsnek devsnek force-pushed the disable-proto branch 3 times, most recently from 39a58eb to 8a31826 Compare March 15, 2020 02:27
@addaleax
Copy link
Member

Maybe clarify somewhere that this only affects the main Context?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

@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
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 cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should update doc/node.1 too.

@devsnek
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
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 disable-proto branch 2 times, most recently from 7489319 to 5559f5f Compare March 15, 2020 23:10
@Slayer95
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
Copy link
Member

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

this only affects the main Context

@nodejs-github-bot
Copy link
Collaborator

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

Fixes nodejs#31951

PR-URL: nodejs#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 merged commit 7a742ec into nodejs:master Mar 18, 2020
@devsnek devsnek deleted the disable-proto branch March 18, 2020 17:24
MylesBorins pushed 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
@devsnek devsnek added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 23, 2020
MylesBorins pushed 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 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
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 22, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Adds `--disable-proto` CLI option which can be set to `delete` or
`throw`.

Fixes nodejs#31951

PR-URL: nodejs#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>
targos pushed a commit that referenced this pull request Apr 28, 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. security Issues and PRs related to security. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable __proto__