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

Add rs-async-zip Zip Traversal issue to DB #1141

Closed
wants to merge 1 commit into from
Closed

Add rs-async-zip Zip Traversal issue to DB #1141

wants to merge 1 commit into from

Conversation

snoopysecurity
Copy link
Contributor

A Zip Traversal issue exists in rs-async-zip. Makes sense to add it. Examples of other libraries that have fixed it can be seen here: https://github.com/snyk/zip-slip-vulnerability#affected-libraries.

To reproduce this issue, you can follow the steps here: https://gist.github.com/snoopysecurity/007503097536b557bc22a7ef24f4d11d

This issue was privately disclosed to the maintainer. He has stated that he has made conscious decision not to mitigate any sort of traversal attacks within the library itself. He has however added a notice to to the example code like here https://github.com/Majored/rs-async-zip/blob/main/examples/fs_parallel_extract.rs#L10 but the library itself does not do any sanitization.

@Shnatsel
Copy link
Member

Thank you for reporting this!

This is a tricky case with upstream being unwilling to fix the issue. We have carried similar advisories in the past, but that caused some issues, see #1092. We wanted to get the moderation team to take a look, but the moderation team has stepped down at an inopportune moment...

So we don't really have a good story around issues that upstream is unwilling to fix. However, I don't think carrying the advisory alone is going to solve anything; we need to talk to the maintainer and either get more prominent warnings put up on the repo (in README and on docs.rs), or point to other projects that have mitigated this vulnerability (e.g. tar, see RUSTSEC-2018-0002 and RUSTSEC-2021-0080) and ask to perform similar checks, using those crates as precedent.

@Majored
Copy link

Majored commented Feb 20, 2022

Heyo. Maintainer here.

I'm failing to see why an advisory notice would be needed here. Directory traversal is a very clear and present issue, I don't disagree, but my library doesn't provide any functions/helpers/types that directly target extracting a ZIP to the disk (and I don't plan on ever adding such either).

Both of the examples provided by @Shnatsel target specific functions in "tar-rs" that extract an entry to the disk.

My crate is intended to be a raw reading/writing crate. You could entirely work with ZIPs in-memory and directory traversal isn't ever an issue. Sure, if you incorrectly use the library and specifically target extraction to the disk, you could be vulnerable to directory traversal - but that's up to the library implementer.

No element of the crate is vulnerable to directory traversal - only the two examples provided in the source GitHub repository, which as already noted, has had clear notices added to them.

@Majored
Copy link

Majored commented Feb 20, 2022

To further my point, Java's ZipFile/ZipInputStream classes don't contain any mitigation either - do we now consider these two classes, which have been around since 2001, vulnerable to directory traversal?

Clearly not, because neither class targets extraction to disk - they're raw readers, just like my crate.

I appreciate @snoopysecurity's disclosure regarding the repository examples, but to label/push the entire crate as vulnerable is objectively incorrect.

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 this pull request may close these issues.

None yet

3 participants