-
Notifications
You must be signed in to change notification settings - Fork 78
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
Comments
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 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.)
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. |
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.
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. |
The validation function would block both leading |
Seems like there are nuances to symlink support that I think @thejoshwolfe is trying to make:
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. |
Hi! I believe the opened issue relates to |
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 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 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 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 ❤️ |
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
Steps To Reproduce:
Create a zip containing a file with a symlink and decompress using yauzl.
Prepare the malicious zip:
Reproduce the bug: I am using 'decompress' here, but the bug is in the underlying dependency - yauzl
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).
The text was updated successfully, but these errors were encountered: