From 7d197ba8c3c63bb6cb20b25c18b13ca12cd3233b Mon Sep 17 00:00:00 2001 From: Siddharth Venkatesan Date: Tue, 26 Nov 2024 18:26:25 -0800 Subject: [PATCH] [api] fix issue in Tar/Zip Utils that resulted in incorrect artifact extraction (#3544) --- 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 | 37 +++++++++++++++--- .../resources/linux-created-offender-root.tar | Bin 0 -> 10240 bytes .../resources/linux-created-offender-root.zip | Bin 0 -> 142 bytes ...ux-created-offender-traversal-elements.tar | Bin 0 -> 10240 bytes ...ux-created-offender-traversal-elements.zip | Bin 0 -> 190 bytes .../windows-created-offender-root.tar | Bin 0 -> 10240 bytes .../windows-created-offender-root.zip | Bin 0 -> 142 bytes ...ws-created-offender-traversal-elements.tar | Bin 0 -> 10240 bytes ...ws-created-offender-traversal-elements.zip | Bin 0 -> 192 bytes .../ai/djl/integration/IntegrationTests.java | 2 +- 12 files changed, 49 insertions(+), 24 deletions(-) create mode 100644 api/src/test/resources/linux-created-offender-root.tar create mode 100644 api/src/test/resources/linux-created-offender-root.zip create mode 100644 api/src/test/resources/linux-created-offender-traversal-elements.tar create mode 100644 api/src/test/resources/linux-created-offender-traversal-elements.zip create mode 100644 api/src/test/resources/windows-created-offender-root.tar create mode 100644 api/src/test/resources/windows-created-offender-root.zip create mode 100644 api/src/test/resources/windows-created-offender-traversal-elements.tar create mode 100644 api/src/test/resources/windows-created-offender-traversal-elements.zip 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 0000000000000000000000000000000000000000..9dd6643369b9e42a9f7ba2c7d83b27d4d8c5557d GIT binary patch literal 10240 zcmeIyK?{N~7zW^-`xWcdO~1|I@(^_F*k4%rMG;o$6#Ms0LqpZ|8S1Sp{H@@fvWZU9 zY@1M&88VsDtECt$`8$2_ial38@~xlCp$m0t`ixr}^4*pG%6zz1#_G^0B^&aeC`Cg4 zB$Gq_t6QF5UBgod;h4BwrWXHq{{5^s4a*brLtfqa&iNX-|KHz{I4b+c=}xo3Pq(IYjRN_j6+95P$## sAOHafKmY;|fB*y_009U<00Izz00bZa0SG_<0uX=z1Rwwb2=q|k25SO2N&o-= literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..0694b6c68506c39db015d2007800443e6935a43a GIT binary patch literal 190 zcmWIWW@Zs#00IBbsz`$rp$i}$2&(|Go}NBdRFYeuUy@o}qE}K;Qkh?>UY4qml$x5S ykdc_2otmP^72wUtB*Ki_T%gflu%r=0VYLD%h-^!MH!B-REh7*H0qHOhhXDW(M@Ldd2)RpMziBj`kYi|*xNGmO5Gstef9{Q+)zsJ{qO^;=$Gg*dH$xG!@ zZEc>Vl;nm?uJmLq24nutP~NtItB`~y4CT;<+L|uo&c%GUr9Uzsua$8+HnQYm{!?Wa zLjEL^@BCM%qB#1R`w-$WaX9oX{_p(r`k2)$cgz=g^!qaxQ{?`CemzQ4Z;Lq{%FU>=F)KgJ~UBxry zGHq=>%}C~kOs@23CJa|l2H t0uX=z1Rwwb2tWV=5P$##AOHafKmY;|fB*y_009U<00Izz00agoa04iGI^F;P literal 0 HcmV?d00001 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 0000000000000000000000000000000000000000..3d7782901d5e0b8af9a42b61d7069510838e4090 GIT binary patch literal 192 zcmWIWW@Zs#00IBbsz`$rp$i}$2&)3Io?Z-Aq+gO-5L1#`T%uP}QBs*-s$Q0=kd&I5 zr;w4DoSmAY$Q9tt$Rxsy+hCyCV6dbSL}9f9D2QTBfHx}}NHHT21_SAE5QhN(hx#XL literal 0 HcmV?d00001 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 {