predictable tmpfile names may lead to potential security issue #3635

Closed
kapouer opened this Issue Jul 8, 2013 · 11 comments

5 participants

@kapouer

Forwarding this potential security issue about npm :

On 08/07/2013 14:23, Daniel Kahn Gillmor wrote:

On 07/08/2013 07:55 AM, Jérémy Lal wrote:
I am curious about how npm install mymodule could be a target for an attacker,
especially considering the temp directory is used only once (at (un)tar times).

if the tmpdir is predictably-named (e.g. it is /tmp/npm-$PID), then an
attacker could watch the process table for a process named "npm", and as
soon as it appears (say, as pid 13577, create a symlink at
/tmp/npm-13577 that points to, say, the home directory of the user npm,
which might have the effect of clobbering any similarly-named files.

This is a crude attack, but depending on the contents of the tarball it
could be pretty unfortunate (e.g. if the tarball contains a file named
secring.gpg, and the attacker points the symlink to the victim's
~/.gnupg ?).

I still do not understand if this is really a security issue.
IMO if a program on your system does that, the whole system is compromised,
you can't really be hardening any software against it.

what we're talking about is a classic symlink attack. I haven't tried
to verify it with npm myself, but using predictable tmpfile names in
world-writable directories is the usual gateway to a vulnerability here.

@luk-

Doesn't sound unreasonable but I'm not sure if this is something that will be changed / changed anytime soon.

@isaacs
npm member

First, yes, if you are running code on a machine that also has malicious users that can look in the process table, you're already pretty well fucked in many ways.

But, sure, defense in depth.

The ideal solution here, for many reasons beyond this issue, is to not use tmp files at all. There's no good reason for them, they're just a relic from workarounds that are no longer necessary. But, the change is a massive rewrite, that I haven't been able to get to yet.

A temporary immediate workaround is to set npm's tmp configuration to something other than /tmp.

A less immediate workaround is f4d3169.

@isaacs isaacs added a commit that closed this issue Jul 10, 2013
@isaacs isaacs Put 6 bytes of random in tmp folder name
Closes #3635
f4d3169
@isaacs isaacs closed this in f4d3169 Jul 10, 2013
@fgeek

Please use CVE-2013-4116 for this issue.

@luk-

@fgeek not sure if that's meant for us or if it's part of something automated, but we don't generally post updates/issues to other bug trackers :)

@isaacs
npm member

@kapouer @fgeek Does this fix satisfy the issue?

@codevi writes:

The tmp file can still be predicted:

find /tmp -name "nmp-pid-*"

Where pid is the value of the process id.

As tmp is world readable, a user can predict/find any random value mixed with static/known contents.

It seems like that will always be an issue, as long as npm uses tmp files at all. Short of using random names for every single file underneath the /tmp tree (which is a bit unreasonable) there'll always be a way that an attacker can repeatedly search for /tmp/*/*/package.

However, once the folder is created, it seems like less of an issue. After all, the problem is that an attacker can create the symlink after the npm process is started, but before it tries to create its tmp file, and thus trick npm into writing to the wrong place. The folder itself is user-owned and only user-writable, and the level underneath it is completely randomized, so I'm not convinced this is still an issue (or really, even ever was).

I'd like to see an actual exploit before adding any more complexity to this. The plan is to eventually do away with tmp file usage completely, so doing more work on it seems foolish.

@luk-

@fgeek ah neat, we don't really use that stuff, but feel free to update it with the rhel people or whatever you need to do if you're involved with them :)

@fgeek

@st-luke Already done. For example in Debian bug tracking system https://security-tracker.debian.org/tracker/CVE-2013-4116 all security vulnerabilities should have CVE (and even some security hardening issues). I can request those for you if you lack the time or something like that.

@luk-
@fgeek

@luk- It's not only that you need them for security advisories/release notes, but it is very important for users of your software. CVE makes it easier to coordinate security vulnerabilities. Now that this vulnerability has CVE identifier we can use it e.g. in Debian to discuss and address same vulnerability and for example share the patch with Ubuntu or other distros. I hope I explained it correctly :) I'm also happy to help if there is need for CVE identifiers.

@mhradilek

You could also verify content after extraction and if it fails then reject it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment