lib: be robust when process global is clobbered #10135

bnoordhuis · 2016-12-06T03:55:43Z

Program bugs sometimes happen to overwrite the process global object,
leading to unpredictable runtime errors. The bug is often hard to track
down because the crash usually looks unrelated and happens far away from
the bug point.

Make core mostly immune to such corruption by ensuring that all core
modules close over a direct reference to the process object instead
of a dynamic reference.

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

jasnell · 2016-12-06T04:05:41Z

Nice. I like it.
I want to be careful on this tho... would this have to be a semver-major? (please say no)

jasnell · 2016-12-06T04:07:13Z

test/parallel/test-process-clobber.js

+/* eslint-disable no-global-assign */
+/* eslint-disable required-modules */
+'use strict';
+


require('../common');

I added the required-modules eslint directive so I didn't have to include test/common.js, because while core is now robust to tampering, the code in test/common.js need not be.

Fishrock123 · 2016-12-06T04:34:00Z

This probably won't work in ES Modules, also this changes this.length... idk if these is some strange wrapper that might rely on that or not.

bnoordhuis · 2016-12-06T10:56:33Z

This probably won't work in ES Modules

Why would that be an issue for built-in modules?

also this changes this.length... idk if these is some strange wrapper that might rely on that or not

Seems unlikely. Is there a plausible scenario where it would make a difference? It can't be monkey-patching the module loader because core modules use (the inaccessible to the outside world)NativeModule.require, not regular require.

addaleax

LGTM

addaleax · 2016-12-06T12:42:28Z

lib/internal/bootstrap_node.js

@@ -504,7 +504,7 @@
  };

  NativeModule.wrapper = [
-    '(function (exports, require, module, __filename, __dirname) { ',
+    '(function (exports, require, module, __filename, __dirname, process) { ',


I guess you might as well lose the __dirname argument here?

sam-github · 2016-12-06T15:58:49Z

I'm +1 on this.

Can process be made read-only, so it can't be assigned null? Are people deleting the process global on purpose, or accident?

Program bugs sometimes happen to overwrite the `process` global object, leading to unpredictable runtime errors. The bug is often hard to track down because the crash usually looks unrelated and happens far away from the bug point. Make core mostly immune to such corruption by ensuring that all core modules close over a direct reference to the process object instead of a dynamic reference.

bnoordhuis · 2016-12-11T15:08:12Z

@addaleax Removed __dirname.

@sam-github I've added a commit that makes process non-writable (but still configurable), PTAL.

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

EDIT: To answer your question: accidental.

addaleax · 2016-12-11T16:46:35Z

Still LGTM but tbh I’d feel a bit better considering the second commit to be semver-major?

bnoordhuis · 2016-12-11T17:54:26Z

No strong feelings. semver-major is fine by me.

sam-github · 2016-12-12T15:32:12Z

lib/internal/bootstrap_node.js

@@ -504,7 +513,7 @@
  };

  NativeModule.wrapper = [
-    '(function (exports, require, module, __filename, __dirname) { ',
+    '(function (exports, require, module, __filename, process) { ',


Its confusing that there are two wrappers now, and they are different.

Am I reading this correctly, internal modules no longer have __dirname? I guess it would be too big a change to have process be injected as a module argument for all modules, internal and external, rather than be in globals? Ben, was there discussion about whether node's "globals" should be true-global or module locals? module/exports/__filename/__dirname have meaning only in local context, but why did require() become a local?

Its confusing that there are two wrappers now, and they are different.

That's the price of progress.

Am I reading this correctly, internal modules no longer have __dirname?

They never really did, it was always undefined because built-in modules don't live on disk.

I guess it would be too big a change to have process be injected as a module argument for all modules, internal and external, rather than be in globals?

I'm not against that per se but that's a bigger change and probably needs more discussion.

why did require() become a local?

I believe it always was. It sort of has to be because it works relative to the calling script.

I assumed it knew its current module using some other mechanism, but that makes sense. Also about __dirname.

evanlucas · 2016-12-15T11:41:24Z

@sam-github every time I've seen it happen has been someone naming a function process

jasnell · 2016-12-27T22:52:51Z

@nodejs/ctc ... any thoughts on this?

indutny

LGTM

rvagg · 2016-12-28T07:40:02Z

+0, I can see this causing some hurt but it's probably a good idea

rvagg · 2016-12-28T07:55:05Z

Actually, a thought: could this mess up people who use spies or mocks in place of process in their tests?

bmeck · 2017-01-12T01:07:15Z

I know at one point N|Solid's agent was heavily guarding against this style of attack by creating local const even to perform calls. Might take inspiration from that.

bmeck · 2017-01-12T00:26:34Z

lib/internal/bootstrap_node.js

@@ -196,7 +200,12 @@
      enumerable: false,
      configurable: true
    });
-    global.process = process;
+    Object.defineProperty(global, 'process', {


I don't think this is needed / warranted. If someone wants to delete global.process it should still work. Natives having proper refs to process is already achieved via the wrapper hange.

It was requested by @sam-github in #10135 (comment).

I as well as @ljharb disagree with this change.

basically, if we set this precedent I would be much more comfortable locking down all things that core could depend on, either by adding them to a closure ala https://github.com/bmeck/node/tree/no-globals or by freezing them on the global. There are many of these globals that break npm modules in the wild if they are overwritten. I do not see process as exceptionally high use vs Error for example. I am against freezing them on the global for polyfill reasons.

Are there many? https://nodejs.org/api/globals.html documents only a bit more than a dozen "globals", and half of them aren't global, they are module-scoped variables. Perhaps all the "globals" should be module-scoped variables? So that its impossible to mess with them globally?

several: https://github.com/bmeck/node/blob/no-globals/lib/internal/globals.js

this does not include prototype hijacking protection, thats raw global refs.

bmeck · 2017-01-12T00:27:32Z

test/parallel/test-process-clobber.js

+require('../common');
+const assert = require('assert');
+
+assert.throws(() => process = null, /Cannot assign to read only property/);


I don't think this should throw. Natives should be robust w/o relying on globals once startup finishes

The built-in libraries are, this checks that process is not assignable per the second commit.

I don't think that is appropriate to lock down.

bmeck · 2017-01-12T02:26:00Z

make lint on https://github.com/bmeck/node/tree/no-globals [was] showing more than 8000 unchecked global accesses (ones that don't come from a require)

addaleax · 2017-01-12T17:39:28Z

Btw, Electron ships with a different module wrapper, maybe it’s worth considering a change like the one proposed here?

bnoordhuis · 2017-01-13T11:55:04Z

Btw, Electron ships with a different module wrapper, maybe it’s worth considering a change like the one proposed here?

That issue makes a good point about breaking const process = ... but the proposed solution introduces an extra function closure for every require() call.

sam-github · 2017-01-13T18:07:23Z

While I am guilty of suggesting this, I'm also OK to give node users enough guns to shoot themselves in the foot. Its possible to creatively or accidently corrupt your runtime in many languages, node is no exception, so if we want to leave that as a possibility, I'm OK with it.

ljharb · 2017-01-13T18:11:30Z

The language runtime, which includes process, should indeed behave that way. Core code, however, should not be so fragile as to be easily broken by users doing inadvisable things.

bmeck · 2017-01-13T18:13:56Z

@ljharb unfortunately some popular libraries mutate internal prototypes so to some extent there is a need to preserve some inadvisable behavior.

sam-github · 2017-01-13T18:15:21Z

Unless we freeze all the prototypes of built-in objects, breaking most node monitoring products as a pretty nasty side-effect, the runtime can be broken by users. Its just a question of how hard we want to make them work to do it.

ljharb · 2017-01-13T18:15:37Z

Again, that's fine in the runtime, but core code should not be broken by those changes.

sam-github · 2017-01-13T18:16:43Z

@ljharb I actually don't know what you mean by attempting to distinguish "runtime" and "core node". Those words mean the same thing to me, but apparently not to you, can you define them?

bmeck · 2017-01-13T18:17:03Z

@ljharb some libraries like express change how internals work, in ways that could potentially break core assumptions. Node just was not designed to defend against monkey patching.

ljharb · 2017-01-13T18:22:31Z

@sam-github I'm saying that the behavior of non-spec (ie, non-language) code that the user did not write or install should not be changeable by user code in unintended ways.

@bmeck and I think that "changing how internals work" is something that shouldn't be wantonly permitted (ie, outside of documented/established/endorsed mechanisms to do so), and that if that warrants breaking changes to make node core more robust against third-party code I run making my own code behave in ways I can't defend against (I can defend against language builtins being modified; I can't defend against core being modified), then that's worth it.

bmeck · 2017-01-13T18:24:18Z

I suggest adding a new CTC discussion around the definition of robust as a goal here and if it is limited to some subset of globals used by Core.

ChALkeR · 2017-01-18T05:34:14Z

Note that a significant amount of packages seem to override global.process under various circumstances.

Examples:

Some others do global.process = require('process'), probably for browserifying reasons.

ChALkeR · 2017-01-25T16:22:59Z

CITGM: https://ci.nodejs.org/job/citgm-smoker/544/

ChALkeR · 2017-02-02T12:59:42Z

I expect making global.process non-writable will break some things (most of which are testcases), including npm tests. It doesn't look like those are ran during a CI or CITGM, so they should be manually triggered, I presume. /cc @MylesBorins

targos · 2017-02-02T14:10:05Z

why don't we just add const process = require('process') in all the lib files where it's needed, and make sure the process module is loaded before executing any user code?

jasnell · 2017-03-22T22:12:21Z

@bnoordhuis ... how do you want to proceed with this one?

bnoordhuis · 2017-04-19T09:04:42Z

I'm going to close and revisit when I have time.

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. labels Dec 6, 2016

mscdex removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 6, 2016

jasnell reviewed Dec 6, 2016

View reviewed changes

addaleax approved these changes Dec 6, 2016

View reviewed changes

addaleax added lts-watch-v4.x labels Dec 6, 2016

cjihrig approved these changes Dec 6, 2016

View reviewed changes

bnoordhuis added 2 commits December 11, 2016 15:12

lib,src: make process global non-writable

54aec1e

bnoordhuis force-pushed the process-clobber-protection branch from d3a744b to 54aec1e Compare December 11, 2016 15:06

nodejs-github-bot added the dont-land-on-v7.x label Dec 11, 2016

sam-github reviewed Dec 12, 2016

View reviewed changes

italoacasas added the wip Issues and PRs that are still a work in progress. label Dec 12, 2016

sam-github approved these changes Dec 12, 2016

View reviewed changes

addaleax removed the dont-land-on-v7.x label Dec 13, 2016

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 27, 2016

indutny approved these changes Dec 27, 2016

View reviewed changes

addaleax removed the lts-watch-v4.x label Dec 28, 2016

bmeck suggested changes Jan 12, 2017

View reviewed changes

bmeck mentioned this pull request Jan 18, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-01-18 nodejs/CTC#61

Closed

rvagg added the ctc-agenda label Jan 18, 2017

Trott mentioned this pull request Jan 23, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-01-25 nodejs/CTC#63

Closed

Trott mentioned this pull request Jan 31, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-02-01 nodejs/CTC#65

Closed

Trott mentioned this pull request Feb 7, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-02-08 nodejs/CTC#68

Closed

bnoordhuis removed the ctc-agenda label Feb 7, 2017

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

bnoordhuis closed this Apr 19, 2017

bmeck mentioned this pull request Jul 26, 2017

module: Allow runMain to be ESM #14369

Closed

4 tasks

lib: be robust when process global is clobbered #10135

lib: be robust when process global is clobbered #10135

Conversation

bnoordhuis commented Dec 6, 2016 • edited Loading

jasnell commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 6, 2016

bnoordhuis commented Dec 6, 2016

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Dec 6, 2016

bnoordhuis commented Dec 11, 2016 • edited Loading

addaleax commented Dec 11, 2016

bnoordhuis commented Dec 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Dec 15, 2016

jasnell commented Dec 27, 2016

indutny left a comment

Choose a reason for hiding this comment

rvagg commented Dec 28, 2016

rvagg commented Dec 28, 2016

bmeck commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Jan 12, 2017 • edited Loading

addaleax commented Jan 12, 2017

bnoordhuis commented Jan 13, 2017

sam-github commented Jan 13, 2017

ljharb commented Jan 13, 2017

bmeck commented Jan 13, 2017

sam-github commented Jan 13, 2017

ljharb commented Jan 13, 2017

sam-github commented Jan 13, 2017

bmeck commented Jan 13, 2017

ljharb commented Jan 13, 2017

bmeck commented Jan 13, 2017

ChALkeR commented Jan 18, 2017 • edited Loading

ChALkeR commented Jan 25, 2017

ChALkeR commented Feb 2, 2017

targos commented Feb 2, 2017

jasnell commented Mar 22, 2017

bnoordhuis commented Apr 19, 2017

bnoordhuis commented Dec 6, 2016 •

edited

Loading

bnoordhuis commented Dec 11, 2016 •

edited

Loading

bmeck Jan 13, 2017 •

edited

Loading

bmeck commented Jan 12, 2017 •

edited

Loading

ChALkeR commented Jan 18, 2017 •

edited

Loading