module: isolate paths expecting to only use non-internals

[bmeck]

Create a choke point so that non-internal module access can
be done discretely from internal module access. Non-internal
access through nonInternal will always check visibility in
a way that is not succeptible to prototype mutation.

Due to internal nature of things, this isolation from proto
mutation is not reasonably testable. An example of getting
internals prior to this patch is:

  let _, s = 'startsWith',
    sp = String.prototype,
    op = Object.prototype,
    sw = sp[s],
    h = 'hasOwnProperty',
  hp = op[h];
  sp[s]=()=>false;
  op[h]=()=>true;
  _ = require('internal/child_process');
  sp[s]=sw;
  op[h]=hp;
  console.dir(_, {depth: null, colors: true});

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

[nodejs-github-bot]

@bmeck build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2235/pipeline

[Trott]

Due to internal nature of things, this isolation from proto
mutation is not reasonably testable.

An end-to-end test that monkey-patching can't result in access to internal modules is perhaps not reasonable, but maybe a test that simply makes sure internal loading doesn't use a monkey-patched startsWith() or hasOwnProperty() at all along the lines of something like this?:

'use strict';
const common = require('../common');

const originalStartsWith = String.prototype.startsWith;
const originalHasOwnProperty = Object.prototype.hasOwnProperty;

String.prototype.startsWith = common.mustNotCall();
Object.prototype.hasOwnProperty = common.mustNotCall();

// This next line is the test. It should not throw because it should not use
// monkey-patched prototype methods.
require('child_process'); 

// Restore in case additional test cases are added or they are needed
// in exit handlers added by `common`, etc.
String.prototype.startsWith = originalStartsWith;
Object.prototype.hasOwnProperty = originalHasOwnProperty;

Can/should probably be extended to other prototype methods, so it could get onerous fast, but maybe just that as a smoke test, at least for now? (I've confirmed that the above will fail with current master and pass with these changes, FWIW.)

[addaleax]

@bmeck I assume this had some conflicts with #25352?

[bmeck]

@addaleax yea, it appears so, I can rebase this off it and add tests for prototype usage like @Trott said.

[sam-github]

This PR could use a better description, or maybe some docs if it affecs how Module.require works?

The code in the description is "before" code, but it doesn't have accompanying "after code, with output", so doesn't really help understand what this PR changes.

From Rich's example, I gather the intention is to make it so that if js builtin's (or anything? its not clear) have their prototypes modified (just prototypes?), node core will not notice the new prototype property values, and will use the original values?

[guybedford]

Great work catching this. I really hope this can be made easier in JS soon.

[bmeck]

@sam-github

The code does run and return the exports of the internal module on w/e version of Node it is run on.

> pbpaste | node # v10.14.2
{ ChildProcess:
   { [Function: ChildProcess]
     super_:
      { [Function: EventEmitter]
        EventEmitter: [Circular],
        usingDomains: false,
        defaultMaxListeners: [Getter/Setter],
        init: [Function],
        listenerCount: [Function] } },
  setupChannel: [Function: setupChannel],
  _validateStdio: [Function: _validateStdio],
  spawnSync: [Function: spawnSync] }

Afterwards it does the expected error of trying to get a hold of internal modules:

Pre-patch error:

> node -e "require('internal/child_process');"
internal/modules/cjs/loader.js:583
    throw err;
    ^

Error: Cannot find module 'internal/child_process'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at [stdin]:1:1
    at Script.runInThisContext (vm.js:96:20)
    at Object.runInThisContext (vm.js:303:38)
    at Object.<anonymous> ([stdin]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:586:27)

Post-patch error:

> pbpaste | ./node
internal/modules/cjs/loader.js:598
    throw err;
    ^

Error: Cannot find module 'internal/child_process'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:596:15)
    at Function.Module._load (internal/modules/cjs/loader.js:525:25)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at [stdin]:9:7
    at Script.runInThisContext (vm.js:123:20)
    at Object.runInThisContext (vm.js:312:38)
    at Object.<anonymous> ([stdin]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:714:30)
    at evalScript (internal/process/execution.js:56:25)

From Rich's example, I gather the intention is to make it so that if js builtin's (or anything? its not clear) have their prototypes modified (just prototypes?), node core will not notice the new prototype property values, and will use the original values?

For some subset of this type of preventing user code from getting to internals via prototypes and global state mutation, yes; this PR is not all encompassing however, and lots of things are leaking references to internal constructors/bindings etc. elsewhere. This just plugs the direct require path to things, lots more would be required if we are intending to lock down arbitrary access to powerful bindings entirely.

[sam-github]

For others following after, @bmeck example code is demonstrating that on 10.x it is possible to directly require internal modules. Its not supposed to be possible, but by replacing some String and Object methods, we can evade the module loading system's checks, at least on 10.x:

const startsWith = String.prototype['startsWith'];
const hasOwnProperty = Object.prototype['hasOwnProperty'];

String.prototype['startsWith']=()=>false;
Object.prototype['hasOwnProperty']=()=>true;

const _ = require('internal/child_process'); // Should throw an error unless --expose-internals is supplied.

String.prototype['startsWith']=startsWith;
Object.prototype['hasOwnProperty']=hasOwnProperty;

console.dir(_, {depth: null, colors: true});

With 10.x, the require won't throw. After this PR, it will throw, as requires of internals are supposed to.

It's a bit obscured, because with node 12.0.0 this example does (as expected) throw, so either the PR is no longer applicable to master because the problem was solved some other way, or possibly the module internals have changed enough that the PR doesn't apply?

Its somewhat confusing to tell.

@Trott 's example tries (I think) to not depend so carefully on attacking the module internals by rewriting prototypes, and instead just tries to damage the prototype methods (by making them always throw errors), and then show that since the public child_process module can be required, the damaged methods must not have been called.

Except as far as I can tell, my variation on his test fails on this PR, though I assume it must have worked for him originally (see below).

Am I understanding this PR's intention better now?

What is its state, @bmeck, will you rebase this and get it landed?

core/node (pr/25449 $%) % nvm run 12 ex-25449-2.js
Running node v12.0.0 (npm v6.9.0)
/home/sam/w/core/node/ex-25449-2.js:3
String.prototype.startsWith = () => { throw new Error('X'); };
                                      ^

Error: X
    at String.startsWith (/home/sam/w/core/node/ex-25449-2.js:3:45)
    at NativeModule.compile (internal/bootstrap/loaders.js:299:31)
    at NativeModule.compileForPublicLoader (internal/bootstrap/loaders.js:220:8)
    at Function.Module._load (internal/modules/cjs/loader.js:537:16)
    at Module.require (internal/modules/cjs/loader.js:666:19)
    at require (internal/modules/cjs/helpers.js:16:16)
    at Object.<anonymous> (/home/sam/w/core/node/ex-25449-2.js:8:1)
    at Module._compile (internal/modules/cjs/loader.js:759:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
    at Module.load (internal/modules/cjs/loader.js:628:32)
core/node (pr/25449 $%) % ./out/Release/node ex-25449-2.js
/home/sam/w/core/node/ex-25449-2.js:4
Object.prototype.hasOwnProperty = () => { throw new Error('X'); };
                                          ^

Error: X
    at Object.hasOwnProperty (/home/sam/w/core/node/ex-25449-2.js:4:49)
    at Function.NativeModule.exists (internal/bootstrap/loaders.js:243:31)
    at Function.NativeModule.require (internal/bootstrap/loaders.js:213:21)
    at Object.require (internal/bootstrap/loaders.js:393:27)
    at Function.Module._load (internal/modules/cjs/loader.js:535:24)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/home/sam/w/core/node/ex-25449-2.js:8:1)
    at Module._compile (internal/modules/cjs/loader.js:714:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:725:10)
core/node (pr/25449 $%) % cat ex-25449-2.js
'use strict';

String.prototype.startsWith = () => { throw new Error('X'); };
Object.prototype.hasOwnProperty = () => { throw new Error('X'); };

// This next line is the test. It should not throw because it should not use
// monkey-patched prototype methods.
require('child_process'); 

[BridgeAR]

@sam-github

It's a bit obscured, because with node 12.0.0 this example does (as expected) throw, so either the PR is no longer applicable to master because the problem was solved some other way, or possibly the module internals have changed enough that the PR doesn't apply?

We use Primordials for a couple of things internally, so it's solved differently. I'll close this even since we try to switch more and more code to use the Primordials.