Permission - Environment variables

[daeyeon]

Hello folks,

I recently made a PR related to environment variables permissions. I basically like the idea of explicitly knowing what resources an application is accessing when it runs.

There were some discussions about whether this permission itself is even necessary. And I started to have my own doubts. Since the environment variables is on the Security WG's permission model roadmap, I'd like to move the discussion here to continue with everyone's thoughts.

---

1. Needed?

This is for providing transparency about which environment variables are being accessed. It informs users by throwing an error when attempts to access disallowed variables occur. Additionally, it allows for setting up an allowlist in a consistent manner, regardless of platforms.

2. How to do?

The current proposal is to add variable names into an allowlist using the --allow-env flag as shown below. Any variables not included in the allowlist will be inaccessible through process.env.

# Limited access
node --experimental-permission

# Full access
node --experimental-permission --allow-env

# Single name
node --experimental-permission --allow-env=HOME 

# Multiple names
node --experimental-permission --allow-env=HOME,PORT

3. How do the others do?

A comparable example is deno, which has the same as this proposed one.

--allow-env=<VARIABLE_NAME> Allow environment access for things like getting and setting of environment variables. Since Deno 1.9, you can specify an optional, comma-separated list of environment variables to provide an allow-list of allowed environment variables.

---

Below are a summarized thoughts of the opposing perspectives discussed so far. Please let me know if there are any omissions or inaccuracies.

1. Needed?

Accessing to environment variables is not inherently dangerous. There is also a question as to why this should be prevented and informed. Regarding the allowlist, there is already a universal way to selectively pass environment variables to any program, which is much easier and better than the new flag provided.

2. How to do?

Since environment variables are just a list of names passed to a process, users can control it in the shell as shown below.

# linux bash
env -i HOME=$HOME PORT=$PORT node

# windows prompt
cmd /c "set HOME=%HOME% && set PORT=%PORT% && node"

Alternatively, we can rely on package manager to only selectively pass environment variables to child processes.

(daeyeon: I guess we could use cross-env to do this.)

{
  "scripts": {
    "dev": "cross-env HOME=$HOME PORT=$PORT node <program>"
  }
} 

3. How do the others do?

There is a concern about the code complexity. Deno is designed to have a built-in and useful permission system from the beginning, which is not the case with Node.js. Since the easy way to control it externally exists, we need to consider the worth of the complexity. Plus, catching up with questionable features shouldn't be our goal.

[RafaelGSS]

Hi @daeyeon!

As I posted in the PR:

Honestly, after thinking for some time, I don't see a security reason to block the process.env.* access. It doesn't seem like a feature someone would use either in production or in development and Node.js doesn't have a history of exploits using environment variables. And if we proceed with this restriction (--allow-env), it will mean there are hundreds of uncovered cases we'll need to deal with, and since this is a security feature, all those cases will be considered a vulnerability.

I feel we should stick all the future scopes to real threats, mostly from third-party packages. For instance, restricting access to net (tcp/udp to a specific port) seems something valuable for the project. In the Security WG scope, we'll work to clarify the Permission Model scope and see what are the boundaries for this feature -- for instance, it's very likely to have a big overhead if we check tcp/udp connections.

I was planning to bring up that discussion when we finish the current initiatives, but I'll mention it beforehand and keep an asynchronous discussion in the Permission Model roadmap issue.

[daeyeon]

@RafaelGSS Thanks for clarifying the scope. I feel I got a better understanding of the direction we should take from the security perspective. If there would be something I can contribute that aligns with the scope, I'd be happy to participate again.

[RafaelGSS]

So @daeyeon, we've discussed it a bit in the Security WG session and Michael seems to agree with the Environment Permission addition.

As a TL;DR my concern was replied:

I don't see a security reason to block the process.env.*

From @mhdawson: It seems to fall under the same rule as the file system. If you install a malicious package by mistake, it's essential to reduce the attack vector as much as you can. Therefore, selecting either if the binary has access to process.env or which environment variables would be available seems something useful. -- Feel free to edit this quote if I don't understand correctly.

Basically, by reading the pull request again, the concerns raised there were:

1. Complexity
2. Usability

Quoting @tniessen:

If denying access to a few specific variables is the most common scenario, and if those have to be specified when starting the Node.js process, why can't the user just unset those specific environment variables when starting the Node.js process?

3. Which attack vector does it remediate?

Unless the environment permission works as a boolean (enabled/disabled), I don't see why specifying a list of allowed environment variables would prevent an attack vector. For instance, --allow-env=AWS_CREDENTIALS would mean that a malicious package would be able to read it as well.

---

So, @mhdawson or anyone else from @nodejs/security-wg who wants to step in a share your point of view would be much appreciated. For now, I can agree only with denying access to the entire environment permission

pinging folks from the PR: @tniessen @ljharb @Qard @richardlau in case you have other thoughts/objections on this.

[mhdawson]

My thought is that environment variables are used to provide sensitive tokens, etc. to a running applicaiton through the environment. For example if an appliation read/printed all environment variables that might be concern even if it's not malicious code doing it. The ability to prevent access to the environment would help minimize errors that could expose sensitive information shared through the environment. I think the model of deny all and then possibly whitelist a small subset would best fit that scenario.

[richardlau]

FWIW I know of one requested use case regarding environment variables, nodejs/diagnostics#332 / nodejs/node-report#114 (this was the report feature before it was merged into the core repository). The permissions model proposal for environment variables only dealt with process.env and would not have covered that use case (report goes through libuv to enumerate the env variables). (Even assuming we ever supported that use case, it might not need to be via the permissions model.)

I think the big difference between, say, file permissions and environment variables is that there is a simple way of denying environment variables to Node.js processes -- unset them before running the process. That would side step all sorts of potential side channels in Node.js.

I also wonder if there will be a disconnect between limiting to process.env vs what end users might expect a permissions model for environment variables to be able to control. There are lots of places where environment variables control aspects of Node.js behaviour which are not going through process.env (e.g. NODE_OPTIONS, environment variables read by dependencies (OpenSSL, ICU, etc.)) which would not be covered.

[RafaelGSS]

I think the model of deny all and then possibly whitelist a small subset would best fit that scenario.

What would it prevent? For instance, if I need to have a whitelist for AWS_CREDENTIALS to use in my application, how the pm will prevent other modules to access it?

For now, I'm tempted to consider --allow-env as a boolean only. But, I'm not quite sure if that will prevent an attack vector. As said previously in this thread, a user can unset it before the app load any module.

any thoughts @nodejs/security ?

[Qard]

It's also worth thinking about if identifying presence of an env var can be attacked. For example, if you try to read a blocked env var and it throws that could be interpreted as confirmation that env var exists. Being able to read it is not necessarily required to get enough context about the environment the app is running in to determine other possible steps to take in an attack.

Should env just leave any blocked keys undefined as it does with any non-present value? Or perhaps it should also throw on non-present values?

There's also the object cloning situation to consider--it's relatively common to blindly clone process.env without caring what's in it for things like passing env minus some changed value into a child process. If accessing a property throws then a lot of apps could suddenly start breaking.

[ljharb]

You're 100% correct that a "blocked" env var should simply be absent, not throw, for the reasons stated.

[darcyclarke]

node --allow-env seems to be the same as node today - unless I'm misunderstanding this thread - but I don't actually think the intent is for a bare node to start throwing on any unconfigured env usage; so how do I do that? ie. if I want to broadly turn off all env usage, what does that look like? (ex. node --block-env or node --block-env --allow-env=HOME)

In terms of whether or not to throw, I don't see a problem with that. I'd find this feature far less useful if it doesn't throw though. The security concerns aren't really that valid given that if someone is able to monitor the status of the runtime execution then they've likely compromised your system at a higher domain (meaning you have other problems).

[marco-ippolito]

I think every time trying to read whatever env variable not whitelisted should throw, thus making impossible to know which one exist and which not

[daeyeon]

so how do I do that? ie. if I want to broadly turn off all env usage, what does that look like?

As of now, the flag should be behind the --experimental-permission flag, which restricts access to all available permissions. I modified the usage for clarity.

[RafaelGSS]

@darcyclarke When --experimental-permission is enabled, all the available permissions should be restricted by default, and permissions should be set by --allow-* flags.

[ljharb]

@marco-ippolito but then you can distinguish between "locked down" or "the env var simply isn't there", and exposing information to attackers isn't ideal.

[bmeck]

This kind of decision is why we had multiple reaction types in policies. CLI based config will be non-ideal for these complex configurations

[zekth]

I think the model of deny all and then possibly whitelist a small subset would best fit that scenario.

For now, I'm tempted to consider --allow-env as a boolean only. But, I'm not quite sure if that will prevent an attack vector. As said previously in this thread, a user can unset it before the app load any module.

If we're going that route with enabling a small subset we definitely need to support glob patterns which will complexify the whole implementation. eg:

node --allow-env=MY_APP_*,AWS_CREDENTIALS

You're 100% correct that a "blocked" env var should simply be absent, not throw, for the reasons stated.

Agree and disagree, it would be seamless for all the current tooling that relies on environment variables to check config for example; however that's also a thing we might want to enforce like Deno introduced in the initial design.

This case would be another story, like node --strict-mode which would throw when trying to access restricted env var for example.

[bmeck]

What is the behavior expectation here for things that do populate the process.env from disk like .env.local file reading? Setters being prevented seems somewhat reasonable if the intent is to absolutely prevent credentials from being in the process, which is different from preventing them from being pulled into the process from the environment. This pattern of reading a file to populate process.env is quite common in the wild. I once again think limiting to CLI makes configuration hard here as well since an application administrator may wish to prevent any library such as https://www.npmjs.com/package/dotenv from populating the fields while other application administrators are fine with them loading from disk. A good example of where they may wish to prevent loading from env but wish to load from disk is to ensure reproducibility across deployment environments.

[RafaelGSS]

First I'd like to understand the usability of this feature, then we could nail the tail to the implementation details, such as reading config from a file, throwing exceptions, and so on.

For now, nobody has shown this feature will be useful (from a security perspective), considering you will be able to achieve the same from a module that runs in the app startup (for instance, the dotenv can accept a list of allowed env var and throw/undefine the others).

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.