child_process: harden the API

[watson]

Ensure that the options object used by exec, execSync, execFile, execFileSync, spawn, spawnSync, and fork, isn't susceptible to prototype pollution.

This is achieved by copying all the properties of the options object into another object that doesn't have a prototype.

Background

If an attacker is able to successfully pollute the prototype just before a call to exec, execSync, execFile, execFileSync, spawn, spawnSync, or fork, they will be able to perform RCE on most Linux systems by manipulating the env option.

Recommended alternative

If this PR doesn't land, or if you're running a version of Node.js that doesn't include this change, the recommended way to spawn a child process is:

const options = Object.create(null)
options.env = Object.assign(Object.create(null), process.env)
spawn(command, options)

By actively passing in an options object that contains an env property you ensure that one isn't created internally which inherits from Object.prototype.

The above example uses spawn, but this approach is also relevant for exec, execSync, execFile, execFileSync, spawnSync, and fork.

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
• Run child_process benchmarks

[nodejs-github-bot]

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

too quick

[vdeturckheim]

LGTM regarding the code and the behavior - I forsee it can be a braking change for some people. Would doc be enough to mitigate it?

[watson]

@vdeturckheim Breaking in what way? I was originally thinking it would be breaking if users relied on the prototype chain for some of the properties. But { ...options } strips that anyway and leaves only Object.prototype, so I concluded this wasn't a problem. But I might be overlooking something?

[vdeturckheim]

@watson nop, I was believing that too. So LGTM! Thanks for the clarification!

[vdeturckheim]

LGTM

[watson]

Force pushed to fix linting errors... I wonder why my tests locally didn't catch these - I guess make test-only doesn't run the linter

[watson]

@vdeturckheim I just took a second look at the code regarding your original concern, and technically normalizeSpawnArguments doesn't do the ...options thing until after it has used the options object to check a few things. But if anybody tried to rely on this, it would probably not be very stable anyway, so I'm not sure anybody actually does that in real life? 🤔

[nodejs-github-bot]

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

[watson]

More linting errors 🙄 I found that neither make test nor make test-only runs the linting locally despite https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests saying so. Only make lint does. Anyway, now it should be good 👍

[nodejs-github-bot]

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

[richardlau]

More linting errors 🙄 I found that neither make test nor make test-only runs the linting locally despite https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests saying so. Only make lint does. Anyway, now it should be good 👍

It does run the linting but any failures are being masked. #30012 should fix that.

[watson]

Looks like the windows tests are filing, but I'm having trouble seeing exactly why. It's not clear to me from the logs, but maybe I'm looking at the wrong logs. E.g. is this the right one? https://ci.nodejs.org/job/node-test-binary-windows-2/3579/console

[richardlau]

Looks like the windows tests are filing, but I'm having trouble seeing exactly why. It's not clear to me from the logs, but maybe I'm looking at the wrong logs. E.g. is this the right one? https://ci.nodejs.org/job/node-test-binary-windows-2/3579/console

https://ci.nodejs.org/job/node-test-binary-windows-2/3579/testReport/

[watson]

There was an oversight in my original patch, which I just fixed and pushed. Unfortunately, that revealed that we actually do "have official support" for inheriting environment variables in the child_process module via the prototype. Support for this was added in an old PR: nodejs/node-v0.x-archive#713

Today the test manipulates the prototype like this:

node/test/parallel/test-child-process-env.js

Lines 35 to 37 in d769ebc

	Object.setPrototypeOf(env, {
	'FOO': 'BAR'
	});

And expects FOO to be present here:

node/test/parallel/test-child-process-env.js

Line 58 in d769ebc

assert.ok(response.includes('FOO=BAR'));

If we want to keep support for this (which I guess we have to if we don't want to make this a breaking change), one solution might be to keep the entire prototype chain, except the very last one that points to Object.prototype. Would that be an acceptable solution?

[gireeshpunathil]

@watson -

If an attacker is able to successfully pollute the prototype

can you please elaborate a little more on the attack surface? If an attacker has access to the said objects, can't they very well initiate any arbitrary code in the process?

[watson]

@gireeshpunathil

can you please elaborate a little more on the attack surface? If an attacker has access to the said objects, can't they very well initiate any arbitrary code in the process?

Prototype pollution can't by itself be used to do RCE, but given the right set of circumstances in the code base, you might be able to find a way to leverage the polluted prototype to perform RCE.

The child_process module is especially powerful as a polluted prototype will spill over into the environment variables of the spawned child processes. If the child process being run is itself a node program, this can easily be used to perform an RCE attack. For details see this deck.

[sam-github]

I think this change needs to touch the docs. If console.log(options.whatever); child_process.spawn(options) does not actually use options.whatever because it wasn't a direct property, people need to be told.

I've mixed feelings about this. I think using prototype inheritance on options objects is fairly rare, so I'm OK losing it as a feature. On the other hand... having a node API surface that sometimes allows it and sometimes doesn't seems pretty horrid. Should we do the check everywhere?

[devsnek]

Should we do a benchmark run for the child_process APIs? I assume this would hit them a bit.

[watson]

@sam-github

I think this change needs to touch the docs.

Good idea. I'll update the docs 👍

@devsnek

Should we do a benchmark run for the child_process APIs? I assume this would hit them a bit.

Good point. Once we have the final version of the PR ready we should run the child_process benchmarks 👍

[BridgeAR]

What's the status here?

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.