Issue #400 - Check for arbitrary file overwrite vulnerability (zip slip) (#401)

Author: rakillen

core/src/main/java/oracle/weblogic/deploy/util/FileUtils.java

@@ -652,13 +652,22 @@ public static void extractZipFileContent(WLSDeployArchive archiveFile, String zi
                     Files.createDirectory(Paths.get(extractPath));
                 }
 
+                // verify that each target file is under the extract directory,
+                // to protect from the file overwrite security vulnerability (zip slip).
+                String canonicalExtractPath = extractDir.getCanonicalPath();
+
                 byte[] buffer = new byte[1024];
                 FileInputStream fis = new FileInputStream(walletZip);
                 ZipInputStream zis = new ZipInputStream(fis);
                 ZipEntry ze = zis.getNextEntry();
                 while (ze != null) {
                     String fileName = ze.getName();
                     File newFile = new File(extractPath + File.separator + fileName);
+                    String canonicalNewFile = newFile.getCanonicalPath();
+                    if(!canonicalNewFile.startsWith(canonicalExtractPath + File.separator)) {
+                        throw new WLSDeployArchiveIOException("WLSDPLY-01119", ze.getName());
+                    }
+
                     new File(newFile.getParent()).mkdirs();
                     FileOutputStream fos = new FileOutputStream(newFile);
                     int len = zis.read(buffer);
@@ -677,7 +686,8 @@ public static void extractZipFileContent(WLSDeployArchive archiveFile, String zi
                 Files.delete(Paths.get(walletZip));
             }
         } catch (IOException | WLSDeployArchiveIOException ioe) {
-            String message = ExceptionHelper.getMessage("WLSDPLY-01118", METHOD, CLASS, ioe.getLocalizedMessage());
+            String message = ExceptionHelper.getMessage("WLSDPLY-01118", archiveFile.getArchiveFileName(),
+                    ioe.getLocalizedMessage());
             IllegalArgumentException iae = new IllegalArgumentException(message);
             LOGGER.throwing(CLASS, METHOD, iae);
             throw iae;

core/src/main/resources/oracle/weblogic/deploy/messages/wlsdeploy_rb.properties

@@ -128,7 +128,9 @@ WLSDPLY-01114=Deleting the file {1} in directory {0}
 WLSDPLY-01115=Unable to delete file {0} from directory {1}
 WLSDPLY-01116=Unable to successfully delete the directory {0}
 WLSDPLY-01117=Model directory {0} has more than one {1} file, found {2} after previously finding {3}
-WLSDPLY-01118=Error extracting zipentry zip file {0}
+WLSDPLY-01118=Error extracting zipentry zip file {0}: {1}
+WLSDPLY-01119=Zip entry is outside of the target directory: {0}
+
 # oracle.weblogic.deploy.util.ProcessHandler.java
 WLSDPLY-01200=Process for command {0} isRunning() unable to get an exit value: {1}
 WLSDPLY-01201=ProcessHandler had no registered wait handler when asked to exec() command: {0}

core/src/test/java/oracle/weblogic/deploy/util/FileUtilsTest.java

@@ -1,13 +1,18 @@
 /*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
  * The Universal Permissive License (UPL), Version 1.0
  */
 package oracle.weblogic.deploy.util;
 
+import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.io.FileOutputStream;
 import java.text.MessageFormat;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
 
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 
 public class FileUtilsTest {
@@ -49,6 +54,16 @@ public class FileUtilsTest {
     private static final String APP_PATH = "wlsdeploy/applications/simpleear.ear";
     private static final String APP_FILE_NAME = "src/test/resources/simpleear.ear";
 
+    private static final File UNIT_TEST_TARGET_DIR = new File(WLSDeployZipFileTest.UNIT_TEST_TARGET_DIR, "fileutils");
+    private static final String WALLET_PATH = "wlsdeploy/wallet.zip";
+
+    @Before
+    public void initialize() throws Exception {
+        if(!UNIT_TEST_TARGET_DIR.exists() && !UNIT_TEST_TARGET_DIR.mkdirs()) {
+            throw new Exception("Unable to create unit test directory: " + UNIT_TEST_TARGET_DIR);
+        }
+    }
+
     @Test
     public void testNormalFile_parseFileName() throws Exception {
         File f = new File(FILE1);
@@ -104,6 +119,56 @@ public void testHashing() throws Exception {
         Assert.assertEquals(appHash, archiveHash);
     }
 
+    @Test
+    /* A wallet zip inside the archive must not contain an entry such as ../info.txt,
+       since this creates a file overwrite security vulnerability (zip slip).
+     */
+    public void testZipVulnerability() throws Exception {
+        String extractPath = UNIT_TEST_TARGET_DIR.getPath();
+
+        // an entry with a simple name or path works fine
+        File zipFile = buildWalletArchiveZip("info.txt");
+        WLSDeployArchive deployArchive = new WLSDeployArchive(zipFile.getPath());
+        FileUtils.extractZipFileContent(deployArchive, WALLET_PATH, extractPath);
+
+        // an entry with parent directory notation should throw an exception
+        try {
+            zipFile = buildWalletArchiveZip("../info.txt");
+            deployArchive = new WLSDeployArchive(zipFile.getPath());
+            FileUtils.extractZipFileContent(deployArchive, WALLET_PATH, extractPath);
+            Assert.fail("Exception not thrown for zip entry outside extract directory");
+
+        } catch(IllegalArgumentException e) {
+            // expected behavior
+        }
+    }
+
+    /* Build an archive zip containing a wallet zip.
+       The wallet contains a single entry with the name of the contentName argument.
+     */
+    private File buildWalletArchiveZip(String contentName) throws Exception {
+
+        // create the wallet zip content
+        ByteArrayOutputStream walletBytes = new ByteArrayOutputStream();
+        try (ZipOutputStream zipStream = new ZipOutputStream(walletBytes)) {
+            ZipEntry zipEntry = new ZipEntry(contentName);
+            zipStream.putNextEntry(zipEntry);
+            byte[] data = "info".getBytes();
+            zipStream.write(data, 0, data.length);
+            zipStream.closeEntry();
+        }
+
+        File archiveFile = new File(UNIT_TEST_TARGET_DIR, "archive.zip");
+
+        try (ZipOutputStream zipStream = new ZipOutputStream(new FileOutputStream(archiveFile))) {
+            ZipEntry zipEntry = new ZipEntry(WALLET_PATH);
+            zipStream.putNextEntry(zipEntry);
+            zipStream.write(walletBytes.toByteArray());
+            zipStream.closeEntry();
+        }
+
+        return archiveFile;
+    }
 
     private void assertMatch(String name, String got, String expected) {
         Assert.assertTrue(MessageFormat.format(FILE_ERR_FORMAT, name, got, expected),