SONAR-10661 fix vulnerability in ZipUtils#unzip()

diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/utils/ZipUtils.java b/sonar-plugin-api/src/main/java/org/sonar/api/utils/ZipUtils.java
index 5d9f0a965a601a7072553670f60a52d8311229c4..a4fdbae4890ba4ca1af83dc36e1355ddcde8a111 100644 (file)
--- a/sonar-plugin-api/src/main/java/org/sonar/api/utils/ZipUtils.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/utils/ZipUtils.java
@@ -26,6 +26,7 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.nio.file.Path;
 import java.util.Enumeration;
 import java.util.function.Predicate;
 import java.util.zip.ZipEntry;
@@ -101,6 +102,8 @@ public final class ZipUtils {
 
   private static void unzipEntry(ZipEntry entry, ZipInputStream zipStream, File toDir) throws IOException {
     File to = new File(toDir, entry.getName());
+    verifyInsideTargetDirectory(entry, to.toPath(), toDir.toPath());
+
     if (entry.isDirectory()) {
       throwExceptionIfDirectoryIsNotCreatable(to);
     } else {
@@ -139,19 +142,23 @@ public final class ZipUtils {
       FileUtils.forceMkdir(toDir);
     }
 
+    Path targetDirNormalizedPath = toDir.toPath().normalize();
     ZipFile zipFile = new ZipFile(zip);
     try {
       Enumeration<? extends ZipEntry> entries = zipFile.entries();
       while (entries.hasMoreElements()) {
         ZipEntry entry = entries.nextElement();
         if (filter.test(entry)) {
-          File to = new File(toDir, entry.getName());
+          File target = new File(toDir, entry.getName());
+
+          verifyInsideTargetDirectory(entry, target.toPath(), targetDirNormalizedPath);
+
           if (entry.isDirectory()) {
-            throwExceptionIfDirectoryIsNotCreatable(to);
+            throwExceptionIfDirectoryIsNotCreatable(target);
           } else {
-            File parent = to.getParentFile();
+            File parent = target.getParentFile();
             throwExceptionIfDirectoryIsNotCreatable(parent);
-            copy(zipFile, entry, to);
+            copy(zipFile, entry, target);
           }
         }
       }
@@ -238,6 +245,13 @@ public final class ZipUtils {
     }
   }
 
+  private static void verifyInsideTargetDirectory(ZipEntry entry, Path entryPath, Path targetDirPath) {
+    if (!entryPath.normalize().startsWith(targetDirPath.normalize())) {
+      // vulnerability - trying to create a file outside the target directory
+      throw new IllegalStateException("Unzipping an entry outside the target directory is not allowed: " + entry.getName());
+    }
+  }
+
   /**
    * @see #unzip(File, File, Predicate)
    * @deprecated replaced by {@link Predicate<ZipEntry>} in 6.2.

diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/utils/ZipUtilsTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/utils/ZipUtilsTest.java
index d721585d477ff1debd3a2291ec67cad4f6e6bf44..76e86dc99fd5170b5609caa5519e63d3941ee3ab 100644 (file)
--- a/sonar-plugin-api/src/test/java/org/sonar/api/utils/ZipUtilsTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/utils/ZipUtilsTest.java
@@ -20,19 +20,20 @@
 package org.sonar.api.utils;
 
 import com.google.common.collect.Iterators;
-import java.net.URL;
-import org.apache.commons.io.FileUtils;
-import org.assertj.core.util.Files;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-
 import java.io.File;
+import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.URL;
 import java.util.Iterator;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
+import org.apache.commons.io.FileUtils;
+import org.assertj.core.util.Files;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -40,6 +41,8 @@ public class ZipUtilsTest {
 
   @Rule
   public TemporaryFolder temp = new TemporaryFolder();
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
 
   @Test
   public void zip_directory() throws IOException {
@@ -106,6 +109,30 @@ public class ZipUtilsTest {
     assertThat(toDir.listFiles()).containsOnly(new File(toDir, "foo.txt"));
   }
 
+  @Test
+  public void fail_if_unzipping_file_outside_target_directory() throws Exception {
+    File zip = new File(getClass().getResource("ZipUtilsTest/zip-slip.zip").toURI());
+    File toDir = temp.newFolder();
+
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("Unzipping an entry outside the target directory is not allowed: ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt");
+
+    ZipUtils.unzip(zip, toDir);
+  }
+
+  @Test
+  public void fail_if_unzipping_stream_outside_target_directory() throws Exception {
+    File zip = new File(getClass().getResource("ZipUtilsTest/zip-slip.zip").toURI());
+    File toDir = temp.newFolder();
+
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("Unzipping an entry outside the target directory is not allowed: ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt");
+
+    try (InputStream input = new FileInputStream(zip)) {
+      ZipUtils.unzip(input, toDir);
+    }
+  }
+
   private URL urlToZip() {
     return getClass().getResource("/org/sonar/api/utils/ZipUtilsTest/shouldUnzipFile.zip");
   }

diff --git a/sonar-plugin-api/src/test/resources/org/sonar/api/utils/ZipUtilsTest/zip-slip.zip b/sonar-plugin-api/src/test/resources/org/sonar/api/utils/ZipUtilsTest/zip-slip.zip
new file mode 100644 (file)
index 0000000..38b3f49
Binary files /dev/null and b/sonar-plugin-api/src/test/resources/org/sonar/api/utils/ZipUtilsTest/zip-slip.zip differ