From ac401e7f543d44dba9797d9dca018377c98c5b3a Mon Sep 17 00:00:00 2001 From: Siddharth Venkatesan Date: Mon, 25 Nov 2024 17:20:18 -0800 Subject: [PATCH] [api] fix issue in Tar/Zip Utils that resulted in incorrect artifact extraction --- api/build.gradle.kts | 1 + api/src/main/java/ai/djl/util/TarUtils.java | 5 +-- api/src/main/java/ai/djl/util/ZipUtils.java | 29 ++++++++++++++---- .../test/java/ai/djl/util/ZipUtilsTest.java | 21 +++++++++++-- .../resources/linux_create_windows_use.tar | Bin 0 -> 10240 bytes .../ai/djl/integration/IntegrationTests.java | 2 +- .../java/ai/djl/testing/TestRequirements.java | 7 +++++ 7 files changed, 51 insertions(+), 14 deletions(-) create mode 100644 api/src/test/resources/linux_create_windows_use.tar diff --git a/api/build.gradle.kts b/api/build.gradle.kts index 025f546c994..9a6a259e39b 100644 --- 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 index d4a6e42b230..fedd1fb71d3 100644 --- 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 index 7c8c298a6cb..a5a77b28b1e 100644 --- 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 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 index 387715bbd44..d686eb6373f 100644 --- 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 new file mode 100644 index 0000000000000000000000000000000000000000..757e28c0044578abc877c2d081d35e23dc25ca70 GIT binary patch literal 10240 zcmeIvK@Ng25QX6$r6;g0wDce@TpMA*0!g5#+}>j1hM0EKL=*qnOkm11d{3SFYdhX& zbzJAr4Rxj3slRp8th#VfqDbjj-Z`=LSo{8ZXRNP`amM&KkKHyxx+1q{mMn8PPg06$ z97F18Zl3z~hi598f4dCBdI*oCO!n`#fE#P~xxb%3