Skip to content

process: set process.env prototype to null

Rodrigo Muino Tomonari requested to merge github/fork/watson/harden-process-env into master

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:

https://github.com/nodejs/node/blob/31217a8e88d7414579284267f8715112bf8a0fc6/test/parallel/test-process-env.js#L41-L42

I traced that test back to fee02db7, 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

Merge request reports