Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[security] Insecure usage of temporary files. #81

Closed
skx opened this issue Jun 16, 2016 · 8 comments · Fixed by #86
Closed

[security] Insecure usage of temporary files. #81

skx opened this issue Jun 16, 2016 · 8 comments · Fixed by #86

Comments

@skx
Copy link

skx commented Jun 16, 2016

The current code makes use of predictable filenames, in a way that causes a security issue.

I reported this to Debian last year:

It was recently highlighted by the nodesecurity people (six months later!):

Suggested fix:

  • Avoid using predictable filenames in world-writable directories.
    • Using ~/.app.pid would be better than /tmp for example.
@chriso
Copy link
Contributor

chriso commented Jun 16, 2016

The package node-cli insecurely uses the lock_file and log_file. Both of these are temporary, but it allows the starting user to overwrite any file they have access to.

I'm not entirely sure what the attack vector is here? If the "starting user" is running a command line node.js app, they can presumably already "overwrite any file they have access to" with <insert_coreutil>.

How would using ~ instead of /tmp mitigate this?

@skx
Copy link
Author

skx commented Jun 16, 2016

Consider a shared host with two logins:

  • evil creates a symlink from /tmp/app.log pointing to /home/user/.bashrc.
  • user runs an application that will create a predictable file in /tmp
    • Because that symlink exists it will be followed, with the net result that their .bashrc file is trashed.

Because both users can write to /tmp there is the potential for damage.

If, instead user only created ~/.app.log and ~/.app.pid then the evil user would not be able to write to it, to trigger the truncation/overwrite.

@chriso
Copy link
Contributor

chriso commented Jun 16, 2016

That makes sense, thanks.

@deadratfink
Copy link

Hi, thx for your nice Tool, but is there any Chance that this will be fixed soon? This marks other projects which use your outstanding tool as insecure on David batches, e.g. see https://david-dm.org/deadratfink/jy-transform/master.

Best, Jens

@XhmikosR
Copy link

XhmikosR commented Aug 9, 2016

@chriso @skx: wouldn't using https://github.com/sindresorhus/tempfile be enough along with process.env.TEMP || process.env.TMP?

@jugglinmike
Copy link
Contributor

I'm proposing we resolve this by removing the functionality altogether. A little unorthodox, but you can see gh-86 for my rationale.

@chriso
Copy link
Contributor

chriso commented Aug 16, 2016

This is resolved in 1.0.0

@jugglinmike
Copy link
Contributor

Thanks, @chriso!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants