prototypes and libraries are unsafe inside node core libs

[devsnek]

In a PR i'm working on @ljharb pointed out several cases where using things like String.prototype.replace or fs.readFileSync are unsafe because user code could override them, forcing me to use things like const StringReplace = Function.call.bind(String.prototype.replace) and use that instead. A fair amount of node code uses this pattern, and a fair amount doesn't guard against this at all. I opened this issue to create a discussion about what the pattern should be moving forward, if there are things we can do to prevent this behavior from affection core libs, etc.

[ljharb]

cc @bmeck
@devsnek in that PR, you'll note a few lines with Function.call.bind( - that's an attempt to make things robust.

[devsnek]

as an alternative to creating a direct reference to every single prototype method or library method we may want to call, perhaps we can shadow the globals in nativemodule wrapper so that they stay scoped to the context that the vm runs them in?

(function (exports, require, module, internalBinding, process, { String, Array, Object, JSON, etc }) {

or if the Function.call.bind is good enough for people, there are a lot of places in the source where it isn't used

[ljharb]

@devsnek that would ensure you retain a reference to String, but anyone could later mutate String.prototype or any of its properties.

[devsnek]

wouldn't it protect from different contexts? unless i'm doing something wrong here

[ljharb]

Oh, maybe I'm misunderstanding. I thought node core code doesn't tend to run in a vm? If it's running in a vm, then there'd be different problems, unless that vm's globals were all deep-frozen.

[devsnek]

i also posted a bad example, i was thinking more like getting a bunch of safe globals like below and then passing them to core libraries,

although i guess unless you actually assign those passed things as globals in the context they won't be very useful for most cases (i need to think this through more 😄)

[mscdex]

I would tread very carefully as you never know who relies on the ability to monkey patch such things...

[ljharb]

Certainly changing old code in this way might be breaking; but ensuring that new code is robust should be safe.

[hashseed]

You could implement your stuff in a new vm context and use vanilla objects from there, but that doesn't seem practical.

[ljharb]

@hashseed that opens new problems, because it might become possible for users to traverse from the objects created, to those vm primordials, exposing them to the same hazard.

[hashseed]

Right. You could have to hold onto the actual functions and perform Function.prototype.call on them. Same goes for Function.prototype.call itself, obviously.

[tniessen]

I might be missing something here, but why would we try to "protect" node.js core from such modifications? If people monkey-patch existing prototypes etc., then any breakage will be their fault, and the vm module is not supposed to protect the process against untrusted code, so the same rule applies there.

[ljharb]

@tniessen If node breaks as a result of JS code running, it's node's fault. Since a node app runs with JS (and C) code written by many authors, it's simply not true that the user experiencing the breakage will necessarily be that user's fault.

[devsnek]

(a good example right now is doing String.prototype.replace = () => '' in a repl, and then trying to continue using repl)