Fix zip slip security flaw

Apply zip slip checking in zip operations throughout the system.
Centralizes most of the zip logic in one class. Create tests to zip
functionalities and zip slip problem.

Change-Id: I721f3d44b34fe6d242c9537f5a515ce1bb534c9a
Issue-ID: SDC-1401
Signed-off-by: andre.schmid <andre.schmid@est.tech>

3 files changed, 82 insertions, 68 deletions

diff --git a/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/pom.xml b/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/pom.xml
index ce88037706..b86964f61f 100644
--- a/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/pom.xml
+++ b/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/pom.xml
@@ -66,6 +66,18 @@
             <version>${commons.codec.version}</version>
         </dependency>
         <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest</artifactId>
+            <version>${hamcrest.version}</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-library</artifactId>
+            <version>${hamcrest.version}</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
             <scope>test</scope>
@@ -85,6 +97,12 @@
             <artifactId>openecomp-sdc-logging-api</artifactId>
             <version>${project.version}</version>
         </dependency>
+        <dependency>
+            <groupId>org.openecomp.sdc</groupId>
+            <artifactId>common-app-api</artifactId>
+            <version>${project.version}</version>
+            <scope>compile</scope>
+        </dependency>
     </dependencies>
 
 </project>
diff --git a/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/main/java/org/openecomp/core/utilities/file/FileUtils.java b/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/main/java/org/openecomp/core/utilities/file/FileUtils.java
index c807d1b979..31338dcda4 100644
--- a/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/main/java/org/openecomp/core/utilities/file/FileUtils.java
+++ b/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/main/java/org/openecomp/core/utilities/file/FileUtils.java
@@ -16,11 +16,6 @@
 
 package org.openecomp.core.utilities.file;
 
-import org.apache.commons.io.FilenameUtils;
-import org.apache.commons.io.IOUtils;
-import org.onap.sdc.tosca.services.YamlUtil;
-import org.openecomp.core.utilities.json.JsonUtil;
-
 import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.FileOutputStream;
@@ -28,11 +23,20 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
 import java.nio.file.Path;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.function.Function;
-import java.util.zip.ZipEntry;
-import java.util.zip.ZipException;
-import java.util.zip.ZipInputStream;
-import java.util.*;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.commons.io.IOUtils;
+import org.onap.sdc.tosca.services.YamlUtil;
+import org.openecomp.core.utilities.json.JsonUtil;
+import org.openecomp.sdc.common.zip.ZipUtils;
+import org.openecomp.sdc.common.zip.exception.ZipException;
 
 /**
  * The type File utils.
@@ -220,26 +224,14 @@ public class FileUtils {
    *
    * @param zipData the zip data
    * @return the file content map from zip
-   * @throws IOException the io exception
+   * @throws ZipException when an error occurs while extracting zip files
    */
-  public static FileContentHandler getFileContentMapFromZip(byte[] zipData) throws IOException {
-
-    try (ZipInputStream inputZipStream = new ZipInputStream(new ByteArrayInputStream(zipData))) {
-
-      FileContentHandler mapFileContent = new FileContentHandler();
-
-      ZipEntry zipEntry;
-
-      while ((zipEntry = inputZipStream.getNextEntry()) != null) {
-        assertEntryNotVulnerable(zipEntry);
-        mapFileContent.addFile(zipEntry.getName(), FileUtils.toByteArray(inputZipStream));
-      }
-
-      return mapFileContent;
-
-    } catch (RuntimeException exception) {
-      throw new IOException(exception);
-    }
+  public static FileContentHandler getFileContentMapFromZip(byte[] zipData)
+      throws ZipException {
+    final Map<String, byte[]> zipFileAndByteMap = ZipUtils.readZip(zipData, true);
+    final FileContentHandler mapFileContent = new FileContentHandler();
+    mapFileContent.setFiles(zipFileAndByteMap);
+    return mapFileContent;
   }
 
 
@@ -286,20 +278,28 @@ public class FileUtils {
    * @return a map containing file names and their absolute paths
    * @throws IOException the io exception
    */
-  public static Map<String, String> writeFilesFromFileContentHandler(FileContentHandler
-                                                                         fileContentHandler,
-                                                                     Path dir)
-      throws IOException {
-
+  public static Map<String, String> writeFilesFromFileContentHandler(final FileContentHandler fileContentHandler,
+                                                                     final Path dir) throws IOException {
     File file;
-    File dirFile = dir.toFile();
-    Map<String, String> filePaths = new HashMap<>();
-    for (Map.Entry<String, byte[]> fileEntry : fileContentHandler.getFiles().entrySet()) {
+    final File dirFile = dir.toFile();
+    final Map<String, String> filePaths = new HashMap<>();
+    for (final Map.Entry<String, byte[]> fileEntry : fileContentHandler.getFiles().entrySet()) {
       file = new File(dirFile, fileEntry.getKey());
-      file.getParentFile().mkdirs();
       filePaths.put(fileEntry.getKey(), file.getAbsolutePath());
-      try (FileOutputStream fop = new FileOutputStream(file.getAbsolutePath());) {
-        fop.write(fileEntry.getValue());
+      final byte[] fileBytes = fileEntry.getValue();
+      if (fileBytes == null) {
+        if (!file.exists() && !file.mkdirs()) {
+          throw new IOException("Could not create directory " + file.getAbsolutePath());
+        }
+        continue;
+      } else {
+        if (!file.getParentFile().exists() && !file.getParentFile().mkdirs()) {
+          throw new IOException("Could not create parent directory for " + file.getAbsolutePath());
+        }
+      }
+
+      try (final FileOutputStream fop = new FileOutputStream(file.getAbsolutePath());) {
+        fop.write(fileBytes);
         fop.flush();
       }
     }
@@ -318,10 +318,4 @@ public class FileUtils {
         fileExtension.equalsIgnoreCase(FileExtension.YAML.getDisplayName());
   }
 
-  private static void assertEntryNotVulnerable(ZipEntry entry) throws ZipException {
-    if (entry.getName().contains("../")) {
-      throw new ZipException("Path traversal attempt discovered.");
-    }
-  }
-
 }
diff --git a/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/test/java/org/openecomp/core/utilities/file/FileUtilsTest.java b/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/test/java/org/openecomp/core/utilities/file/FileUtilsTest.java
index a4928ac739..facfe57622 100644
--- a/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/test/java/org/openecomp/core/utilities/file/FileUtilsTest.java
+++ b/openecomp-be/lib/openecomp-core-lib/openecomp-utilities-lib/src/test/java/org/openecomp/core/utilities/file/FileUtilsTest.java
@@ -17,8 +17,12 @@
 package org.openecomp.core.utilities.file;
 
 import static junit.framework.TestCase.assertTrue;
-import static org.junit.Assert.assertEquals;
+import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.anEmptyMap;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.core.Is.is;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 
 import java.io.File;
 import java.io.IOException;
@@ -34,6 +38,7 @@ import java.util.stream.Stream;
 import org.apache.commons.io.IOUtils;
 import org.junit.Assert;
 import org.junit.Test;
+import org.openecomp.sdc.common.zip.exception.ZipException;
 
 /**
  * @author EVITALIY
@@ -74,25 +79,22 @@ public class FileUtilsTest {
     }
 
     @Test
-    public void testWriteFilesFromFileContentHandler() throws IOException {
-        Path dir = Files.createTempDirectory("CSAR_" + System.currentTimeMillis());
+    public void testWriteFilesFromFileContentHandler() throws IOException, ZipException {
+        final Path tempDirectory = Files.createTempDirectory("CSAR_" + System.currentTimeMillis());
         try {
-            byte[] uploadedFileData = IOUtils.toByteArray(
-                FileUtilsTest.class.getResource("resource-Spgw-csar-ZTE" +
-                    ".csar"));
-            FileContentHandler contentMap = FileUtils.getFileContentMapFromZip(uploadedFileData);
-            Map<String, String> filePaths = FileUtils.writeFilesFromFileContentHandler(contentMap,
-                dir);
-
-            assertFalse(filePaths.isEmpty());
-            assertEquals(filePaths.size(), 18);
-            for (Map.Entry<String, String> fileEntry : filePaths.entrySet()) {
-                File f = new File(fileEntry.getValue());
-                assertTrue(f.exists());
+            byte[] uploadedFileData =
+                IOUtils.toByteArray(FileUtilsTest.class.getResource("resource-Spgw-csar-ZTE.csar"));
+            final FileContentHandler contentMap = FileUtils.getFileContentMapFromZip(uploadedFileData);
+            final Map<String, String> filePaths = FileUtils.writeFilesFromFileContentHandler(contentMap, tempDirectory);
+
+            assertThat("The file map should not be empty", filePaths, is(not(anEmptyMap())));
+            assertThat("The file map should have size 20", filePaths, is(aMapWithSize(20)));
+            for (final Map.Entry<String, String> fileEntry : filePaths.entrySet()) {
+                final File f = new File(fileEntry.getValue());
+                assertThat(String.format("The file '%s' is expected to", f.getAbsolutePath()), f.exists(), is(true));
             }
-        }
-        finally {
-            org.apache.commons.io.FileUtils.deleteDirectory(dir.toFile());
+        } finally {
+            org.apache.commons.io.FileUtils.deleteDirectory(tempDirectory.toFile());
         }
     }
 
@@ -106,22 +108,22 @@ public class FileUtilsTest {
 
     @Test
     public void testGetFileWithoutExtention() {
-        Assert.assertEquals(FileUtils.getFileWithoutExtention("test.txt"), "test");
+        Assert.assertEquals("test", FileUtils.getFileWithoutExtention("test.txt"));
     }
 
     @Test
     public void testGetFileWithoutExtentionContainsNoExtension() {
-        Assert.assertEquals(FileUtils.getFileWithoutExtention("test"), "test");
+        Assert.assertEquals("test", FileUtils.getFileWithoutExtention("test"));
     }
 
     @Test
     public void testGetFileExtention() {
-        Assert.assertEquals(FileUtils.getFileExtension("test.txt"), "txt");
+        Assert.assertEquals("txt", FileUtils.getFileExtension("test.txt"));
     }
 
     @Test
     public void testGetNetworkPackageName() {
-        Assert.assertEquals(FileUtils.getNetworkPackageName("heat.zip"), "heat");
+        Assert.assertEquals("heat", FileUtils.getNetworkPackageName("heat.zip"));
     }
 
     @Test
@@ -191,6 +193,6 @@ public class FileUtilsTest {
         }
 
         Assert.assertNotNull(inputStream);
-        Assert.assertEquals(builder.toString(), "hello-test");
+        Assert.assertEquals("hello-test", builder.toString());
     }
 }