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

Detect unsafe symlinks to prevent arbitrary file read/write during archive extraction #94

Open
ikkisoft opened this issue Nov 2, 2018 · 7 comments

Comments

@ikkisoft
Copy link

ikkisoft commented Nov 2, 2018

When decompression uses an attacker-controlled zip, it is possible to create a malicious archive containing symlinks which leads to the file decompression outside the original filesystem location. This can be abused to read/write files in arbitrary location.

While the library checks for path traversal attacks (see

yauzl/index.js

Line 607 in 6a9e652

function validateFileName(fileName) {
) symlink are not disabled (and there's not even an option to disable it).

Steps To Reproduce:

Create a zip containing a file with a symlink and decompress using yauzl.

Prepare the malicious zip:

ln -s / root
zip -v --symlinks boom1.zip root
zip -v --symlinks boom2.zip ./root/tmp/boom.txt

Reproduce the bug: I am using 'decompress' here, but the bug is in the underlying dependency - yauzl

const decompress = require('decompress');

decompress('boom1.zip', 'dist').then(files => {
    console.log('done zip1!');
        decompress('boom2.zip', 'dist').then(files => {
            console.log('done zip2!');
    });
});

IMHO, yauzl should have symlink disabled by default, and have an option to turn it on. This is how most common zip libraries deal with the problem (ops, feature).

@thejoshwolfe
Copy link
Owner

thejoshwolfe commented Nov 5, 2018

I'm all for better support for symlinks. yauzl is currently oblivious to when an entry is a symlink.

I've gotten reports that symlinks are encoded in divergent and nonstandard ways across different zip implementations. It's going to take a bit of effort to fully understand how symlinks are supposed to work in zipfiles.

The behavior I propose would be a relatively naive check on each symlink value to make sure it doesn't escape the archive with .. segments or an absolute path. If an archive contains a symlink to another item in the archive, then that doesn't seem like a security concern, and that's likely going to be the most common usecase for zipped symlinks.

The validation function would take as inputs the filepath in the zipfile entry (where the symlink would be created) and the value of the symlink (where it would point to). The validation function would not look at the actual file system or other entries in the zipfile. (The original value would be emitted to yauzl clients, not the canonicalized value described below.)

  • canonicalize the value: eliminate all . segments.
  • the number of leading .. segments must be less than the number of segments in the entry filepath.

If a symlink fails validation, an error would be emitted similar to when an entry's filepath is an absolute path, but you can disable this particular validation with an option when you create the Zipfile object, since this is not technically a violation of the spec, and some archives probably want to do this. The validation would be enabled by default, otherwise it's pretty worthless.

This is going to break some "correct" usecases, which suggests that this should be a major version bump, but it's also going to break malicious usecases, which suggests a minor version bump. I think this should definitely be a minor version bump to increase adoption.

@ikkisoft
Copy link
Author

ikkisoft commented Nov 6, 2018

I've successfully used this library to abuse an app without having to do path traversal, so I would stay away from trying to sanitize the symlinks.

e.g.

//On Mac, this will look for a NFS share with that file. Similar things in Win with UNC paths
zip -v --symlinks malicious.zip /net/192.168.1.1/tmp/foobar

I would suggest to simply create an option to disable symlinks. In a minor release, it could be 'enabled' by default not to break the current behaviour. Additionally, you can add a clear statement on the README in bold. During the next major version bump, disable symlinks by default.

@thejoshwolfe
Copy link
Owner

The validation function would block both leading .. segments and absolute paths, which are either a leading separator (which will reject your example) or /[A-Za-z]:[\/\\]/. Absolute paths are already checked and blocked for entry file paths; this would use the same logic.

@lirantal
Copy link

lirantal commented Nov 8, 2018

Seems like there are nuances to symlink support that I think @thejoshwolfe is trying to make:

  1. There is general support for symlinks - i.e: enable or disable them completely
  2. Support for symlinks for contents inside the archive

I see @ikkisoft's concern though that a blacklist might be difficult to come up with and later in the future it might be vulnerable to new input that we hadn't though of right now.

@snoopysecurity
Copy link

Hi! I believe the opened issue relates to decompress and was fixed a while back https://github.com/kevva/decompress/issues/71. I don't believe yauzl itself is vulnerable. I tried to reproduce using the above proof of concept and with the example usage shown within https://github.com/thejoshwolfe/yauzl#usage and wasn't able to.

@thejoshwolfe
Copy link
Owner

thejoshwolfe commented Mar 7, 2024

Sorry for the delayed response everyone.

Thanks @lirantal for the clarifications. I would like to emphasize a few points if any security minded people come to this thread and haven't read the (admittedly very long) readme for this project: yauzl is a low level library that never writes to the file system. Yauzl is not a zip file extraction tool, but simply a zip file reading library that other tools can use to perform extraction. I would like to further emphasize: symlinks are not supported by the zip file specification, making all symlinks in zip files non standard. This means that yauzl thusfar considers symlinks in zipfiles to be regular files, and clients would need to check the externalFileAttributes to determine on their own whether a file is a symlink.

Regarding the proposal to "disable symlinks", I'm not even sure what that would mean in this context. Does that mean hiding symlink entries from the listing of entries? Or giving an error from openReadStream()? Because yauzl treats symlinks as regular files, I don't see how it makes sense to "disable" them at this level. The concept is more meaningful in an extraction context, like the https://github.com/kevva/decompress project @snoopysecurity linked above, where you could skip extracting the symlink and avoid creating it on disk.

I do think it would be a nice enhancement to security to have an additional check in yauzl that raises an error when a zip file contains suspicious contents. However, this is far from a "vulnerability" in yauzl, because as stated above the real problem happens at extraction onto the file system, which is beyond the scope of this library.

@thejoshwolfe thejoshwolfe changed the title Arbitrary file read/write during archive extraction via symlink Detect unsafe symlinks to prevent arbitrary file read/write during archive extraction Mar 9, 2024
@lirantal
Copy link

lirantal commented Jul 5, 2024

@thejoshwolfe sounds good to me and thanks for circling back on this. I'll share with the security team at Snyk too in-case they'd want to continue the conversation. Appreciate your input and perspective ❤️

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

No branches or pull requests

4 participants