From bf5eeb23a769a2e2b75f432b74f10fdbcfd2f161 Mon Sep 17 00:00:00 2001 From: "andre.schmid" Date: Fri, 27 Sep 2019 13:27:11 +0100 Subject: 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 --- .../org/openecomp/sdc/common/utils/CommonUtil.java | 118 ++++++--------------- .../openecomp/sdc/common/utils/CommonUtilTest.java | 18 ++-- 2 files changed, 42 insertions(+), 94 deletions(-) (limited to 'openecomp-be/lib/openecomp-common-lib') diff --git a/openecomp-be/lib/openecomp-common-lib/src/main/java/org/openecomp/sdc/common/utils/CommonUtil.java b/openecomp-be/lib/openecomp-common-lib/src/main/java/org/openecomp/sdc/common/utils/CommonUtil.java index ae7d44efd8..8610ecb74b 100644 --- a/openecomp-be/lib/openecomp-common-lib/src/main/java/org/openecomp/sdc/common/utils/CommonUtil.java +++ b/openecomp-be/lib/openecomp-common-lib/src/main/java/org/openecomp/sdc/common/utils/CommonUtil.java @@ -20,62 +20,50 @@ package org.openecomp.sdc.common.utils; import com.google.common.collect.Multimap; - +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.io.FilenameUtils; import org.apache.commons.lang3.tuple.ImmutablePair; import org.apache.commons.lang3.tuple.Pair; import org.openecomp.core.utilities.file.FileContentHandler; -import org.openecomp.core.utilities.file.FileUtils; import org.openecomp.core.utilities.orchestration.OnboardingTypesEnum; import org.openecomp.sdc.common.errors.CoreException; import org.openecomp.sdc.common.errors.ErrorCategory; import org.openecomp.sdc.common.errors.ErrorCode; import org.openecomp.sdc.common.errors.Messages; - -import java.io.ByteArrayInputStream; -import java.io.File; -import java.io.IOException; -import java.lang.reflect.Field; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.Set; -import java.util.zip.ZipEntry; -import java.util.zip.ZipException; -import java.util.zip.ZipInputStream; +import org.openecomp.sdc.common.zip.ZipUtils; +import org.openecomp.sdc.common.zip.exception.ZipException; public class CommonUtil { - static final String DEFAULT = "default"; - static final String _DEFAULT = "_default"; private CommonUtil() { // prevent instantiation } - public static FileContentHandler validateAndUploadFileContent(OnboardingTypesEnum type, - byte[] uploadedFileData) - throws IOException { - return getFileContentMapFromOrchestrationCandidateZipAndValidateNoFolders(type, - uploadedFileData); - } - /** - * Gets files out of the zip AND validates zip is flat (no folders) + * Reads the files from the zip AND validates zip is flat (no folders). * - * @param uploadFileData zip file + * @param type the onboarding type + * @param uploadedFileData zip file bytes * @return FileContentHandler if input is valid and has no folders + * @throws IOException when the zip could not be read */ - private static FileContentHandler getFileContentMapFromOrchestrationCandidateZipAndValidateNoFolders( - OnboardingTypesEnum type, byte[] uploadFileData) - throws IOException { - Pair> pair = - getFileContentMapFromOrchestrationCandidateZip(uploadFileData); - + public static FileContentHandler validateAndUploadFileContent(final OnboardingTypesEnum type, + final byte[] uploadedFileData) throws IOException { + final Pair> pair; + try { + pair = getFileContentMapFromOrchestrationCandidateZip(uploadedFileData); + } catch (final ZipException e) { + throw new IOException(e); + } if (isFileOriginFromZip(type.toString())) { validateNoFolders(pair.getRight()); } @@ -84,47 +72,22 @@ public class CommonUtil { } public static Pair> getFileContentMapFromOrchestrationCandidateZip( - byte[] uploadFileData) - throws IOException { - ZipEntry zipEntry; - List folderList = new ArrayList<>(); - FileContentHandler mapFileContent = new FileContentHandler(); - try (ByteArrayInputStream in = new ByteArrayInputStream(uploadFileData); - ZipInputStream inputZipStream = new ZipInputStream(in)) { - byte[] fileByteContent; - String currentEntryName; - - while ((zipEntry = inputZipStream.getNextEntry()) != null) { - assertEntryNotVulnerable(zipEntry); - currentEntryName = zipEntry.getName(); - fileByteContent = FileUtils.toByteArray(inputZipStream); - - int index = lastIndexFileSeparatorIndex(currentEntryName); - if (index != -1) { - folderList.add(currentEntryName); - } - if (isFile(currentEntryName)) { - mapFileContent.addFile(currentEntryName, fileByteContent); - } + byte[] uploadFileData) throws ZipException { + final Map zipFileMap = ZipUtils.readZip(uploadFileData, true); + final List folderList = new ArrayList<>(); + final FileContentHandler mapFileContent = new FileContentHandler(); + + zipFileMap.forEach((key, value) -> { + if (value == null) { + folderList.add(key); + } else { + mapFileContent.addFile(key, value); } - - } catch (RuntimeException exception) { - throw new IOException(exception); - } + }); return new ImmutablePair<>(mapFileContent, folderList); } - private static void assertEntryNotVulnerable(ZipEntry entry) throws ZipException { - if (entry.getName().contains("../")) { - throw new ZipException("Path traversal attempt discovered."); - } - } - - private static boolean isFile(String currentEntryName) { - return !(currentEntryName.endsWith("\\") || currentEntryName.endsWith("/")); - } - private static void validateNoFolders(List folderList) { if (CollectionUtils.isNotEmpty(folderList)) { throw new CoreException((new ErrorCode.ErrorCodeBuilder()) @@ -134,19 +97,6 @@ public class CommonUtil { } } - private static int lastIndexFileSeparatorIndex(String filePath) { - int length = filePath.length() - 1; - - for (int i = length; i >= 0; i--) { - char currChar = filePath.charAt(i); - if (currChar == '/' || currChar == File.separatorChar || currChar == File.pathSeparatorChar) { - return i; - } - } - // if we've reached to the start of the string and didn't find file separator - return -1 - return -1; - } - private static boolean validateFilesExtensions(Set allowedExtensions, FileContentHandler files) { for (String fileName : files.getFileList()) { diff --git a/openecomp-be/lib/openecomp-common-lib/src/test/java/org/openecomp/sdc/common/utils/CommonUtilTest.java b/openecomp-be/lib/openecomp-common-lib/src/test/java/org/openecomp/sdc/common/utils/CommonUtilTest.java index bc0bd137a0..119616a9be 100644 --- a/openecomp-be/lib/openecomp-common-lib/src/test/java/org/openecomp/sdc/common/utils/CommonUtilTest.java +++ b/openecomp-be/lib/openecomp-common-lib/src/test/java/org/openecomp/sdc/common/utils/CommonUtilTest.java @@ -16,23 +16,21 @@ package org.openecomp.sdc.common.utils; -import org.testng.annotations.Test; +import static org.onap.sdc.tosca.services.CommonUtil.DEFAULT; +import static org.onap.sdc.tosca.services.CommonUtil.UNDERSCORE_DEFAULT; +import static org.testng.Assert.assertTrue; import java.util.HashMap; import java.util.Map; - -import static org.testng.Assert.assertTrue; +import org.testng.annotations.Test; public class CommonUtilTest { @Test public void testGetObjectAsMap() { - Map obj = new HashMap<>(1); - obj.put(CommonUtil.DEFAULT, ""); - Map newMap = CommonUtil.getObjectAsMap(obj); - - boolean exists = newMap.containsKey(CommonUtil._DEFAULT); - - assertTrue(exists); + final Map obj = new HashMap<>(1); + obj.put(DEFAULT, ""); + assertTrue(CommonUtil.getObjectAsMap(obj).containsKey(UNDERSCORE_DEFAULT)); } + } -- cgit 1.2.3-korg