From bc9da54e12efa3a0deb40cac3df28794fe5e94c8 Mon Sep 17 00:00:00 2001 From: Jonathan Wei Date: Tue, 12 Jun 2018 10:03:08 -0700 Subject: [PATCH] Fix Zip Slip vulnerability (#5850) * Fix evil zip exploit * PR comment, checkstyle * PR comments * Add link to vulnerability report * Fix test --- .../main/java/io/druid/indexer/JobHelper.java | 30 ++++++++---- .../hadoop/DatasourceRecordReader.java | 2 +- .../indexer/updater/HadoopConverterJob.java | 2 +- .../java/io/druid/indexer/JobHelperTest.java | 43 ++++++++++++++++ indexing-hadoop/src/test/resources/evil.zip | Bin 0 -> 545 bytes .../java/util/common/CompressionUtils.java | 36 ++++++++++++++ .../util/common/CompressionUtilsTest.java | 46 ++++++++++++++++++ 7 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 indexing-hadoop/src/test/resources/evil.zip diff --git a/indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java b/indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java index bbe4f2bc37e..70dbeaebe0f 100644 --- a/indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java +++ b/indexing-hadoop/src/main/java/io/druid/indexer/JobHelper.java @@ -25,6 +25,7 @@ import com.google.common.base.Strings; import com.google.common.base.Throwables; import com.google.common.io.Files; import io.druid.indexer.updater.HadoopDruidConverterConfig; +import io.druid.java.util.common.CompressionUtils; import io.druid.java.util.common.DateTimes; import io.druid.java.util.common.FileUtils; import io.druid.java.util.common.IAE; @@ -43,6 +44,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.LocalFileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.io.retry.RetryPolicies; +import org.apache.hadoop.io.retry.RetryPolicy; import org.apache.hadoop.io.retry.RetryProxy; import org.apache.hadoop.mapreduce.Job; import org.apache.hadoop.mapreduce.MRJobConfig; @@ -676,9 +678,21 @@ public class JobHelper final Path zip, final Configuration configuration, final File outDir, - final Progressable progressable + final Progressable progressable, + final RetryPolicy retryPolicy ) throws IOException { + final RetryPolicy effectiveRetryPolicy; + if (retryPolicy == null) { + effectiveRetryPolicy = RetryPolicies.exponentialBackoffRetry( + NUM_RETRIES, + SECONDS_BETWEEN_RETRIES, + TimeUnit.SECONDS + ); + } else { + effectiveRetryPolicy = retryPolicy; + } + final DataPusher zipPusher = (DataPusher) RetryProxy.create( DataPusher.class, new DataPusher() { @@ -693,13 +707,11 @@ public class JobHelper try (ZipInputStream in = new ZipInputStream(fileSystem.open(zip, 1 << 13))) { for (ZipEntry entry = in.getNextEntry(); entry != null; entry = in.getNextEntry()) { final String fileName = entry.getName(); - try (final OutputStream out = new BufferedOutputStream( - new FileOutputStream( - outDir.getAbsolutePath() - + File.separator - + fileName - ), 1 << 13 - )) { + final String outputPath = new File(outDir, fileName).getAbsolutePath(); + + CompressionUtils.validateZipOutputFile(zip.getName(), new File(outputPath), outDir); + + try (final OutputStream out = new BufferedOutputStream(new FileOutputStream(outputPath))) { for (int len = in.read(buffer); len >= 0; len = in.read(buffer)) { progressable.progress(); if (len == 0) { @@ -721,7 +733,7 @@ public class JobHelper } } }, - RetryPolicies.exponentialBackoffRetry(NUM_RETRIES, SECONDS_BETWEEN_RETRIES, TimeUnit.SECONDS) + effectiveRetryPolicy ); return zipPusher.push(); } diff --git a/indexing-hadoop/src/main/java/io/druid/indexer/hadoop/DatasourceRecordReader.java b/indexing-hadoop/src/main/java/io/druid/indexer/hadoop/DatasourceRecordReader.java index 12170be38dc..21ac6055f9a 100644 --- a/indexing-hadoop/src/main/java/io/druid/indexer/hadoop/DatasourceRecordReader.java +++ b/indexing-hadoop/src/main/java/io/druid/indexer/hadoop/DatasourceRecordReader.java @@ -90,7 +90,7 @@ public class DatasourceRecordReader extends RecordReader tmpSegmentDirs.add(dir); logger.info("Locally storing fetched segment at [%s]", dir); - JobHelper.unzipNoGuava(path, context.getConfiguration(), dir, context); + JobHelper.unzipNoGuava(path, context.getConfiguration(), dir, context, null); logger.info("finished fetching segment files"); QueryableIndex index = HadoopDruidIndexerConfig.INDEX_IO.loadIndex(dir); diff --git a/indexing-hadoop/src/main/java/io/druid/indexer/updater/HadoopConverterJob.java b/indexing-hadoop/src/main/java/io/druid/indexer/updater/HadoopConverterJob.java index 408d3a45ee1..5a91190035d 100644 --- a/indexing-hadoop/src/main/java/io/druid/indexer/updater/HadoopConverterJob.java +++ b/indexing-hadoop/src/main/java/io/druid/indexer/updater/HadoopConverterJob.java @@ -520,7 +520,7 @@ public class HadoopConverterJob log.warn("Unable to make directory"); } - final long inSize = JobHelper.unzipNoGuava(inPath, context.getConfiguration(), inDir, context); + final long inSize = JobHelper.unzipNoGuava(inPath, context.getConfiguration(), inDir, context, null); log.debug("Loaded %d bytes into [%s] for converting", inSize, inDir.getAbsolutePath()); context.getCounter(COUNTER_GROUP, COUNTER_LOADED).increment(inSize); diff --git a/indexing-hadoop/src/test/java/io/druid/indexer/JobHelperTest.java b/indexing-hadoop/src/test/java/io/druid/indexer/JobHelperTest.java index a7613249d05..4c27497cc2c 100644 --- a/indexing-hadoop/src/test/java/io/druid/indexer/JobHelperTest.java +++ b/indexing-hadoop/src/test/java/io/druid/indexer/JobHelperTest.java @@ -25,6 +25,8 @@ import io.druid.data.input.impl.CSVParseSpec; import io.druid.data.input.impl.DimensionsSpec; import io.druid.data.input.impl.StringInputRowParser; import io.druid.data.input.impl.TimestampSpec; +import io.druid.java.util.common.CompressionUtils; +import io.druid.java.util.common.ISE; import io.druid.java.util.common.Intervals; import io.druid.java.util.common.granularity.Granularities; import io.druid.query.aggregation.AggregatorFactory; @@ -34,7 +36,10 @@ import io.druid.segment.indexing.granularity.UniformGranularitySpec; import io.druid.timeline.DataSegment; import io.druid.timeline.partition.NoneShardSpec; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.io.retry.RetryPolicies; import org.apache.hadoop.mapreduce.Job; +import org.apache.hadoop.util.Progressable; import org.joda.time.Interval; import org.junit.Assert; import org.junit.Before; @@ -43,8 +48,10 @@ import org.junit.Test; import org.junit.rules.TemporaryFolder; import java.io.File; +import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import java.nio.file.Files; import java.util.HashMap; import java.util.Map; @@ -178,6 +185,42 @@ public class JobHelperTest ); } + @Test + public void testEvilZip() throws IOException + { + final File tmpDir = temporaryFolder.newFolder("testEvilZip"); + + final File evilResult = new File("/tmp/evil.txt"); + Files.deleteIfExists(evilResult.toPath()); + + File evilZip = new File(tmpDir, "evil.zip"); + Files.deleteIfExists(evilZip.toPath()); + CompressionUtils.makeEvilZip(evilZip); + + try { + JobHelper.unzipNoGuava( + new Path(evilZip.getCanonicalPath()), + new Configuration(), + tmpDir, + new Progressable() + { + @Override + public void progress() + { + + } + }, + RetryPolicies.TRY_ONCE_THEN_FAIL + ); + } + catch (ISE ise) { + Assert.assertTrue(ise.getMessage().contains("does not start with outDir")); + Assert.assertFalse("Zip exploit triggered, /tmp/evil.txt was written.", evilResult.exists()); + return; + } + Assert.fail("Exception was not thrown for malicious zip file"); + } + private static class HadoopDruidIndexerConfigSpy extends HadoopDruidIndexerConfig { diff --git a/indexing-hadoop/src/test/resources/evil.zip b/indexing-hadoop/src/test/resources/evil.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 diff --git a/java-util/src/main/java/io/druid/java/util/common/CompressionUtils.java b/java-util/src/main/java/io/druid/java/util/common/CompressionUtils.java index 9ee57459f56..5df98df80c6 100644 --- a/java-util/src/main/java/io/druid/java/util/common/CompressionUtils.java +++ b/java-util/src/main/java/io/druid/java/util/common/CompressionUtils.java @@ -232,6 +232,9 @@ public class CompressionUtils while (enumeration.hasMoreElements()) { final ZipEntry entry = enumeration.nextElement(); final File outFile = new File(outDir, entry.getName()); + + validateZipOutputFile(pulledFile.getCanonicalPath(), outFile, outDir); + result.addFiles( FileUtils.retryCopy( new ByteSource() @@ -252,6 +255,25 @@ public class CompressionUtils return result; } + public static void validateZipOutputFile( + String sourceFilename, + final File outFile, + final File outDir + ) throws IOException + { + // check for evil zip exploit that allows writing output to arbitrary directories + final File canonicalOutFile = outFile.getCanonicalFile(); + final String canonicalOutDir = outDir.getCanonicalPath(); + if (!canonicalOutFile.toPath().startsWith(canonicalOutDir)) { + throw new ISE( + "Unzipped output path[%s] of sourceFile[%s] does not start with outDir[%s].", + canonicalOutFile, + sourceFilename, + canonicalOutDir + ); + } + } + /** * Unzip from the input stream to the output directory, using the entry's file name as the file name in the output directory. * The behavior of directories in the input stream's zip is undefined. @@ -272,6 +294,8 @@ public class CompressionUtils while ((entry = zipIn.getNextEntry()) != null) { final File file = new File(outDir, entry.getName()); + validateZipOutputFile("", file, outDir); + NativeIO.chunkedCopy(zipIn, file); result.addFile(file); @@ -562,4 +586,16 @@ public class CompressionUtils return in; } } + + // Helper method for unit tests (for checking that we fixed https://snyk.io/research/zip-slip-vulnerability) + public static void makeEvilZip(File outputFile) throws IOException + { + ZipOutputStream zipOutputStream = new ZipOutputStream(new FileOutputStream(outputFile)); + ZipEntry zipEntry = new ZipEntry("../../../../../../../../../../../../../../../tmp/evil.txt"); + zipOutputStream.putNextEntry(zipEntry); + byte[] output = StringUtils.toUtf8("evil text"); + zipOutputStream.write(output); + zipOutputStream.closeEntry(); + zipOutputStream.close(); + } } diff --git a/java-util/src/test/java/io/druid/java/util/common/CompressionUtilsTest.java b/java-util/src/test/java/io/druid/java/util/common/CompressionUtilsTest.java index d8f878b9ed7..bbb49b9f51f 100644 --- a/java-util/src/test/java/io/druid/java/util/common/CompressionUtilsTest.java +++ b/java-util/src/test/java/io/druid/java/util/common/CompressionUtilsTest.java @@ -313,6 +313,52 @@ public class CompressionUtilsTest } } + @Test + public void testEvilZip() throws IOException + { + final File tmpDir = temporaryFolder.newFolder("testEvilZip"); + + final File evilResult = new File("/tmp/evil.txt"); + java.nio.file.Files.deleteIfExists(evilResult.toPath()); + + File evilZip = new File(tmpDir, "evil.zip"); + java.nio.file.Files.deleteIfExists(evilZip.toPath()); + CompressionUtils.makeEvilZip(evilZip); + + try { + CompressionUtils.unzip(evilZip, tmpDir); + } + catch (ISE ise) { + Assert.assertTrue(ise.getMessage().contains("does not start with outDir")); + Assert.assertFalse("Zip exploit triggered, /tmp/evil.txt was written.", evilResult.exists()); + return; + } + Assert.fail("Exception was not thrown for malicious zip file"); + } + + @Test + public void testEvilZipInputStream() throws IOException + { + final File tmpDir = temporaryFolder.newFolder("testEvilZip"); + + final File evilResult = new File("/tmp/evil.txt"); + java.nio.file.Files.deleteIfExists(evilResult.toPath()); + + File evilZip = new File(tmpDir, "evil.zip"); + java.nio.file.Files.deleteIfExists(evilZip.toPath()); + CompressionUtils.makeEvilZip(evilZip); + + try { + CompressionUtils.unzip(new FileInputStream(evilZip), tmpDir); + } + catch (ISE ise) { + Assert.assertTrue(ise.getMessage().contains("does not start with outDir")); + Assert.assertFalse("Zip exploit triggered, /tmp/evil.txt was written.", evilResult.exists()); + return; + } + Assert.fail("Exception was not thrown for malicious zip file"); + } + @Test // Sanity check to make sure the test class works as expected public void testZeroRemainingInputStream() throws IOException