From 70d11b934a899e0d930d52dc5dd4db79669eeeb6 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 16 Aug 2022 16:04:02 +0200 Subject: [PATCH] Prevents unzipping files outside of the target folder --- .../contract/stubrunner/util/ZipCategory.java | 20 +++++++++++++----- .../stubrunner/util/ZipCategorySpec.groovy | 13 ++++++++++++ .../resources/zip/zip-malicious-traversal.zip | Bin 0 -> 545 bytes 3 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 spring-cloud-contract-stub-runner/src/test/resources/zip/zip-malicious-traversal.zip diff --git a/spring-cloud-contract-stub-runner/src/main/java/org/springframework/cloud/contract/stubrunner/util/ZipCategory.java b/spring-cloud-contract-stub-runner/src/main/java/org/springframework/cloud/contract/stubrunner/util/ZipCategory.java index dde85eaa75..7340e75f54 100644 --- a/spring-cloud-contract-stub-runner/src/main/java/org/springframework/cloud/contract/stubrunner/util/ZipCategory.java +++ b/spring-cloud-contract-stub-runner/src/main/java/org/springframework/cloud/contract/stubrunner/util/ZipCategory.java @@ -25,6 +25,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.zip.ZipEntry; +import java.util.zip.ZipException; import java.util.zip.ZipInputStream; import org.springframework.util.StreamUtils; @@ -65,14 +66,23 @@ public final class ZipCategory { try (ZipInputStream zipInput = new ZipInputStream(fileInputStream)) { for (ZipEntry entry = zipInput.getNextEntry(); entry != null; entry = zipInput.getNextEntry()) { if (!entry.isDirectory()) { - final File file = new File(destination, entry.getName()); - if (file.getParentFile() != null) { - file.getParentFile().mkdirs(); + final File destinationFile = new File(destination, entry.getName()); + /* + * If we see the relative traversal string of ".." we need to make sure + * that the outputdir + name doesn't leave the outputdir. + */ + String zipEntryName = entry.getName(); + if (!destinationFile.toPath().normalize().startsWith(destination.toPath())) { + throw new ZipException("The file " + zipEntryName + + " is trying to leave the target output directory of " + destination); } - try (OutputStream output = Files.newOutputStream(file.toPath())) { + if (destinationFile.getParentFile() != null) { + destinationFile.getParentFile().mkdirs(); + } + try (OutputStream output = Files.newOutputStream(destinationFile.toPath())) { StreamUtils.copy(zipInput, output); } - unzippedFiles.add(file); + unzippedFiles.add(destinationFile); } else { final File dir = new File(destination, entry.getName()); diff --git a/spring-cloud-contract-stub-runner/src/test/groovy/org/springframework/cloud/contract/stubrunner/util/ZipCategorySpec.groovy b/spring-cloud-contract-stub-runner/src/test/groovy/org/springframework/cloud/contract/stubrunner/util/ZipCategorySpec.groovy index 047a3ea4e4..9544779d7e 100644 --- a/spring-cloud-contract-stub-runner/src/test/groovy/org/springframework/cloud/contract/stubrunner/util/ZipCategorySpec.groovy +++ b/spring-cloud-contract-stub-runner/src/test/groovy/org/springframework/cloud/contract/stubrunner/util/ZipCategorySpec.groovy @@ -36,4 +36,17 @@ class ZipCategorySpec extends Specification { }?.text?.trim() == 'test' } + def 'should not allow malicious traversal'() throws Exception { + given: + File zipFile = new File(ZipCategorySpec.classLoader.getResource('zip/zip-malicious-traversal.zip').toURI()) + File tempDir = File.createTempDir() + tempDir.deleteOnExit() + when: + use(ZipCategory) { + zipFile.unzipTo(tempDir) + } + then: + Exception e = thrown() + e.getCause().getMessage().contains("is trying to leave the target output directory") + } } diff --git a/spring-cloud-contract-stub-runner/src/test/resources/zip/zip-malicious-traversal.zip b/spring-cloud-contract-stub-runner/src/test/resources/zip/zip-malicious-traversal.zip new file mode 100644 index 0000000000000000000000000000000000000000..38b3f499de0163e62ca15ce18350a9d9a477a51b GIT binary patch literal 545 zcmWIWW@h1H0D=Au{XYEp{-1?`Y!K#PkYPyA&ri`SsVE5z;bdU8U359h4v0%DxEUB( zzA-W|u!sQFm1JZVD*#cV0!Xz&eqJh90MJm76a&LlprHwl)s`S02)6*So}T`Ippx7I z{nWC|9FT|Lj?Pm62|-=W$Rx*%D=;L0E@xl>dYWNLBZ!3v8dgZqpan~SHzSh>Gwx6T jnE?Vz8bg8PfCLE8QsgiR@MdKLxrhk}K_2A>d6oeH^pk5C literal 0 HcmV?d00001