From d543ffe565ceb2ff22622b24fb659910dbc9e028 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 | 6 ++-- api/src/main/java/ai/djl/util/ZipUtils.java | 28 ++++++++++-------- .../test/java/ai/djl/util/ZipUtilsTest.java | 18 ++++++++--- .../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, 40 insertions(+), 22 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..280e94b0092 100644 --- a/api/src/main/java/ai/djl/util/TarUtils.java +++ b/api/src/main/java/ai/djl/util/TarUtils.java @@ -48,10 +48,8 @@ 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 = entry.getName(); + ZipUtils.validateArchiveEntry(entryName, 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..45d8744578b 100644 --- a/api/src/main/java/ai/djl/util/ZipUtils.java +++ b/api/src/main/java/ai/djl/util/ZipUtils.java @@ -52,12 +52,10 @@ 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); - } - set.add(name); - Path file = dest.resolve(name).toAbsolutePath(); + String entryName = entry.getName(); + validateArchiveEntry(entry.getName(), dest); + set.add(entryName); + Path file = dest.resolve(entryName).toAbsolutePath(); if (entry.isDirectory()) { Files.createDirectories(file); } else { @@ -121,14 +119,18 @@ private static void addToZip(Path root, Path file, ZipOutputStream zos) throws I } } - static String removeLeadingFileSeparator(String name) { - int index = 0; - for (; index < name.length(); index++) { - if (name.charAt(index) != File.separatorChar) { - break; - } + static void validateArchiveEntry(String name, Path destination) throws IOException { + if (name.contains("..")) { + throw new IOException("Bad archive entry, traversal element is not allowed: " + name); + } + Path expectedOutputPath = destination.resolve(name).normalize(); + if (!expectedOutputPath.startsWith(destination.normalize())) { + throw new IOException( + "Bad archive entry " + + name + + ". Attempted write outside destination " + + destination); } - return name.substring(index); } private static final class ValidationInputStream extends FilterInputStream { diff --git a/api/src/test/java/ai/djl/util/ZipUtilsTest.java b/api/src/test/java/ai/djl/util/ZipUtilsTest.java index 387715bbd44..21cd94d51be 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,21 @@ 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); try (InputStream is = Files.newInputStream(path)) { - TarUtils.untar(is, output, false); + Assert.assertThrows(() -> TarUtils.untar(is, output, false)); + } + } + + @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); + try (InputStream is = Files.newInputStream(tarPath)) { + Assert.assertThrows(() -> TarUtils.untar(is, output, false)); } - Assert.assertTrue(Files.exists(file)); } @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