This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit f113ec65e200d6f551a1997ff50ab5c853d3cd55 Author: Juan Cabrerizo <[email protected]> AuthorDate: Wed Nov 30 10:52:58 2022 +0000 address PR comments II --- .../brooklyn/util/core/file/ArchiveUtils.java | 33 ++++++++++----------- .../brooklyn/util/core/file/ArchiveUtilsTest.java | 14 ++++----- .../resources/brooklyn/util/file.core/README.md | 8 +++++ .../brooklyn/util/file.core/evilLinux.zip | Bin 326 -> 174 bytes 4 files changed, 30 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/util/core/file/ArchiveUtils.java b/core/src/main/java/org/apache/brooklyn/util/core/file/ArchiveUtils.java index 1a374d28e2..38876f6eed 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/file/ArchiveUtils.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/file/ArchiveUtils.java @@ -366,35 +366,32 @@ public class ArchiveUtils { File targetPath = new File(targetFolder); targetPath.mkdir(); Enumeration<? extends ZipEntry> zipFileEntries = zip.entries(); - while (zipFileEntries.hasMoreElements()) { - ZipEntry entry = zipFileEntries.nextElement(); - String originalName = entry.getName(); - File destFile = new File(targetPath, originalName); - // validate input path - try { - String canonicalDestinationDirPath = null; + try { + String canonicalDestinationDirPath = targetPath.getCanonicalPath(); + while (zipFileEntries.hasMoreElements()) { + ZipEntry entry = zipFileEntries.nextElement(); + String originalName = entry.getName(); + File destFile = new File(targetPath, originalName); + // Validate input path for avoiding to extract files outside `targetFolder` // enforce the file uses the appropriate file separator for the controller SO String sanitizedName = originalName .replace("\\", File.separator) .replace("/", File.separator); File sanitizedDestFile = new File(targetPath, sanitizedName); - canonicalDestinationDirPath = targetPath.getCanonicalPath(); String canonicalDestinationFile = sanitizedDestFile.getCanonicalPath(); if (!canonicalDestinationFile.startsWith(canonicalDestinationDirPath + File.separator)) { throw new IllegalStateException("Entry is outside of the target dir: " + entry.getName()); } - } catch (IOException e) { - throw Exceptions.propagate(e); - } - destFile.getParentFile().mkdirs(); - if (!entry.isDirectory()) { - try (InputStream in = zip.getInputStream(entry); OutputStream out = new FileOutputStream(destFile)) { - Streams.copy(in, out); - } - catch (IOException e) { - throw Exceptions.propagate(e); + + destFile.getParentFile().mkdirs(); + if (!entry.isDirectory()) { + try (InputStream in = zip.getInputStream(entry); OutputStream out = new FileOutputStream(destFile)) { + Streams.copy(in, out); + } } } + } catch (IOException e) { + throw Exceptions.propagate(e); } } } diff --git a/core/src/test/java/org/apache/brooklyn/util/core/file/ArchiveUtilsTest.java b/core/src/test/java/org/apache/brooklyn/util/core/file/ArchiveUtilsTest.java index c6a5f3f36a..a3eba8b3ae 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/file/ArchiveUtilsTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/file/ArchiveUtilsTest.java @@ -30,6 +30,7 @@ import java.util.zip.ZipFile; import com.google.common.io.ByteStreams; import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.exceptions.Exceptions; import org.testng.annotations.AfterClass; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeClass; @@ -114,16 +115,15 @@ public class ArchiveUtilsTest extends BrooklynAppUnitTestSupport { assertFilesEqual(new File(destDir, destFile), origJar); } @Test(groups="Integration") - public void testUnzipFileAccessingPathOutsideTargetFolderEvilWinFormat() throws Exception{ - InputStream evilZip = ResourceUtils.create(this).getResourceFromUrl("classpath://brooklyn/util/file.core/evilWin.zip"); + public void testUnzipFileAccessingPathOutsideTargetFolderEvilWinFormat() { Asserts.assertFailsWith(() -> doTestUnzip("classpath://brooklyn/util/file.core/evilWin.zip"), e -> { Asserts.expectedFailureContainsIgnoreCase(e, "Entry is outside of the target dir"); return true; }); } @Test(groups="Integration") - public void testUnzipFileAccessingPathOutsideTargetFolderEvilLinuxFormat() throws Exception{ + public void testUnzipFileAccessingPathOutsideTargetFolderEvilLinuxFormat() { Asserts.assertFailsWith(() -> doTestUnzip("classpath://brooklyn/util/file.core/evilLinux.zip"), e -> { Asserts.expectedFailureContainsIgnoreCase(e, "Entry is outside of the target dir"); return true; }); } @Test(groups="Integration") - public void testUnzipFileAccessingPathOutsideTargetFolderNoEvil() throws Exception{ + public void testUnzipFileAccessingPathOutsideTargetFolderNoEvil() { doTestUnzip("classpath://brooklyn/util/file.core/noEvil.zip"); } @@ -152,14 +152,14 @@ public class ArchiveUtilsTest extends BrooklynAppUnitTestSupport { private void doTestUnzip(String url) { File tempZipFile = null; - InputStream evilZip = ResourceUtils.create(this).getResourceFromUrl(url); + InputStream zip = ResourceUtils.create(this).getResourceFromUrl(url); try { tempZipFile = File.createTempFile("test", "zip"); tempZipFile.deleteOnExit(); - java.nio.file.Files.write(tempZipFile.toPath(), ByteStreams.toByteArray(evilZip), StandardOpenOption.TRUNCATE_EXISTING); + java.nio.file.Files.write(tempZipFile.toPath(), ByteStreams.toByteArray(zip), StandardOpenOption.TRUNCATE_EXISTING); ArchiveUtils.extractZip(new ZipFile(tempZipFile), destDir.getAbsolutePath()); } catch (Exception e) { - throw new RuntimeException(e); + throw Exceptions.propagate(e); } finally { tempZipFile.delete(); } diff --git a/core/src/test/resources/brooklyn/util/file.core/README.md b/core/src/test/resources/brooklyn/util/file.core/README.md new file mode 100644 index 0000000000..0455d1f0cc --- /dev/null +++ b/core/src/test/resources/brooklyn/util/file.core/README.md @@ -0,0 +1,8 @@ +# Evil zip files objective + +The `evil*.zip` files in this directory contain a script, and they have been creating using a [pen test tool](https://github.com/jcabrerizo/evilarc) +that allows prefixing the name of the file, the script in this example, with a number of `../` making the default +extraction of them a risk for the underlying operating system, allowing to put any file in any path, or replacing +exiting binary files, exposing the host to a remote command execution. + +This vulnerability is named *Zip Slip*, you can find more information in here: https://security.snyk.io/research/zip-slip-vulnerability diff --git a/core/src/test/resources/brooklyn/util/file.core/evilLinux.zip b/core/src/test/resources/brooklyn/util/file.core/evilLinux.zip index 01d929ab67..bd95632453 100644 Binary files a/core/src/test/resources/brooklyn/util/file.core/evilLinux.zip and b/core/src/test/resources/brooklyn/util/file.core/evilLinux.zip differ
