Security Hardening: Prevent Zip Slip (Arbitrary File Write) in ExtractUtil

java/com/google/copybara/remotefile/extractutil/ExtractUtil.java

@@ -14,14 +14,15 @@
  * limitations under the License.
  */
 package com.google.copybara.remotefile.extractutil;
-
+import java.nio.file.Path;
 import com.google.common.io.MoreFiles;
 import com.google.copybara.exception.ValidationException;
 import com.google.copybara.util.Glob;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.PathMatcher;
 import javax.annotation.Nullable;
 import org.apache.commons.compress.archivers.ArchiveEntry;
 import org.apache.commons.compress.archivers.ArchiveInputStream;
@@ -43,18 +44,26 @@ private ExtractUtil() {}
   public static void extractArchive(
       InputStream contents, Path targetPath, ExtractType type, @Nullable Glob fileFilter)
       throws IOException, ValidationException {
+    Path root = targetPath.toAbsolutePath().normalize();
+    PathMatcher rootedFilter = fileFilter != null ? fileFilter.relativeTo(root) : null;
+
     ArchiveEntry archiveEntry;
     try (ArchiveInputStream<?> inputStream = createArchiveInputStream(contents, type)) {
       while (((archiveEntry = inputStream.getNextEntry()) != null)) {
-        if ((fileFilter != null
-                && !fileFilter
-                    .relativeTo(targetPath.toAbsolutePath())
-                    .matches(targetPath.resolve(Path.of(archiveEntry.getName()))))
+        Path resolvedPath = root.resolve(archiveEntry.getName()).normalize();
+
+        // Security check: Prevent Zip Slip vulnerability
+        if (!resolvedPath.startsWith(root)) {
+          throw new IOException("Zip entry is outside of the target dir: " + archiveEntry.getName());
+        }
+
+        if ((rootedFilter != null && !rootedFilter.matches(resolvedPath))
             || archiveEntry.isDirectory()) {
           continue;
         }
-        Files.createDirectories(targetPath.resolve(archiveEntry.getName()).getParent());
-        MoreFiles.asByteSink(targetPath.resolve(archiveEntry.getName())).writeFrom(inputStream);
+
+        Files.createDirectories(resolvedPath.getParent());
+        MoreFiles.asByteSink(resolvedPath).writeFrom(inputStream);
       }
     }
   }

javatests/com/google/copybara/remotefile/extractutil/ExtractUtilTest.java

@@ -15,6 +15,7 @@
  */
 package com.google.copybara.remotefile.extractutil;
 
+import static org.junit.Assert.assertThrows;
 import static com.google.common.truth.Truth.assertThat;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
@@ -151,4 +152,26 @@ public void testExtractArchive_tarBz2File() throws Exception {
     assertThat(MoreFiles.asByteSource(outputPath.resolve("foo.txt")).asCharSource(UTF_8).read())
         .isEqualTo("copybara");
   }
+
+  @Test
+  public void testExtractArchive_zipSlipVulnerability() throws Exception {
+    Path maliciousZip = testFolder.resolve("malicious.zip");
+    try (ZipOutputStream zipOs = new ZipOutputStream(Files.newOutputStream(maliciousZip))) {
+      ZipEntry ze = new ZipEntry("../../evil.txt");
+      zipOs.putNextEntry(ze);
+      zipOs.write("evil content".getBytes(UTF_8));
+      zipOs.closeEntry();
+    }
+
+    Path outputPath = testFolder.resolve("output_safe");
+    Files.createDirectories(outputPath);
+
+    InputStream is = Files.newInputStream(maliciousZip);
+    IOException thrown =
+        assertThrows(
+            IOException.class,
+            () -> ExtractUtil.extractArchive(is, outputPath, ExtractType.ZIP, null));
+
+    assertThat(thrown).hasMessageThat().contains("Zip entry is outside of the target dir");
+  }
 }