[ofbiz-framework] branch trunk updated: Fixed: Prevent Zip Slip vulnerability (OFBIZ-12056) While working with FileUtil::unzipFileToFolder I noticed that it's vulnerable to Zip slip vulnerability: https://snyk.io/research/zip-slip-vulnerability.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[ofbiz-framework] branch trunk updated: Fixed: Prevent Zip Slip vulnerability (OFBIZ-12056) While working with FileUtil::unzipFileToFolder I noticed that it's vulnerable to Zip slip vulnerability: https://snyk.io/research/zip-slip-vulnerability.

jleroux@apache.org
This is an automated email from the ASF dual-hosted git repository.

jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/trunk by this push:
     new e136cb1  Fixed: Prevent Zip Slip vulnerability (OFBIZ-12056) While working with FileUtil::unzipFileToFolder I noticed that it's vulnerable to Zip slip vulnerability: https://snyk.io/research/zip-slip-vulnerability.
e136cb1 is described below

commit e136cb1d9885fc6e0910637542308a9b7c10eb9f
Author: Jacques Le Roux <[hidden email]>
AuthorDate: Sat Nov 14 11:39:10 2020 +0100

    Fixed: Prevent Zip Slip vulnerability (OFBIZ-12056)
    While working with FileUtil::unzipFileToFolder I noticed that it's vulnerable to
    Zip slip vulnerability: https://snyk.io/research/zip-slip-vulnerability.
   
    Fortunately OOTB code does not use FileUtil::unzipFileToFolder so I did not
    create a CVE, nor reported to
    https://github.com/snyk/zip-slip-vulnerability#user-content-projects-affected-and-fixed.
    If you think we should please shime in...
---
 .../java/org/apache/ofbiz/base/util/FileUtil.java  | 41 ++++++++++++++++++----
 .../apache/ofbiz/base/util/FileUtilTests.groovy    |  4 +--
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/FileUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/FileUtil.java
index 5c06cdd..7d08226 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/FileUtil.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/FileUtil.java
@@ -370,12 +370,32 @@ public final class FileUtil {
     }
 
     /**
+     * This useful to prevents Zip slip vulnerability cf. https://snyk.io/research/zip-slip-vulnerability
+     * @param destinationDirName The file path name to normalize
+     * @param zipEntry Zip entry to check before creating
+     * @return A file in destinationDir
+     */
+    public static File newFile(String destinationDirName, ZipEntry zipEntry) throws IOException {
+        File destinationDir = new File(destinationDirName);
+
+        File destFile = new File(destinationDir, zipEntry.getName());
+        String destDirPath = destinationDir.getCanonicalPath();
+        String destFilePath = destFile.getCanonicalPath();
+
+        if (!destFilePath.startsWith(destDirPath + File.separator)) {
+            throw new IOException("Entry is outside of the target dir: " + zipEntry.getName());
+        }
+        return destFile;
+    }
+
+    /**
      * Unzip file structure of the given zipFile to specified outputFolder
      * @param zipFile
      * @param outputFolder
+     * @param handleZipSlip if true FileUtil::newFile is used
      * @throws IOException
      */
-    public static void unzipFileToFolder(File zipFile, String outputFolder) throws IOException {
+    public static boolean unzipFileToFolder(File zipFile, String outputFolder, boolean handleZipSlip) throws IOException {
         byte[] buffer = new byte[8192];
 
         //create output directory if not exists
@@ -384,15 +404,23 @@ public final class FileUtil {
             folder.mkdir();
         }
 
-        //get the zip file content
+        // get the Zip file content
         ZipInputStream zis = new ZipInputStream(new FileInputStream(zipFile));
-        //get the zipped file list entry
+        // get the zipped file list entry
         ZipEntry ze = zis.getNextEntry();
 
         while (ze != null) {
-            String fileName = ze.getName();
-            File newFile = new File(outputFolder, fileName);
-
+            File newFile = null;
+            if (handleZipSlip) {
+                newFile = newFile(outputFolder, ze); // Prevents Zip slip vulnerability
+                if (null == newFile) {
+                    zis.closeEntry();
+                    zis.close();
+                    return false;
+                }
+            } else {
+                newFile = new File(outputFolder, ze.getName());
+            }
             //create all non existing folders
             //else you will hit FileNotFoundException for compressed folder
             new File(newFile.getParent()).mkdirs();
@@ -407,6 +435,7 @@ public final class FileUtil {
         }
         zis.closeEntry();
         zis.close();
+        return true;
     }
 
     /**
diff --git a/framework/base/src/test/groovy/org/apache/ofbiz/base/util/FileUtilTests.groovy b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/FileUtilTests.groovy
index ff39594..d9a2abd 100644
--- a/framework/base/src/test/groovy/org/apache/ofbiz/base/util/FileUtilTests.groovy
+++ b/framework/base/src/test/groovy/org/apache/ofbiz/base/util/FileUtilTests.groovy
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.ofbiz.base.util;
+package org.apache.ofbiz.base.util
 
 import org.apache.commons.io.FileUtils
 import org.junit.Test
@@ -55,7 +55,7 @@ public class FileUtilTests {
         if (readme.exists()) readme.delete()
 
         //validate unzip and compare the two files
-        FileUtil.unzipFileToFolder(readmeZipped, zipFilePath)
+        FileUtil.unzipFileToFolder(readmeZipped, zipFilePath, false)
 
         assert FileUtils.contentEquals(originalReadme, new File(zipFilePath, fileName))
     }