Handle zip traversal vulnerability

webberig commented 3 years ago

Fixes #91 by checking the file path as described in this article

google-cla[bot] commented 3 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

webberig commented 3 years ago

@googlebot I signed it!

nicolaswww commented 3 years ago

Great job! I have created a fork in the meantime :) (some owner please merge it)

srgpqt commented 3 years ago

This fix sometimes fails, presumably when outputDirectory is not itself canonical. Had to revert it in our project.

ZumelzuR commented 3 years ago

can somebody merge this?????

DavidTalevski commented 2 years ago

This fix sometimes fails, presumably when outputDirectory is not itself canonical. Had to revert it in our project.

When the outputDirectory is a relative path you can convert it to a canonical path and still apply the same check.

  File file = new File(outputDirectory + compressedName);
  String canonicalDestinationPath = (new File(outputDirectory)).getCanonicalPath();
  String canonicalPath = file.getCanonicalPath();
  if (!canonicalPath.startsWith(canonicalDestinationPath)) {
      String errorMessage = "Zip traversal security error";
      callbackContext.error(errorMessage);
      Log.e(LOG_TAG, errorMessage);
      return;
  }

lczw commented 2 years ago

Should this really be approved, if it fails when the outputDirectory is not canonical? The code snippet by @DavidTalevski should be the way to go, or is there some weird behavior that occurs with it?

DavidTalevski commented 2 years ago

@lczw I've tested the solution for both relative and full paths and it works for both. So far I have not encountered any weird behaviors.

lukaszkurowskilbn commented 2 years ago

It's ok. ;)

czw., 16 gru 2021, 10:52 użytkownik David Talevski @.***> napisał:

@lczw https://github.com/lczw I've tested the solution for both relative and full paths and it works for both. So far I have not encountered any weird behaviors.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MobileChromeApps/cordova-plugin-zip/pull/92#issuecomment-995610886, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCYCNLU3DNSNGZW53JLDFLURGZFBANCNFSM4WYKYZKA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

lczw commented 2 years ago

I see. I have encountered a problem where there was a mismatch which is fixed by getting the canonical path of the output directory.

However, if it works on your end then it's probably something that should be fixed in my project instead of the plugin ^^

DavidTalevski commented 2 years ago

I see. I have encountered a problem where there was a mismatch which is fixed by getting the canonical path of the output directory.

However, if it works on your end then it's probably something that should be fixed in my project instead of the plugin ^^

I was describing the getCanonicalPath solution. I also had an issue with the PR code with relative paths which got fixed when turning them to canonical, so the problem is most likely not on your end.

lczw commented 2 years ago

I misunderstood your comment, sorry. Your code works fine for me as well, which is why I though it's a good idea to use it instead of the PR's code

aymanar commented 2 years ago

So is the final decision that this will not be merged?

jcperuffo commented 2 years ago

Hi devs,

I've manually applied this fix into my code today and would like to share a note about the problem. The fix was applied to an Android APP (Ionic v3 / Angular) that uses the cordova-plugin-zip lib. After apply the fix and test the APP, I noticed that the zip content that I had into my APP was not being unzipped, and while checking logs with adb logcat, I saw the message "Zip traversal security error" from the fix applied.

After debug the problem, I saw that, in my APP, the output directory where the content was being extracted is the user directory inside the Android filesystem. Compare the directories below:

Output from "file.getCanonicalPath()": /data/data/br.com.someapplication.sub/files/...

My APP output directory (where the zip content would be extract to): /data/user/0/br.com.someapplication.sub/files/...

Because of this, the code if (!canonicalPath.startsWith(outputDirectory)... fails and my zip files are not unzipped.

So, to be able to use the fix #92 and attend my scenario, I've added one more verification, as below:

...
File file = new File(outputDirectory + compressedName);
String canonicalPath = file.getCanonicalPath();
String absolutePath = file.getAbsolutePath();
if (!canonicalPath.startsWith(outputDirectory) && !absolutePath.startsWith(outputDirectory)) {
    String errorMessage = "Zip traversal security error";
    ...

Hope this can help someone.

bikubi commented 2 years ago

@jcperuffo we ran into the same problem & your solution worked. thanks!
I made a fork: https://github.com/bikubi/cordova-plugin-zip/ or see commit above.

SeroK commented 2 years ago

This fix sometimes fails, presumably when outputDirectory is not itself canonical. Had to revert it in our project.

When the outputDirectory is a relative path you can convert it to a canonical path and still apply the same check.
  File file = new File(outputDirectory + compressedName);
  String canonicalDestinationPath = (new File(outputDirectory)).getCanonicalPath();
  String canonicalPath = file.getCanonicalPath();
  if (!canonicalPath.startsWith(canonicalDestinationPath)) {
      String errorMessage = "Zip traversal security error";
      callbackContext.error(errorMessage);
      Log.e(LOG_TAG, errorMessage);
      return;
  }

I think this is the fix we want.

MobileChromeApps / cordova-plugin-zip