Module search security 8830

[iankronquist]

It seems like some core contributors aren't the biggest fans of this, but this is a concern which should not be completely ignored.
I think that this is reasonable to protect against on multi-user Windows systems.
I would also update the accompanying docs.
I do not like what I'm doing with environment variables in the test, so if there is a better way to mock $HOME or the process module, please let me know.

[feross]

This was recently discussed on joyent/node. Have you seen @chrisdickinson's reply?

nodejs/node-v0.x-archive#8830 (comment)

[iankronquist]

Yes I have, and I will not be surprised if this is rejected. If so, it would be great if I could get some guidance as to what the community needs. Pointers to features which should actually be implemented, or what real bugs should be fixed would be helpful, though I know this is hard with the massive backlog.

[algesten]

-1

It adds a weird random unexpected behaviour for a very specific situation. For our servers it wouldn't add any level of security since each project is deployed under /usr/local and run by a user that still has a $HOME under /home.

[iankronquist]

@algesten it seems like @ivan has a different opinion specific to multi-user windows systems (of which there are many). This is different than your setup.
nodejs/node-v0.x-archive#8830 (comment)

[bnoordhuis]

Pointers to features which should actually be implemented, or what real bugs should be fixed would be helpful, though I know this is hard with the massive backlog.

I agree we need to do better here, although I'm not sure how. I'll bring it up at the TC meeting tonight.

[iankronquist]

@bnoordhuis Thanks! I'd really like to help, but it's hard swimming through a see of bugs, only to have this happen.

[isaacs]

Is this part necessary? Each test is run in a separate process, so the env gets reset.

[iankronquist]

It may not be. That would simplify the test.

[rvagg]

TC discussed this today, see #178 for meeting minutes and a link to the video. The agreement was to move discussion back here.

Unofficially, the tone of the discussion leads me to believe that this PR is unlikely to land without more compelling arguments and perhaps examples. It'd be worth reviewing the meeting video to get the nuance of the discussion around this issue though.

[isaacs]

I think that the only real hazard here is C:\node_modules on multi-user windows systems, since Windows allows dir-creation in C:\ without admin rights.

User creation requires root access (in which case you're hosed already) and creating dirs in C:\Users\ also requires admin access (again, hosed).

Stopping at the home dir is an arbitrary and surprising behavior. An alternate proposal, to stop lookup at the cwd, is just as arbitrary and will break many existing use-cases without a clear benefit. For example, if you have a script that does require("grunt") in package.json, and grunt is a top-level dep that is deduped, then it'll need to search up past the cwd of the child process to find it.

I'd be more comfortable simply saying that C:\node_modules isn't searched unless the module path starts with C:\node_modules. However, I'd first want to verify that on multi-user systems, Windows still allows dir creation by non-admins in C:\. If that (horrendous!) default has changed, then this change unnecessary, and should not be added.

I am not trying to be excessively pedantic about keeping the module semantics frozen, and I apologize for the apparent resistance. Any change in this area has the potential to upset module interoperability, which affects all of our users in surprising ways.

tl;dr

Here's what I'm ok with.

If C:\ dir creation is allowed on multi-user systems (someone check on this), then:

1. If the module doing the requiring is not itself under C:\node_modules, then do not check C:\node_modules.
2. Add a paragraph to doc/api/modules.markdown explaining why this limitation exists.

[ivan]

However, I'd first want to verify that on multi-user systems, Windows still allows dir creation by non-admins in C:\

Yeah, as nodejs/node-v0.x-archive#8830 (comment) describes, Vista, 7, and 8 allow anyone in Authenticated Users to create C:\node_modules. Authenticated users are anyone who logs in, not necessarily admins.

[ivan]

User creation requires root access (in which case you're hosed already)

This is not clear. Imagine an automated process (e.g. signup on a shared hosting provider) that will allow you to name yourself "node_modules", or a human that can be socially engineered into letting you have a "node_modules" account for a made-up reason like "I need this for my node.js code to work". The process or the admin isn't the bad guy trying to hose you, so you were not hosed until node search behavior decided to run the attacker's code.

[isaacs]

Yeah, as nodejs/node-v0.x-archive#8830 (comment) describes, Vista, 7, and 8 allow anyone in Authenticated Users to create C:\node_modules. Authenticated users are anyone who logs in, not necessarily admins.

Fair enough.

Another question: is anyone using Node in environments like this? Isn't shared hosting sort of a thing of the past? VMs and container systems are easier and cheaper, and more secure for countless other reasons.

[ivan]

Not sure about "shared hosting" in particular, but node ships with end-user-facing desktop software (e.g. LightTable, keybase.io, Atom, and Photoshop CC 2014), so it seems very plausible that someone would run node in an environment with arbitrarily-named users in /home or C:\Users.

[algesten]

@ivan as isaac pointed out in the TC meeting. if you have a shared hosting provider with no checks on what user names people can take, you have bigger problems than this. same goes for the social engineering scenario. there are a whole slew of bad names that any sane sysadmin will disallow normal users i.e.daemon, sync, nobody, postgres, newrelic etc etc

as i see it the only "solution" to this problem (if it is a problem. i'm not convinced) is to disallow climbing above cwd, with the breakage of many very useful usages. could be a compile switch --without-above-cwd-resolution so people with this very specific shared system concern can make their own gimped node. (but then any savy user would just compile their own, and we're back to square 1)

userland is dangerous. don't trust it.

[isaacs]

A strategy that @piscisaureus suggested in the last TC meeting, which I haven't completely thought through yet, is to only search up to the left-most node_modules path entry when inside a requiring module is inside of a node_modules folder.

So, if a module at /a/b/c/node_modules/d/node_modules/e/foo.js were to do require('x'), it would search:

/a/b/c/node_modules/d/node_modules/e/node_modules/x
/a/b/c/node_modules/d/node_modules/x
/a/b/c/node_modules/x  # highest level node_modules, stop here.

Instead of:

/a/b/c/node_modules/d/node_modules/e/node_modules/x
/a/b/c/node_modules/d/node_modules/x
/a/b/c/node_modules/x
/a/b/node_modules/x
/a/node_modules/x
/node_modules/x

This seems like it'd work fine for most npm use cases, and would prevent the possibility of exploits from C:\node_modules. As a bonus, it'd be a tiny bit faster startup time.

If we do implement that strategy, then I'd suggest that it should be behind a feature flag for at least a major version cycle so we can vet it very carefully.

[isaacs]

Also, of course, that strategy would not help in cases where the requiring module isn't in a node_modules folder. But, since most npm-installed bins are actually symlinks into node_modules, this is still pretty good protection. The hazard would be if you're in /home/me/projects/foo/foo.js, then it'll still walk all the way up the tree.

[rvagg]

Closing this because it's become stale. It can be reopened if necessary but it seems like a new PR for discussion focusing on @isaacs' last suggestion might be in order for those concerned about this issue (fwiw I'd support that change but it'd break some people's existing workflows--e.g. I know of people who put all of their dev stuff inside a folder called node_modules just to avoid npm link hassles).