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.