Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[api] fix issue in Tar/Zip Utils that resulted in incorrect artifact … #3544

Merged
merged 1 commit into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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();
Dismissed Show dismissed Hide dismissed
validateArchiveEntry(entry.getName(), dest);
set.add(entryName);
Path file = dest.resolve(entryName).toAbsolutePath();
siddvenk marked this conversation as resolved.
Show resolved Hide resolved
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())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd better block ".." as well in this method. it prevent file overwrite inside the destination folder. In original version, we already blocking "..", and nobody complained about it.

Copy link
Contributor Author

@siddvenk siddvenk Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default behavior of extracting a tar is to overwrite isn't it? I'm not sure why we need to differ.

If an archive mytar.tar had (in order)

b.txt
a/../b.txt

a/../b.txt would overwrite b.txt (e.g. using tar -xvf mytar.tar)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and added it back to keep in line with what we had before, but curious to know what you think about my point above.

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
Loading