child_process: fix incomplete prototype pollution hardening

[lirantal]

Prior pull request (#48726) hardened against prototype pollution vulnerabilities but effectively missed some use-cases which opened a window for prototype pollution for some child_process functions such as spawn(), spawnSync(), and execFileSync().

This PR adds a (failing) test case that confirms the issue and follows-up with a fix for the bug in child_process functions to ensure consistent behavior.

Note: this is not considered a security vulnerability because the Node.js threat model does not recognize prototype pollution as a viable security vulnerability in the runtime.

Expectation vs Actual

Expectation:

• Per the spawn() API documentation, spawn() should default to shell: false. Similarly, execFile() follows the same.
• Per the referenced prototype pollution hardedning Pull Request from 2023, the following simulated attack shouldn't work: Object.prototype.shell = true; child_process.spawn('ls', ['-l && touch /tmp/new'])

Actual:

• Object.prototype.shell = true; child_process.execFile('ls', ['-l && touch /tmp/new']) - ✅ No side effects, hardening works well for.
• Object.prototype.shell = true; child_process.spawn('ls', ['-l && touch /tmp/new']) - ✅ No side effects, hardening works well for.
• Object.prototype.shell = true; child_process.execFile('ls', ['-l && touch /tmp/new'], { stdio: 'inherit'}) - ✅ No side effects, hardening works well for.
• Object.prototype.shell = true; child_process.spawn('ls', ['-l && touch /tmp/new'], { stdio: 'inherit'}) - ❌ Vulnerability manifests, hardening fails.

Note: other APIs are also prone to this issue: execFileSync and spawnSync.

Demo of the inconsistent prototype hardening findings

The following is provided as an easy reproduction and proof-of-concept (POC) for described findings on the prototype mishandling issue:

const { execFile, spawn, spawnSync, execFileSync } = require("child_process");

// Simulate a successful prototype attack impact:
const a = {};
a.__proto__.shell = true;

// Show the prototype is indeed affected:
console.log("Object.shell value:", Object.shell, "\n");

// Node.js user-land code use of various child_process functions:
execFile("ls", ["-l && touch /tmp/from-ExecFile"], {
  stdio: "inherit",
});

spawn("ls", ["-la && touch /tmp/from-Spawn"], {
  stdio: "inherit",
});

execFileSync("ls", ["-l && touch /tmp/from-ExecFileSync"], {
  stdio: "inherit",
});

spawnSync("ls", ["-la && touch /tmp/from-SpawnSync"], {
  stdio: "inherit",
});

The output would be as follows:

$ node app.js
Object.shell value: true

[...]

$ ls -alh /tmp/from*
Permissions Size User     Date Modified Name
.rw-r--r--     0 lirantal  4 Jul 14:14   /tmp/from-ExecFileSync
.rw-r--r--     0 lirantal  4 Jul 14:14   /tmp/from-Spawn
.rw-r--r--     0 lirantal  4 Jul 14:14   /tmp/from-SpawnSync

Demonstrating that while the child_process.exec function is hardened from prototype chain look up of the shell property, this isn't the case for other process execution functions, mainly: spawn, execFileSync, spawnSync.

[lirantal]

So as the described in the pull request, I've initially added a test case to show it is failing. Not sure how long previous build logs are kept so going to slot in a screenshot too for the build failure of the prototype test of the newly added test cases

and will now progress with updating the PR with the code fix

[H4ad]

Can you also include the fix for the test in the same PR and commit?

[RedYetiDev]

Note: this is not considered a security vulnerability because the Node.js threat model does not recognize prototype pollution as a viable security vulnerability in the runtime.

For Reference:
"Node.js trusts the inputs provided to it by application code. It is up to the application to sanitize appropriately. Therefore any scenario that requires control over user input is not considered a vulnerability."

[lirantal]

Can you also include the fix for the test in the same PR and commit?
@H4ad added now

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[lirantal]

Looks like all tests except node-test-commit-arm-fanned and node-test-binary-armv7l are passing. Is this expected given that these two are flakey?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 47b8779