src: support for reading NODE_EXTRA_CA_CERTS from env file

[xsbchen]

Fixes: #51426

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[joyeecheung]

It doesn't seem right to override this with the dot env files without checking privileges which was what SafeGetEnv was for. Maybe @bnoordhuis knows better whether this violates what it tried to guard against.

[anonrig]

We don't need this function anymore. Ref: AssignNodeOptionsIfAvailable

[xsbchen]

got it, fixed

[anonrig]

Suggested change

	void Dotenv::AssignValueIfAvailable(const char* key, std::string* value) {
	void Dotenv::AssignValueIfAvailable(const std::string& key, std::string* value) {

[xsbchen]

fixed

[xsbchen]

It doesn't seem right to override this with the dot env files without checking privileges which was what SafeGetEnv was for. Maybe @bnoordhuis knows better whether this violates what it tried to guard against.

Thanks @joyeecheung, I'm not sure whether this change is reliable. @bnoordhuis If you are free, please help to review it.

Update: I check the comment on SafeGetEnv, it says Look up the environment variable and allow the lookup if the current process only has the capability CAP_NET_BIND_SERVICE set. If the current process does not have any capabilities set and the process is running as setuid root then lookup will not be allowed.. It seems that only want we need to get environment variables from the system (by uv_os_getenv) need to consider this capability.

[xsbchen]

any update?

[joyeecheung]

I think we need someone like @bnoorhuis or someone from @nodejs/security to review it and confirm that this is not introducing a attack vector

[lirantal]

Just briefly looked at it so I hope I don't miss anything important. Here's my observation:

• If someone already controls the --env-file, they could just as well use other command-line flags that are more problematic like loaders, etc.
• However, what if someone doesn't control --env-file but they do control the actual environment config file (the .env)? Like for example, they could exploit a path traversal or arbitrary file writes to change the contents of .env with their own.

[joyeecheung]

I think the original intent of SafeGetEnv is to guard against e.g. improper permission escalation when the binary gets the setuid bit set. Not sure what the threat model is. It might also make sense to completely ignore the .env file when the binary gets modified like that.

[xsbchen]

so need to do the same check like SafeGetEnv before getting value from .env file right? @joyeecheung @lirantal

[joyeecheung]

That would be what I'd do - just ignoring the .env too when the permission check fails - but I don't think I am familiar enough with the original threat model to make a call.

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.