process: set process.env prototype to null

[watson]

This removes all Object.prototype inherited features from process.env which make this a breaking change (e.g. hasOwnProperty(), toString() etc). Due to this, it might not be feasible to land this PR, but I wanted to create it, if nothing else, as a forum where we could have the discussion (I could have made an issue, but already had the code ready).

Why?

This is a naive attempt to protect Node.js apps a little better against prototype pollution attacks. Environment variables are often used to configure an application. They are also used to configure Node.js itself (e.g. NODE_OPTIONS).

Since prototype pollution affects all normal objects in Node.js, you could argue that we shouldn't treat process.env in a special way. The reason I went down this rabbit hole anyway was that process.env is so powerful that it might deserve special treatment.

It came to my attention in #30008 that spawn and friends from the child_process module defaults to using process.env if no special env property is given in options. This means that all child processes spawned can be easily attacked using prototype pollution. This of course also applies to the main process if process.env.* is checked at runtime. So while the particular issue with child processes will be fixed by #30008, I felt it might be worth it fixing this for process.env in general.

Performance

@joyeecheung raised a valid point in #node-dev on IRC about whether or not property access performance of process.env would be impacted if we removed the prototype. It seems that since the object is created in C++ land, this doesn't affect it:

$ ./node --allow-natives-syntax
Welcome to Node.js v13.0.0-pre.
Type ".help" for more information.
> Object.getPrototypeOf(process.env)
null
> %HasFastProperties(process.env)
true

If we want to continue with this, we should consider running the benchmarks as well.

State of this PR

My C++ code might not be ideal. But at least it compiles and gets the point across. There is one failing test in test/parallel/test-process-env.js, as that actually checks if process.env inherits from Object.prototype:

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

Lines 41 to 42 in 31217a8

	assert.strictEqual(Object.prototype.hasOwnProperty,
	process.env.hasOwnProperty);

I traced that test back to fee02db, where it also seems like inheritance from Object.prototype was introduced (if I'm reading the C++ correctly).

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

[addaleax]

It seems that since the object is created in C++ land, this doesn't affect it:

I don't think that check actually gives relevant information -- property accesses will still go through C++ first anyway, so the engine will not be able to fully optimize them with or without this patch

[devsnek]

if someone can pollute your prototypes can't they also just fs.readFile(./allYourSecrets)

[addaleax]

This LGTM code-wise, but … I’ve thought about doing this in the past myself, and it didn’t quite seem worth the risk of breakage, esp. with hasOwnProperty(), as unfortunate as that is.

Would it maybe make sense to use a prototype chain of process.env → { hasOwnProperty: Object.prototype.hasOwnProperty } → null rather than process.env → null? It’s a bit unorthodox but it might actually be okay here?

[watson]

@devsnek not directly. They need to find a path from prototype pollution to remote code execution (RCE) - one which isn't always available. In #30008 I'm fixing one such path, but others almost certainly exist that we just haven't found yet.

But don't get me wrong. If you have an app that's vulnerable to prototype pollution an attacker can probably do a lot of damage - if you're luckey they'll just crash the app, if you're unluckey they have found a path to do RCE. That's why I'm mostly trying to plug those holes.

[devsnek]

@watson to clarify, this is a protection against code that is not in the same process/machine?

[watson]

@devsnek I'm not sure if I understand the question fully, so I'm sorry if my answer isn't what you're looking for. This PR is about protection against prototype pollution of the process.env object of the current process.

[watson]

@addaleax

This LGTM code-wise, but … I’ve thought about doing this in the past myself, and it didn’t quite seem worth the risk of breakage, esp. with hasOwnProperty(), as unfortunate as that is.

Would it maybe make sense to use a prototype chain of process.env → { hasOwnProperty: Object.prototype.hasOwnProperty } → null rather than process.env → null? It’s a bit unorthodox but it might actually be okay here?

Really interesting idea. I like it! I'll look more into this...

[Qard]

What about something like this:

const lockedPrototype = Object.create(null)
Object.defineProperties(lockedPrototype, Object.getOwnPropertyDescriptors(Object.prototype))
Object.freeze(lockedPrototype)
Object.setPrototypeOf(process.env, lockedPrototype)

That gives you the full Object API, but locked to its initial state.

[BridgeAR]

What's the status here?

[jasnell]

@watson ... unfortunately this appears to have stalled. Closing but we can reopen if it is picked back up again