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..a0f14497300 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("Invalid archive entry, contains traversal elements: " + name); + } + Path expectedOutputPath = destination.resolve(name).toAbsolutePath().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..4bba245acd6 100644 --- a/api/src/test/java/ai/djl/util/ZipUtilsTest.java +++ b/api/src/test/java/ai/djl/util/ZipUtilsTest.java @@ -47,15 +47,40 @@ public void testEmptyZipFile() throws IOException { @Test public void testOffendingTar() throws IOException { - Path path = Paths.get("src/test/resources/offending.tar"); + String[] offendingTars = + new String[] { + "src/test/resources/linux-created-offender-traversal-elements.tar", + "src/test/resources/windows-created-offender-traversal-elements.tar", + "src/test/resources/linux-created-offender-root.tar", + "src/test/resources/windows-created-offender-root.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); + for (String offendingTar : offendingTars) { + Path tarPath = Paths.get(offendingTar); + try (InputStream is = Files.newInputStream(tarPath)) { + Assert.assertThrows(() -> TarUtils.untar(is, output, false)); + } + } + } + + @Test + public void testOffendingZip() throws IOException { + String[] offendingTars = + new String[] { + "src/test/resources/linux-created-offender-traversal-elements.zip", + "src/test/resources/windows-created-offender-traversal-elements.zip", + "src/test/resources/linux-created-offender-root.zip", + "src/test/resources/windows-created-offender-root.zip", + }; + Path output = Paths.get("build/output"); + Files.createDirectories(output); + for (String offendingTar : offendingTars) { + Path tarPath = Paths.get(offendingTar); + try (InputStream is = Files.newInputStream(tarPath)) { + Assert.assertThrows(() -> ZipUtils.unzip(is, output)); + } } - Assert.assertTrue(Files.exists(file)); } @Test diff --git a/api/src/test/resources/linux-created-offender-root.tar b/api/src/test/resources/linux-created-offender-root.tar new file mode 100644 index 00000000000..9dd6643369b Binary files /dev/null and b/api/src/test/resources/linux-created-offender-root.tar differ diff --git a/api/src/test/resources/linux-created-offender-root.zip b/api/src/test/resources/linux-created-offender-root.zip new file mode 100644 index 00000000000..6582d0658ea Binary files /dev/null and b/api/src/test/resources/linux-created-offender-root.zip differ diff --git a/api/src/test/resources/linux-created-offender-traversal-elements.tar b/api/src/test/resources/linux-created-offender-traversal-elements.tar new file mode 100644 index 00000000000..e7ea4e6ab38 Binary files /dev/null and b/api/src/test/resources/linux-created-offender-traversal-elements.tar differ diff --git a/api/src/test/resources/linux-created-offender-traversal-elements.zip b/api/src/test/resources/linux-created-offender-traversal-elements.zip new file mode 100644 index 00000000000..0694b6c6850 Binary files /dev/null and b/api/src/test/resources/linux-created-offender-traversal-elements.zip differ diff --git a/api/src/test/resources/windows-created-offender-root.tar b/api/src/test/resources/windows-created-offender-root.tar new file mode 100644 index 00000000000..294c8846c19 Binary files /dev/null and b/api/src/test/resources/windows-created-offender-root.tar differ diff --git a/api/src/test/resources/windows-created-offender-root.zip b/api/src/test/resources/windows-created-offender-root.zip new file mode 100644 index 00000000000..5fe93df26b0 Binary files /dev/null and b/api/src/test/resources/windows-created-offender-root.zip differ diff --git a/api/src/test/resources/windows-created-offender-traversal-elements.tar b/api/src/test/resources/windows-created-offender-traversal-elements.tar new file mode 100644 index 00000000000..83f9a95c09b Binary files /dev/null and b/api/src/test/resources/windows-created-offender-traversal-elements.tar differ diff --git a/api/src/test/resources/windows-created-offender-traversal-elements.zip b/api/src/test/resources/windows-created-offender-traversal-elements.zip new file mode 100644 index 00000000000..3d7782901d5 Binary files /dev/null and b/api/src/test/resources/windows-created-offender-traversal-elements.zip differ diff --git a/integration/src/test/java/ai/djl/integration/IntegrationTests.java b/integration/src/test/java/ai/djl/integration/IntegrationTests.java index 23267808fd7..b1f4d991d9f 100644 --- 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 {