[api] fix issue in Tar/Zip Utils that resulted in incorrect artifact …

…extraction
deepjavalibrary · Nov 26, 2024 · ac401e7 · ac401e7
1 parent 73f7cb0
commit ac401e7
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 14 deletions.
diff --git a/api/build.gradle.kts b/api/build.gradle.kts
@@ -15,6 +15,7 @@ dependencies {
         exclude("junit", "junit")
     }
     testImplementation(libs.slf4j.simple)
+    testImplementation(project(":testing"))
     testRuntimeOnly(project(":engines:pytorch:pytorch-model-zoo"))
     testRuntimeOnly(project(":engines:pytorch:pytorch-jni"))
 }

diff --git a/api/src/main/java/ai/djl/util/TarUtils.java b/api/src/main/java/ai/djl/util/TarUtils.java
@@ -48,10 +48,7 @@ public static void untar(InputStream is, Path dir, boolean gzip) throws IOExcept
         try (TarArchiveInputStream tis = new TarArchiveInputStream(bis)) {
             TarArchiveEntry entry;
             while ((entry = tis.getNextEntry()) != null) {
-                String entryName = ZipUtils.removeLeadingFileSeparator(entry.getName());
-                if (entryName.contains("..")) {
-                    throw new IOException("Malicious zip entry: " + entryName);
-                }
+                String entryName = ZipUtils.sanitizeAndValidateArchiveEntry(entry.getName(), dir);
                 Path file = dir.resolve(entryName).toAbsolutePath();
                 if (entry.isDirectory()) {
                     Files.createDirectories(file);

diff --git a/api/src/main/java/ai/djl/util/ZipUtils.java b/api/src/main/java/ai/djl/util/ZipUtils.java
@@ -12,6 +12,8 @@
  */
 package ai.djl.util;
 
+import org.apache.commons.io.FilenameUtils;
+
 import java.io.BufferedOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
@@ -52,10 +54,7 @@ public static void unzip(InputStream is, Path dest) throws IOException {
         ZipEntry entry;
         Set<String> set = new HashSet<>();
         while ((entry = zis.getNextEntry()) != null) {
-            String name = removeLeadingFileSeparator(entry.getName());
-            if (name.contains("..")) {
-                throw new IOException("Malicious zip entry: " + name);
-            }
+            String name = sanitizeAndValidateArchiveEntry(entry.getName(), dest);
             set.add(name);
             Path file = dest.resolve(name).toAbsolutePath();
             if (entry.isDirectory()) {
@@ -121,13 +120,31 @@ private static void addToZip(Path root, Path file, ZipOutputStream zos) throws I
         }
     }
 
+    static String sanitizeAndValidateArchiveEntry(String name, Path destination)
+            throws IOException {
+        String sanitizedName = removeLeadingFileSeparator(name);
+        Path expectedOutputPath = destination.resolve(sanitizedName).normalize();
+        if (!expectedOutputPath.startsWith(destination.normalize())) {
+            throw new IOException(
+                    "Bad archive entry "
+                            + name
+                            + ". Attempted write outside destination "
+                            + destination);
+        }
+        return sanitizedName;
+    }
+
     static String removeLeadingFileSeparator(String name) {
+        String osAwareArchiveEntryName = FilenameUtils.separatorsToSystem(name);
         int index = 0;
-        for (; index < name.length(); index++) {
-            if (name.charAt(index) != File.separatorChar) {
+        for (; index < osAwareArchiveEntryName.length(); index++) {
+            if (osAwareArchiveEntryName.charAt(index) != File.separatorChar) {
                 break;
             }
         }
+        // We return the substring of the original entry here because for zip archives
+        // we validate this entry against the header and do not want to modify the
+        // separator char as that can cause false positive issues in ValidationInputStream logic
         return name.substring(index);
     }
 

diff --git a/api/src/test/java/ai/djl/util/ZipUtilsTest.java b/api/src/test/java/ai/djl/util/ZipUtilsTest.java
@@ -12,6 +12,8 @@
  */
 package ai.djl.util;
 
+import ai.djl.testing.TestRequirements;
+
 import org.apache.commons.compress.archivers.zip.Zip64Mode;
 import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
 import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream;
@@ -49,13 +51,26 @@ public void testEmptyZipFile() throws IOException {
     public void testOffendingTar() throws IOException {
         Path path = Paths.get("src/test/resources/offending.tar");
         Path output = Paths.get("build/output");
-        Path file = output.resolve("tmp/empty.txt");
-        Utils.deleteQuietly(file);
         Files.createDirectories(output);
+        Path outputFile = output.resolve("tmp/empty.txt");
+        Utils.deleteQuietly(outputFile);
         try (InputStream is = Files.newInputStream(path)) {
             TarUtils.untar(is, output, false);
         }
-        Assert.assertTrue(Files.exists(file));
+        Assert.assertTrue(Files.exists(outputFile));
+    }
+
+    @Test
+    public void testLinuxCreatedWindowsUsedOffendingTar() throws IOException {
+        TestRequirements.windows();
+        Path tarPath = Paths.get("src/test/resources/linux_create_windows_use.tar");
+        Path output = Paths.get("build/output");
+        Files.createDirectories(output);
+        Path outputFile = output.resolve("Windows/System32/drivers/etc/dummy.txt");
+        try (InputStream is = Files.newInputStream(tarPath)) {
+            TarUtils.untar(is, output, false);
+        }
+        Assert.assertTrue(Files.exists(outputFile));
     }
 
     @Test

diff --git a/api/src/test/resources/linux_create_windows_use.tar b/api/src/test/resources/linux_create_windows_use.tar
diff --git a/integration/src/test/java/ai/djl/integration/IntegrationTests.java b/integration/src/test/java/ai/djl/integration/IntegrationTests.java
@@ -38,7 +38,7 @@ public void runIntegrationTests() {
             // TODO: windows CPU build is having OOM issue if 3 engines are loaded and running tests
             // together
             if (System.getProperty("os.name").startsWith("Win")) {
-                engines.add("MXNet");
+                engines.add("PyTorch");
             } else if ("aarch64".equals(System.getProperty("os.arch"))) {
                 engines.add("PyTorch");
             } else {

diff --git a/testing/src/main/java/ai/djl/testing/TestRequirements.java b/testing/src/main/java/ai/djl/testing/TestRequirements.java
@@ -96,6 +96,13 @@ public static void notWindows() {
         }
     }
 
+    /** Requires that the test runs on windows, not OSX or linux. */
+    public static void windows() {
+        if (!System.getProperty("os.name").toLowerCase().startsWith("win")) {
+            throw new SkipException("This test requires windows");
+        }
+    }
+
     /** Requires that the test runs on x86_64 arch. */
     public static void notArm() {
         if ("aarch64".equals(System.getProperty("os.arch"))) {