diff --git a/modules/collect/src/main/java/com/opengamma/strata/collect/io/ZipUtils.java b/modules/collect/src/main/java/com/opengamma/strata/collect/io/ZipUtils.java index f09df8b342..2f6177b407 100644 --- a/modules/collect/src/main/java/com/opengamma/strata/collect/io/ZipUtils.java +++ b/modules/collect/src/main/java/com/opengamma/strata/collect/io/ZipUtils.java @@ -11,6 +11,7 @@ import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.util.Base64; import java.util.HashSet; @@ -21,18 +22,21 @@ import java.util.function.BiConsumer; import java.util.zip.GZIPInputStream; import java.util.zip.ZipEntry; +import java.util.zip.ZipException; import java.util.zip.ZipInputStream; import java.util.zip.ZipOutputStream; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.io.ByteStreams; import com.opengamma.strata.collect.ArgChecker; /** * Utility class to simplify accessing and creating zip files, and other packed formats. */ public final class ZipUtils { + // need to watch out for ZIP slip attack when unzipping to file system + // https://github.com/snyk/zip-slip-vulnerability + private static final Path DUMMY_PATH = Paths.get("/dummy/"); private ZipUtils() { } @@ -53,6 +57,7 @@ public static Set unzipPathNames(BeanByteSource source) { try (ZipInputStream in = new ZipInputStream(source.openStream())) { ZipEntry entry = in.getNextEntry(); while (entry != null) { + validateZipPathName(DUMMY_PATH, entry); if (!entry.isDirectory()) { entryNames.add(entry.getName()); } @@ -82,8 +87,10 @@ public static Optional unzipPathNameInMemory(BeanByteSource sou try (ZipInputStream in = new ZipInputStream(source.openStream())) { ZipEntry entry = in.getNextEntry(); while (entry != null) { + validateZipPathName(DUMMY_PATH, entry); if (!entry.isDirectory() && entry.getName().equals(relativePathName)) { - return Optional.of(ArrayByteSource.ofUnsafe(ByteStreams.toByteArray(in)).withFileName(entry.getName())); + ArrayByteSource extractedBytes = extractInputStream(in, entry.getName()); + return Optional.of(extractedBytes); } in.closeEntry(); entry = in.getNextEntry(); @@ -103,15 +110,17 @@ public static Optional unzipPathNameInMemory(BeanByteSource sou * @param source the byte source to unzip * @param path the path to unzip to * @throws UncheckedIOException if an IO error occurs + * @throws SecurityException if the path is not absolute and the calling thread cannot access system property "user.dir" */ public static void unzip(BeanByteSource source, Path path) { + Path absolutePath = path.toAbsolutePath(); Set deduplicate = new HashSet<>(); try (ZipInputStream in = new ZipInputStream(source.openStream())) { ZipEntry entry = in.getNextEntry(); while (entry != null) { + Path resolved = validateZipPathName(absolutePath, entry); if (!entry.isDirectory()) { - if (deduplicate.add(new ZipKey(entry))) { - Path resolved = path.resolve(entry.getName()); + if (deduplicate.add(new ZipKey(entry, resolved))) { Files.createDirectories(resolved); Files.copy(in, resolved, StandardCopyOption.REPLACE_EXISTING); } @@ -182,6 +191,7 @@ public static Map unpackInMemory(BeanByteSource source) *

* Unpacking handles ZIP, GZ and BASE64 formats based entirely on the suffix of the input file name. * If the input suffix is not recognized as a packed format, the consumer is invoked with the original file. + * This method is not recursive. * * @param source the byte source to unpack * @param consumer the consumer, which is passed the relative path name and content for each entry @@ -198,8 +208,8 @@ public static void unpackInMemory(BeanByteSource source, BiConsumer consumer) { try (GZIPInputStream in = new GZIPInputStream(source.openStream())) { String shortFileName = fileName.substring(0, fileName.length() - 3); - ArrayByteSource entrySource = ArrayByteSource.ofUnsafe(ByteStreams.toByteArray(in)).withFileName(shortFileName); - consumer.accept(shortFileName, entrySource); + ArrayByteSource extractedBytes = extractInputStream(in, shortFileName); + consumer.accept(shortFileName, extractedBytes); } catch (IOException ex) { throw new UncheckedIOException(ex); } @@ -275,25 +286,44 @@ private static boolean suffixMatches(String name, String suffix) { return name.regionMatches(true, name.length() - suffix.length(), suffix, 0, suffix.length()); } + // unzip the input, trying to recover from large files or ZIP bombs + private static ArrayByteSource extractInputStream(InputStream in, String fileName) throws IOException { + try { + return ArrayByteSource.from(in).withFileName(fileName); + } catch (OutOfMemoryError ex) { + System.gc(); + throw new IOException("Unzipped input too large: " + fileName); + } + } + + // prevent ZIP slip attack + private static Path validateZipPathName(Path rootPath, ZipEntry entry) throws ZipException { + Path resolved = rootPath.normalize().resolve(entry.getName()).normalize(); + if (!resolved.startsWith(rootPath)) { + throw new ZipException("ZIP file contains illegal file name: " + entry.getName()); + } + return resolved; + } + //------------------------------------------------------------------------- // handle duplicate entries in a zip file // while such a zip file is stupid, it is apparently valid private static class ZipKey { - private final String fileName; + private final Path resolvedPath; private final long crc; private final long size; - private ZipKey(ZipEntry entry) { - fileName = entry.getName(); - crc = entry.getCrc(); - size = entry.getSize(); + private ZipKey(ZipEntry entry, Path resolvedPath) { + this.resolvedPath = resolvedPath; + this.crc = entry.getCrc(); + this.size = entry.getSize(); } @Override public boolean equals(Object obj) { if (obj instanceof ZipKey) { ZipKey key = (ZipKey) obj; - return fileName.equals(key.fileName) && + return resolvedPath.equals(key.resolvedPath) && crc == key.crc && size == key.size; } diff --git a/modules/collect/src/test/java/com/opengamma/strata/collect/io/ZipUtilsTest.java b/modules/collect/src/test/java/com/opengamma/strata/collect/io/ZipUtilsTest.java index 6161072cfb..86c8a4662f 100644 --- a/modules/collect/src/test/java/com/opengamma/strata/collect/io/ZipUtilsTest.java +++ b/modules/collect/src/test/java/com/opengamma/strata/collect/io/ZipUtilsTest.java @@ -6,11 +6,13 @@ package com.opengamma.strata.collect.io; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.fail; import static org.assertj.core.api.Assertions.within; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -62,6 +64,14 @@ public void test_unzipPathNames() { assertThat(names).containsOnly("test/alpha/Alpha.txt", "test/beta/Beta1.txt", "test/beta/Beta2.txt"); } + @Test + public void test_unzipPathNames_zipSlip() { + ArrayByteSource zipFile = ResourceLocator.ofClasspath(ZipUtilsTest.class, "zip-slip.zip").getByteSource().load(); + + assertThatExceptionOfType(UncheckedIOException.class) + .isThrownBy(() -> ZipUtils.unzipPathNames(zipFile)); + } + //------------------------------------------------------------------------- @Test public void test_unzipPathNameInMemory() { @@ -78,6 +88,14 @@ public void test_unzipPathNameInMemory() { .hasValue("BETA2"); } + @Test + public void test_unzipPathNameInMemory_zipSlip() { + ArrayByteSource zipFile = ResourceLocator.ofClasspath(ZipUtilsTest.class, "zip-slip.zip").getByteSource().load(); + + assertThatExceptionOfType(UncheckedIOException.class) + .isThrownBy(() -> ZipUtils.unzipPathNameInMemory(zipFile, "test/alpha/Alpha.txt")); + } + //------------------------------------------------------------------------- @Test public void test_unzip_toPath() { @@ -102,6 +120,14 @@ public void test_unzip_toPath_withFolders() { assertThat(tmpDir.resolve("test/beta/Beta2.txt")).hasContent("BETA2"); } + @Test + public void test_unzip_toPath_zipSlip() { + ArrayByteSource zipFile = ResourceLocator.ofClasspath(ZipUtilsTest.class, "zip-slip.zip").getByteSource().load(); + + assertThatExceptionOfType(UncheckedIOException.class) + .isThrownBy(() -> ZipUtils.unzip(zipFile, tmpDir)); + } + //------------------------------------------------------------------------- @Test public void test_zipInMemory() throws Exception { @@ -143,7 +169,7 @@ public void test_unzipInMemory_toMap_zip() { //------------------------------------------------------------------------- @Test - public void test_unzipInMemory_zip() { + public void test_unzipInMemory() { ArrayByteSource source1 = ArrayByteSource.ofUtf8("Hello World").withFileName("TestFile1.txt"); ArrayByteSource source2 = ArrayByteSource.ofUtf8("Hello Planet").withFileName("TestFile2.txt"); ArrayByteSource zipFile = ZipUtils.zipInMemory(ImmutableList.of(source1, source2)).withFileName("Test.foo"); @@ -164,6 +190,20 @@ public void test_unzipInMemory_zip() { assertThat(counter).hasValue(2); } + @Test + public void test_unzipInMemory_zipSlip() { + ArrayByteSource zipFile = ResourceLocator.ofClasspath(ZipUtilsTest.class, "zip-slip.zip").getByteSource().load(); + + assertThatExceptionOfType(UncheckedIOException.class) + .isThrownBy(() -> ZipUtils.unzipInMemory( + zipFile, + (name, extracted) -> { + if (!name.equals("good.txt")) { + fail("Should not get here: " + name); + } + })); + } + //------------------------------------------------------------------------- @Test public void test_unpackInMemory_toMap_zip() { diff --git a/modules/collect/src/test/resources/com/opengamma/strata/collect/io/zip-slip.zip b/modules/collect/src/test/resources/com/opengamma/strata/collect/io/zip-slip.zip new file mode 100644 index 0000000000..38b3f499de Binary files /dev/null and b/modules/collect/src/test/resources/com/opengamma/strata/collect/io/zip-slip.zip differ