Fixing child_process module to check values passed strictly to the options object

[lirantal]

Issue

The way the child_process module currently works with validating the extra options passed to it is by accessing the properties on the options object. However, there is no strict check to validate that the property has been defined on the direct object, and so what happens is that when accessing the property it will bubble up to the prototype chain until it hits the Object prototype and use that.

It draws attention to a possible security implication where-as the prototype chain can be manipulated and thus triggering a further vulnerability with the child_process module to execute via shell expansion which allows to execute arbitrary commands.

Reproduce

Steps To Reproduce:

1. Create a snippet file as follows:

  const { spawn } = require('child_process');
  Object.prototype.shell = true;
  const cmd = 'ls'
  const args = ['; echo baz']
  const ls = spawn(cmd, args, {stdio: 'inherit'}, function(a,b,c) {
    console.log(a, b, c)
  });

2. Run it
3. Observe the baz word added to the output even though the options passed to spawn don't directly include the shell option

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

[lirantal]

/cc @cjihrig @mcollina following up on the previously submitted report.

[mcollina]

Tagging as semver-major because of possible breakage.

I think this change should be documented.

[cjihrig]

Generally LGTM. It would be nice to try cloning the options with Object.create(null). If that works, we could avoid needing to check hasOwnProperty() for shell and potentially other options in the future.

[lirantal]

I think this change should be documented.

@mcollina Sure. Could you share with me what you were thinking in terms of documenting it, and where?

[lirantal]

@cjihrig I updated the tests and code, everything is passing locally so I pushed the changes.

[Trott]

This needs one more @nodejs/tsc approval and a CITGM run.

[rvagg]

Isn't there a rabbit hole here? if we accept this then won't we be up for changing a lot of options objects? Is there something that can strictly isolate this to child_process that couldn't be argued for any other API? If it's about security, then what about vm where you could slip in some cachedData via the same method? And why are you letting wild prototype chains near your options objects anyway? Node has a long history of caveat emptor on such matters, Node isn't a sandbox, it's not your browser.

Don't get me wrong, I'd love if all our APIs were really strict, on own-properties as well as types, but they aren't and this is a really big ship to turn around so we'd better be absolutely sure we're up for that pain.

[mcollina]

LGTM

[mcollina]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1642/
CI: https://ci.nodejs.org/job/node-test-pull-request/18881/

[mcollina]

@rvagg I don't think we should do this everywhere, but in this specific case this makes harder to escalate a security vulnerability on an http route for example.

[rvagg]

@mcollina so do you have a limiting principle? Or are you just happy to roll on a case-by-case basis? I'm not a fan of pragmatism on such matters and would rather have a principle which we can apply across the board. Otherwise we're in for (a) a lot of discussion and argument for the many other options and (b) lots of code churn and semver-major breakage because we don't have a limiting principle and will therefore likely be applying similar changes across our API (vm is another example where you could make a "security" case but I reckon it wouldn't be hard to make such a case broadly across our API).

[refack]

IIUC this is an issue that crops up every once in a while (#12956, and #12981) in that the core lib lives in the same domain as user code, so monkey-patching can violate many assumptions.
For example in this case the user can hijack Object.assign and circumvent this guard?

> Object._assign = Object.assign;
> Object.assign = (a, b) => Object._assign(a, b, {shell: true})
> var o = Object.assign(Object.create(null), {a:1})
> o
[Object: null prototype] { a: 1, shell: true }

[lirantal]

I understand that the counter-argument is that the prototype chain is the building block of the language and where you're going with theoretically requiring to make this change elsewhere as well.

With that said, this type of issue came up through a related vulnerability reported to the ecosystem, which I later brought up to the node core security team and eventually agreed to accept it as a pull request that conforms with secure coding practices rather than an actual vulnerability. At the very least, the programmer's intention in the context of the modified code was to explicitly check a property was set on the argument it received.

[mcollina]

@rvagg I don't, and I think this needs to be done on a case-by-case basis. I'm fine in not landing this as long as we spell clearly that this type of things are not a security vulnerability in our threat model.

Do you want to talk about this in the next tsc meeting?

[rvagg]

I think TSC discussion would be a good thing, this is one of those topics that has broader impact.

[mcollina]

I'm -1 on landing this without a @nodejs/tsc discussion about this topic.

(I'm ok with the actual change, and this -1 is only to avoid this being landed without that discussion)

[joyeecheung]

@lirantal Can you elaborate on whether this particular API is more vulnerable than the others that also accept an option object? If it's not any more vulnerable in generable, or does not have more serious security implications, then from a maintenance viewpoint I'd prefer we do this in a central place for all the options (or most of them) in our API.

[gabrielschulhof]

Object.prototype and an own property are not the only two places where a value can be stored. There may be a legitimate prototype chain such that there is a prototype in between Object.prototype and the object itself. IOW a property on the options object may legitimately not be an own property. With this change we'd be expressing an opinion that doesn't really give us any more security because of all the other changes we'd have to make, like also addressing #24267 (comment).

[mcollina]

In any case, it would be good to have our threat model documented in https://github.com/nodejs/node/blob/master/SECURITY.md.

[mcollina]

@lirantal would you be able to join the next TSC meeting to present this pull request, and why doing this change is worthwhile?

[ChALkeR]

@mcollina @gabrielschulhof Do you have any example that would be broken by this particular change?

---

That said, protecting against malicious Object.prototype modifications is not much useful in general.

1. Object.prototype modification is not trivial to achieve. Yes, I am aware about bugs in some libraries that made that possible — but those was security issues that were possible to be fixed.
2. Just too much could be broken with malicious Object.prototype modifications — not only Node.js core, but most of the libraries out there on npm do not expect that and could be terribly broken in a similar way once the attacker achieves Object.prototype modification. For real-world apps, this fix would most likely cover a tiny portion of the attack surface in that aspect.

I don't think that hardening against malicious Object.prototype modifications is achievable to a sufficient level or that it is worth the amount of changes in all the ecosystem libs. Knowing that fixing the bugs that allow that modification is relatively simple.

I am not against this particular PR, in fact, I lean forward to that we should land it (if it won't cause actual breakage).
But I don't think that it's worth rewriting everything to safeguard against Object.prototype modifications.

[mcollina]

I'm -1 only for having this discussion. I'm +0 on the change itself.

[lirantal]

Apologies for not replying sooner, my e-mail inbox didn't receive a lot of love recently.
Thanks everyone for chiming in on this thread. Just having the security discussion going around to increase awareness is worth it, regardless of the proposed change. I like @mcollina idea with regards to the threat modeling work for Node.js, which sounds like a good task for the @nodejs/security-wg folks with the help of the TSC.

I believe the thread comments summarize it well - an unauthorized access to manipulate the prototype chain will probably break many things anyway, and I don't think we're trying to solve here prototype manipulations across all of node.

If we're not 100% to land because of code conventions between this place and others in the code-base I'm ok, but at the very least the change proposed in this PR is making senseful code practice.

[mhdawson]

@lirantal sorry for the late notice but do you have time to come to the TSC meeting Tomorrow (Wednesday) at 4EST to participate in a discussion on this?

[lirantal]

No worries, I should be able to make it.

[mhdawson]

@lirantal I'll send you the information to join through email.

[lirantal]

Sounds good, thanks!

[sam-github]

I think that if console.log(options.SHELL) prints a string, then child_process.execFile(options) should use that string for the shell. Anything else is strange and unintuitive. If every node API was like that, people might learn to expect it, but if it is only child_process that ignores the options passed to it then the API behaviour would be pretty weird.

[lirantal]

I can relate to that Sam.

[Fishrock123]

I think that if console.log(options.SHELL) prints a string, then child_process.execFile(options) should use that string for the shell. Anything else is strange and unintuitive. If every node API was like that, people might learn to expect it, but if it is only child_process that ignores the options passed to it then the API behaviour would be pretty weird.

I agree with Sam.

From what I can tell, the primary base concern here is that a theoretical vulnerability here would be harder to detect than just accessing child_process directly. However, any code that can do that can _(easily enough, even to hide, you did see the event-stream vuln, right?) just access child_process itself if you want to.

Back to Sam's quote: Really, there are a great number of things inherent in how JavaScript works where similar issues manifest. JavaScript itself could be 'more secure' in this and similar ways but that is a job that is on a different level than us, IMO.

[rvagg]

Is it worth considering something like util.strictify() or child_process.strict in the same pattern as util.promisify() and fs.promises. A way to take our current API and apply a strict argument checking layer to it. Check for strict types, hasOwn on options properties and everything else we can do to enforce explicitness. Make the API what it should have always been but opt-in.
Of course it increases our API surface area by quite a bit and would probably result in test suite bloat, so that's not great.
Another option might be to make all of the new promises APIs, like the ones in fs.promises and dns.promses (which are experimental and can change) strict in this same way. New API, new sanitization.

[ChALkeR]

/cc @nodejs/security-wg, just in case if there is more input on this topic.

Upd: sorry, hit the wrong button. :-/

[mhdawson]

This was discussed in the TSC meeting today and the net of the discussion is that the TSC does not believe that the PR should land as the Node.js security model does not protect against these kinds of attacks and changing that in one small way while leaving the other unchanged does not add enough value to outweigh the risk of breaking existing code or setting a precedent for many similar changes.

[mhdawson]

Removing TSC agenda as we should have removed after the discussion in the last meeting.

[lirantal]

We discussed in the TSC meeting that this shouldn't be regarded as something to be fixed and is expected behavior. Closing.