GeezerVison

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

Tagging as semver-major because of possible breakage.

I think this change should be documented.

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?

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

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

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.

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

@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.

@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).

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 }

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.

@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?

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

@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.

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

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

@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.

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

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.

@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?

No worries, I should be able to make it.

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

Sounds good, thanks!

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 can relate to that Sam.

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.

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

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

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.

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

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