security,unix: audit use of process.env in lib/ for setuid binary

[bnoordhuis]

Functions like os.tmpdir() and Module._initPaths() use file paths from environment variables.

This is unsafe when the node binary has the setuid bit set - i.e., when it runs with the privileges of a different user (usually root) than the user executing it - because it can be used to read or write files that otherwise wouldn't be accessible.

On the C++ side we have secure_getenv() which checks that the real uid and gid match the effective uid and gid before accessing an environment variable. Perhaps we need something similar for JS land.

Caveat emptor: our implementation of secure_getenv() does not take Linux process capabilities into consideration but neither does glibc's, as far as I can tell. - edit 20171221: it does now and it's been renamed to SafeGetenv().

[evanlucas]

@bnoordhuis are you thinking that we should disable reading all environment variables or certain ones?

[bnoordhuis]

Good question. Variables like NODE_DISABLE_COLORS are probably safe but I wouldn't oppose a blanket ban.

[Trott]

@bnoordhuis @evanlucas Should this remain open? If so, should we add a help wanted and maybe even a mentor available label?

[j0t3x]

I would like to take this as my second contribution, is this possible?. I'm currently checking os and module code. I'll come back as soon as I'm more comfortable with that.

[bnoordhuis]

@j0t3x Let me know what you find or if you have questions.

[j0t3x]

@bnoordhuis between vacations and family I was unable to start the work earlier. So sorry for the delay.

Please let me know if I'm understanding this correctly.

Here goes the problem: os.tmpdir() and Module._initPaths() lack the functionality of checking for the match between real and effective uid&gid.

Possible solution: build a function to check for the match, but where should this function go, utils?

About the env variables, in which cases is it necessary to disable all/some?

Thanks to all in advance.

[bnoordhuis]

One solution is to add a method to process.binding('util') that calls SafeGetenv(). The file to look for is src/node_util.cc.

[j0t3x]

Hi @bnoordhuis,

Coming back after some research time.
I'm sorry again for the delay, my job is dragging me xD but ill get there :)

Here are some questions:
Up in the comment thread @evanlucas mentioned something about banning env vars. This will only happen when the setuid bit is set, right? which ones?

For the method i need to understand a little bit more how exactly will be used: For example, Should i throw an error if the env var is banned?

Thanks in advance.

[j0t3x]

@bnoordhuis I already have it implemented in c++, to make the js part, I need your help with this questions :D

Thanks in advance.

[bnoordhuis]

Missed your ping, sorry.

banning env vars. This will only happen when the setuid bit is set, right? which ones?

That's a policy question that we haven't really decided on. Could be judged on a case-by-case basis or simply by issuing a blanket ban.

Since you're working on it, you get to make the call. Maybe someone will flag it during review but we'll cross that bridge when we get to it.

Should i throw an error if the env var is banned?

No, just ignore it as if it isn't set. (edit: that's how SafeGetenv() already works, by the way.)

[j0t3x]

@bnoordhuis

C++ SafeGetEnv() already does a blanket ban right? So if I use it, that should do the job.
If there's a strong case to not go for a blanket ban I'm all ears. For now, I will proceed this way.
Let me know if I'm making a mistake about it.

Thanks!

[j0t3x]

Hi @bnoordhuis ,

After just exposing to jsland the already existent c++ SafeGetenv() which access the system env vars directly. I'm getting fails in 4 tests:

• parallel/test-os

• parallel/test-module-loading-globalpaths

• parallel/test-module-globalpaths-nodepath

• parallel/test-require-dot

here are some comments & questions about it:

Apparently, and this is something I need to understand, there are some env vars not accessed through SafeGetenv() that may be created in jsland, like NODE_PATH & USERPROFILE. I can't seem to find where all env vars are declared. Can you help me understand how env vars are declared, distributed between jsland & c++?

For the errors in tests, my main problem is that instead of using system declared env vars for the test cases, user constructed env vars are used. I need to further understand why is this the case because if I go directly to system declared env vars in os.tmpdir() and Module._initPaths(), tests mentioned above will fail. Should I remove the test code related to both functions?

I found one annotation by @addaleax in test-tls-env-bad-extra-ca:

TODO(addaleax): Make SafeGetenv work like process.env encoding-wise

is this related to this issue? what does this mean?

Thanks in advance for your great help.

[bnoordhuis]

process.env is a magic accessor object. Look for EnvGetter() and friends in src/node.cc.

That said, adding a suid check to those functions is probably too slow; people already complain about the overhead in applications that check process.env.NODE_ENV frequently.

[addaleax]

I found one annotation by @addaleax in test-tls-env-bad-extra-ca:

TODO(addaleax): Make SafeGetenv work like process.env encoding-wise

is this related to this issue? what does this mean?

No, it’s completely unrelated. What it means is that on Windows, using getenv() is not a great idea because the returned value is a single-byte encoding, which means that not all characters can be represented in the result.

EnvGetter() in node.cc has a proper solution for that (and I think @refack was working on something using libuv as a platform abstraction a while ago?).

[j0t3x]

First of all, thanks to both of you for your help. I'm ready for the pr now.
There's just one little weird thing pending. I'm getting this test failed but has no relation to my branch changes. @bnoordhuis
Any clues to whats happening?

=== release test-inspector-break-when-eval ===
Path: sequential/test-inspector-break-when-eval
[test] Connecting to a child Node process
[test] Testing /json/list
[err] Debugger listening on ws://127.0.0.1:64630/deb4b9ce-e4bc-4e72-b650-729603fd9c45
[err] For help see https://nodejs.org/en/docs/inspector
[err]
[err] Debugger attached.
[err]
[test] Setting up a debugger
[test] Breaking in the code
[out] Ready!
[out]
Timed out waiting for matching notification (break on /Users/tex/Documents/repos/node/test/fixtures/inspector-global-function.js:9))
1
Command: out/Release/node /Users/tex/Documents/repos/node/test/sequential/test-inspector-break-when-eval.js

Thanks again for the great and quick help.

[bnoordhuis]

Can you check make test with the master branch? If the test fails there, you know it's not caused by your changes.

[j0t3x]

@bnoordhuis
I get no error when using master branch so should be my bad, but I cant get what is happening in test-inspector-break-when-eval fail message.

[bnoordhuis]

@j0t3x Can you back out any changes you made to process.env and see if that makes a difference? Maybe PR your code so people can look at it.

[j0t3x]

@bnoordhuis there's no error in tests now, I'm going to start the pr. Thanks for the help.

[bnoordhuis]

To really close this out we'll also need to devise something for the getenv/setenv/etc. calls in our dependencies. We could perhaps exploit weak symbols or build our deps with -Dgetenv=node_getenv, etc.