From d1ae0d29330395923cc561853267f1fd897c8bb4 Mon Sep 17 00:00:00 2001
From: Arik Hadas <arik.hadas1@gmail.com>
Date: Fri, 24 Sep 2021 22:13:58 +0300
Subject: [PATCH] fix #508: prevent ZipSlip

---
 .../com/mucommander/job/impl/UnpackJob.java   | 33 ++++++-------------
 .../src/main/resources/dictionary.properties  |  2 +-
 .../main/resources/dictionary_en.properties   |  1 +
 package/readme.txt                            |  1 +
 4 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/mucommander-core/src/main/java/com/mucommander/job/impl/UnpackJob.java b/mucommander-core/src/main/java/com/mucommander/job/impl/UnpackJob.java
index 105264f8c4..c665cdca11 100644
--- a/mucommander-core/src/main/java/com/mucommander/job/impl/UnpackJob.java
+++ b/mucommander-core/src/main/java/com/mucommander/job/impl/UnpackJob.java
@@ -33,7 +33,6 @@
 import com.mucommander.ui.dialog.file.ProgressDialog;
 import com.mucommander.ui.main.MainFrame;
 
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.FileSystems;
@@ -251,10 +250,18 @@ else if(entryPath.equals(selectedEntry.getPath())) {
                 // Create destination AbstractFile instance
                 AbstractFile destFile = destFolder.getChild(relDestPath);
 
+                // Check for ZipSlip (see https://snyk.io/research/zip-slip-vulnerability)
+                do {
+                    if (destFolder.isParentOf(destFile))
+                        break;
 
-                //Check for ZipSlip
-                if(!checkForZipSlip(destFile, destFolder))
+                    int ret = showErrorDialog(errorDialogTitle, Translator.get("unpack.entry_out_of_target_dir", destFile.getName()));
+                    // Retry loops
+                    if(ret==FileJobAction.RETRY)
+                        continue;
+                    // Cancel, skip or close dialog returns false
                     return false;
+                } while(true);
 
                 // Check if the file does not already exist in the destination
                 destFile = checkForCollision(entryFile, destFolder, destFile, false);
@@ -317,26 +324,6 @@ else if(entryPath.equals(selectedEntry.getPath())) {
         return false;
     }
 
-    /** This methods checks for a zipSlip attack.
-     * It is inspired by the mitigation shown in https://snyk.io/research/zip-slip-vulnerability.
-     * @param destFile destination file to unpack to.
-     * @param destFolder destionation folder where the file is put.
-     * @return false if file is outside the target unpack fodler, true if everything is fine.
-     **/
-    private boolean checkForZipSlip(AbstractFile destFile, AbstractFile destFolder) {
-
-        String canonicalDestinationDirPath = destFolder.getCanonicalPath();
-        if (!canonicalDestinationDirPath.endsWith(File.separator)) {
-            canonicalDestinationDirPath = canonicalDestinationDirPath + File.separator;
-        }
-        String canonicalDestinationFile = destFile.getCanonicalPath();
-        if (!canonicalDestinationFile.startsWith(canonicalDestinationDirPath)) {
-            showErrorDialog(errorDialogTitle, Translator.get("entry_outside_of_target_dir", destFile.getName()));
-            return false;
-        }
-        return true;
-    }
-
     // This job modifies the base destination folder and its subfolders
     @Override
     protected boolean hasFolderChanged(AbstractFile folder) {
diff --git a/mucommander-translator/src/main/resources/dictionary.properties b/mucommander-translator/src/main/resources/dictionary.properties
index afb5cf61c4..222a5ae756 100644
--- a/mucommander-translator/src/main/resources/dictionary.properties
+++ b/mucommander-translator/src/main/resources/dictionary.properties
@@ -765,6 +765,7 @@ move_dialog.cannot_move_to_itself = Cannot move files to a subfolder.
 pack.error_on_file = Error while compressing {0}
 pack_dialog.cannot_write = Unable to create file in destination folder.
 unpack.unable_to_open_zip = Unable to open zip file {0}.
+unpack.entry_out_of_target_dir = Entry is outside of the target dir: {0}
 image_viewer.previous_image = Previous image
 image_viewer.next_image = Next image
 mkdir_dialog.title = Make directory
@@ -787,4 +788,3 @@ delete.delete_link_only = Delete link
 delete.delete_linked_folder = Delete folder
 Unpack.label = Unpack files
 Pack.label = Pack files
-entry_outside_of_target_dir=Entry is outside of the target dir: {0}
\ No newline at end of file
diff --git a/mucommander-translator/src/main/resources/dictionary_en.properties b/mucommander-translator/src/main/resources/dictionary_en.properties
index 65b74a1c0b..f28867e5fe 100644
--- a/mucommander-translator/src/main/resources/dictionary_en.properties
+++ b/mucommander-translator/src/main/resources/dictionary_en.properties
@@ -763,6 +763,7 @@ move_dialog.cannot_move_to_itself = Cannot move files to a subfolder.
 pack.error_on_file = Error while compressing {0}
 pack_dialog.cannot_write = Unable to create file in destination folder.
 unpack.unable_to_open_zip = Unable to open zip file {0}.
+unpack.entry_out_of_target_dir = Entry is outside of the target dir: {0}
 image_viewer.previous_image = Previous image
 image_viewer.next_image = Next image
 mkdir_dialog.title = Make directory
diff --git a/package/readme.txt b/package/readme.txt
index c093552078..3cf4e64bda 100644
--- a/package/readme.txt
+++ b/package/readme.txt
@@ -43,6 +43,7 @@ Improvements:
 - Symbolic links are unpacked from tar archives.
 - The portable version stores preferences files in the installation directory.
 - It is now possible to change the modification date of multiple search results.
+- Prevention of ZipSlip (https://snyk.io/research/zip-slip-vulnerability)
 
 Localization:
 - Updated Russian translation