Skip to content

Commit

Permalink
[api] fix issue in Tar/Zip Utils that resulted in incorrect artifact …
Browse files Browse the repository at this point in the history
…extraction (#3544)
  • Loading branch information
siddvenk committed Dec 6, 2024
1 parent 503289a commit 7415cc5
Show file tree
Hide file tree
Showing 12 changed files with 49 additions and 24 deletions.
6 changes: 2 additions & 4 deletions api/src/main/java/ai/djl/util/TarUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
28 changes: 15 additions & 13 deletions api/src/main/java/ai/djl/util/ZipUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,10 @@ 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);
}
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 {
Expand Down Expand Up @@ -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 {
Expand Down
37 changes: 31 additions & 6 deletions api/src/test/java/ai/djl/util/ZipUtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 7415cc5

Please sign in to comment.