diff --git a/java/com/google/copybara/remotefile/extractutil/ExtractUtil.java b/java/com/google/copybara/remotefile/extractutil/ExtractUtil.java index 2c645252d..b09a6a9b2 100644 --- a/java/com/google/copybara/remotefile/extractutil/ExtractUtil.java +++ b/java/com/google/copybara/remotefile/extractutil/ExtractUtil.java @@ -14,7 +14,7 @@ * limitations under the License. */ package com.google.copybara.remotefile.extractutil; - +import java.nio.file.Path; import com.google.common.io.MoreFiles; import com.google.copybara.exception.ValidationException; import com.google.copybara.util.Glob; @@ -22,6 +22,7 @@ import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.PathMatcher; import javax.annotation.Nullable; import org.apache.commons.compress.archivers.ArchiveEntry; import org.apache.commons.compress.archivers.ArchiveInputStream; @@ -43,18 +44,26 @@ private ExtractUtil() {} public static void extractArchive( InputStream contents, Path targetPath, ExtractType type, @Nullable Glob fileFilter) throws IOException, ValidationException { + Path root = targetPath.toAbsolutePath().normalize(); + PathMatcher rootedFilter = fileFilter != null ? fileFilter.relativeTo(root) : null; + ArchiveEntry archiveEntry; try (ArchiveInputStream inputStream = createArchiveInputStream(contents, type)) { while (((archiveEntry = inputStream.getNextEntry()) != null)) { - if ((fileFilter != null - && !fileFilter - .relativeTo(targetPath.toAbsolutePath()) - .matches(targetPath.resolve(Path.of(archiveEntry.getName())))) + Path resolvedPath = root.resolve(archiveEntry.getName()).normalize(); + + // Security check: Prevent Zip Slip vulnerability + if (!resolvedPath.startsWith(root)) { + throw new IOException("Zip entry is outside of the target dir: " + archiveEntry.getName()); + } + + if ((rootedFilter != null && !rootedFilter.matches(resolvedPath)) || archiveEntry.isDirectory()) { continue; } - Files.createDirectories(targetPath.resolve(archiveEntry.getName()).getParent()); - MoreFiles.asByteSink(targetPath.resolve(archiveEntry.getName())).writeFrom(inputStream); + + Files.createDirectories(resolvedPath.getParent()); + MoreFiles.asByteSink(resolvedPath).writeFrom(inputStream); } } } diff --git a/javatests/com/google/copybara/remotefile/extractutil/ExtractUtilTest.java b/javatests/com/google/copybara/remotefile/extractutil/ExtractUtilTest.java index 41d455b8d..00c9b9a33 100644 --- a/javatests/com/google/copybara/remotefile/extractutil/ExtractUtilTest.java +++ b/javatests/com/google/copybara/remotefile/extractutil/ExtractUtilTest.java @@ -15,6 +15,7 @@ */ package com.google.copybara.remotefile.extractutil; +import static org.junit.Assert.assertThrows; import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; @@ -151,4 +152,26 @@ public void testExtractArchive_tarBz2File() throws Exception { assertThat(MoreFiles.asByteSource(outputPath.resolve("foo.txt")).asCharSource(UTF_8).read()) .isEqualTo("copybara"); } + + @Test + public void testExtractArchive_zipSlipVulnerability() throws Exception { + Path maliciousZip = testFolder.resolve("malicious.zip"); + try (ZipOutputStream zipOs = new ZipOutputStream(Files.newOutputStream(maliciousZip))) { + ZipEntry ze = new ZipEntry("../../evil.txt"); + zipOs.putNextEntry(ze); + zipOs.write("evil content".getBytes(UTF_8)); + zipOs.closeEntry(); + } + + Path outputPath = testFolder.resolve("output_safe"); + Files.createDirectories(outputPath); + + InputStream is = Files.newInputStream(maliciousZip); + IOException thrown = + assertThrows( + IOException.class, + () -> ExtractUtil.extractArchive(is, outputPath, ExtractType.ZIP, null)); + + assertThat(thrown).hasMessageThat().contains("Zip entry is outside of the target dir"); + } }