Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Commit

Permalink
Clobber a Link if it's in the way of a File
Browse files Browse the repository at this point in the history 
Fixes isaacs/node-tar#212
  • Loading branch information
@isaacs
isaacs committed May 15, 2019
1 parent 1e4527f commit 6a77d2f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/writer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ Writer.prototype._stat = function (current) {

// if it's a type change, then we need to clobber or error.
// if it's not a type change, then let the impl take care of it.
if (currentType !== self.type) {
if (currentType !== self.type || self.type === 'File' && current.nlink > 1) {
return rimraf(self._path, function (er) {
if (er) return self.error(er)
self._old = null
Expand Down

3 comments on commit 6a77d2f

@ret2libc
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix really solve the issue? Doesn't it make it just racy? (e.g. if at the time of check the file is a regular one and it is switched to an hardlink just before the create() function is called)

@mssalvatore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this patch is attempting to resolve a TOCTOU condition. The context of the original issue (https://hackerone.com/reports/344595) is that arbitrary files on the filesystem could be overwritten if a crafted tar archive were extracted.

Within that context, I don't believe this fix is has a race condition. It is not trying to prevent general TOCTOU issues. Rather, it's preventing the condition where you're in the process of iterating through the entries in a tar archive and you:

  1. Extract a hardlink named "LINK" that points to some arbitrary location (like /etc/passwd)
  2. Extract a regular file that also has the name "LINK"

Without this fix, this scenario would result in /etc/passwd being overwritten with the contents of the regular file "LINK".

All that being said, I am really not a javascript/node.js developer, so:

  1. Since node.js handles I/O asynchronously, are there concurrency concerns while looping through and extracting the entries of a tarball?
  2. Why would someone use fstream at all in this capacity when they could use the tar package instead? I'm curious as to why this fix was really necessary in the first place.

@ret2libc
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this patch is attempting to resolve a TOCTOU condition. The context of the original issue (https://hackerone.com/reports/344595) is that arbitrary files on the filesystem could be overwritten if a crafted tar archive were extracted.

True, though I was not able to trigger the issue while extracting a tar. Maybe I'm just missing something, but I ended up analyzing the issue in the context of a copy from one directory to another. Also, from reading various comments around this seemed more like a security fix to make npm-audit happy.

Please sign in to comment.