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

Pixeebot / Introduced protections against "zip slip" attacks #2020

Closed
wants to merge 1 commit into from
Closed

Pixeebot / Introduced protections against "zip slip" attacks #2020

wants to merge 1 commit into from

Conversation

zcarroll4
Copy link

@zcarroll4 zcarroll4 commented Sep 22, 2023

Description

This change updates all new instances of ZipInputStream to protect against malicious entries that attempt to escape their "file root" and overwrite other files on the running filesystem.

Normally, when you're using ZipInputStream it's because you're processing zip files. That code might look like this:

File file = new File(unzipTargetDirectory, zipEntry.getName()); // use file name from zip entry
InputStream is = zip.getInputStream(zipEntry); // get the contents of the zip entry
IOUtils.copy(is, new FileOutputStream(file)); // write the contents to the provided file name

This looks fine when it encounters a normal zip entry within a zip file, looking something like this pseudo-data:

path: data/names.txt
contents: Zeus\nHelen\nLeda...

However, there's nothing to prevent an attacker from sending an evil entry in the zip that looks more like this:

path: ../../../../../etc/passwd
contents: root::0:0:root:/:/bin/sh

Yes, in the above code, which looks like every piece of zip-processing code you can find on the Internet, attackers could overwrite any files to which the application has access. This rule replaces the standard ZipInputStream with a hardened subclass which prevents access to entry paths that attempt to traverse directories above the current directory (which no normal zip file should ever do.) Our changes end up looking something like this:

+ import io.github.pixee.security.ZipSecurity;
  ...
- var zip = new ZipInputStream(is, StandardCharsets.UTF_8);
+ var zip = ZipSecurity.createHardenedInputStream(is, StandardCharsets.UTF_8);

Powered by: pixeebot (codemod ID: pixee:java/harden-zip-entry-paths)

* Introduced protections against "zip slip"  attacks

* update gradle files, include pixee-java-security

---------

Co-authored-by: pixeebot[bot] <[email protected]>
Co-authored-by: Zach <[email protected]>
@jpstotz
Copy link
Collaborator

jpstotz commented Sep 22, 2023

@zcarroll4 Thanks for the PR. But are you sure Jadx is still vulnerable to such attacks? Because we already have a ZIP entry path traversal check in place.

Our implementation is just not integrated into ZipInputStream https://github.com/skylot/jadx/blob/master/jadx-core/src/main/java/jadx/api/plugins/utils/ZipSecurity.java

@skylot
Copy link
Owner

skylot commented Sep 22, 2023

@zcarroll4 as @jpstotz mention jadx already use similar zip security utils (with additional checks to prevent zip bomb attacks #980). Changes in this PR done in code which used only in jadx dev tools and not used in normal jadx operation. Anyway, to unify work with zip files, I will fix this case to use jadx utils in later commit.
As a result, I don't see any changes in this PR which can be used, so I will close it.

@skylot skylot closed this Sep 22, 2023
@skylot
Copy link
Owner

skylot commented Sep 22, 2023

Update:
Turns out code for work with zip files in ClsSet class was not needed at all, so I just remove it (4d87b44) 🤣

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.

3 participants