From a6a37bd0f012a4ccfca3760418bf53ca5da65a4c Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Thu, 10 Aug 2023 09:27:26 +0000 Subject: [PATCH] add check for number of files inside zip git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1911588 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/openxml4j/opc/ZipPackage.java | 9 +- .../util/ZipArchiveThresholdInputStream.java | 15 +-- .../poi/openxml4j/util/ZipSecureFile.java | 67 ++++++++++++ .../openxml4j/TestOPCPackageFileLimit.java | 64 +++++++++++ .../poi/openxml4j/util/TestZipSecureFile.java | 21 ++++ .../apache/poi/xssf/TestXSSFFileChecks.java | 100 ++++++++++++++++++ poi-ooxml/src/test/java9/module-info.java | 1 + 7 files changed, 270 insertions(+), 7 deletions(-) create mode 100644 poi-ooxml/src/test/java/org/apache/poi/openxml4j/TestOPCPackageFileLimit.java create mode 100644 poi-ooxml/src/test/java/org/apache/poi/xssf/TestXSSFFileChecks.java diff --git a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/ZipPackage.java b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/ZipPackage.java index 5c7d798196..cc714e9465 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/ZipPackage.java +++ b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/ZipPackage.java @@ -29,6 +29,7 @@ import java.io.OutputStream; import java.util.Collections; import java.util.Enumeration; import java.util.List; +import java.util.Locale; import java.util.stream.Collectors; import org.apache.commons.compress.archivers.zip.ZipArchiveEntry; @@ -50,6 +51,7 @@ import org.apache.poi.openxml4j.util.ZipArchiveThresholdInputStream; import org.apache.poi.openxml4j.util.ZipEntrySource; import org.apache.poi.openxml4j.util.ZipFileZipEntrySource; import org.apache.poi.openxml4j.util.ZipInputStreamZipEntrySource; +import org.apache.poi.openxml4j.util.ZipSecureFile; import org.apache.poi.util.IOUtils; import org.apache.poi.util.TempFile; @@ -308,8 +310,13 @@ public final class ZipPackage extends OPCPackage { // (Need to create relationships before other // parts, otherwise we might create a part before // its relationship exists, and then it won't tie up) + final List list = Collections.list(zipEntries); + if (list.size() > ZipSecureFile.getMaxFileCount()) { + throw new InvalidFormatException(String.format( + Locale.ROOT, ZipSecureFile.MAX_FILE_COUNT_MSG, ZipSecureFile.getMaxFileCount())); + } final List entries = - Collections.list(zipEntries).stream() + list.stream() .filter(zipArchiveEntry -> !ignoreEntry(zipArchiveEntry)) .map(zae -> new EntryTriple(zae, contentTypeManager)) .filter(mm -> mm.partName != null) diff --git a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveThresholdInputStream.java b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveThresholdInputStream.java index 97ed930c37..75e896e78b 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveThresholdInputStream.java +++ b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveThresholdInputStream.java @@ -17,9 +17,6 @@ package org.apache.poi.openxml4j.util; -import static org.apache.poi.openxml4j.util.ZipSecureFile.MAX_ENTRY_SIZE; -import static org.apache.poi.openxml4j.util.ZipSecureFile.MIN_INFLATE_RATIO; - import java.io.EOFException; import java.io.FilterInputStream; import java.io.IOException; @@ -34,10 +31,10 @@ import org.apache.poi.openxml4j.exceptions.NotOfficeXmlFileException; import org.apache.poi.util.IOUtils; import org.apache.poi.util.Internal; +import static org.apache.poi.openxml4j.util.ZipSecureFile.*; + @Internal public class ZipArchiveThresholdInputStream extends FilterInputStream { - // don't alert for expanded sizes smaller than 100k - private static final long GRACE_ENTRY_SIZE = 100*1024L; private static final String MAX_ENTRY_SIZE_MSG = "Zip bomb detected! The file would exceed the max size of the expanded data in the zip-file.\n" + @@ -58,6 +55,7 @@ public class ZipArchiveThresholdInputStream extends FilterInputStream { */ private ZipArchiveEntry entry; private boolean guardState = true; + private long entryCount; public ZipArchiveThresholdInputStream(InputStream is) { super(is); @@ -125,7 +123,7 @@ public class ZipArchiveThresholdInputStream extends FilterInputStream { final String entryName = entry == null ? "not set" : entry.getName(); // check the file size first, in case we are working on uncompressed streams - if(payloadSize > MAX_ENTRY_SIZE) { + if (payloadSize > MAX_ENTRY_SIZE) { throw new IOException(String.format(Locale.ROOT, MAX_ENTRY_SIZE_MSG, payloadSize, rawSize, MAX_ENTRY_SIZE, entryName)); } @@ -150,6 +148,11 @@ public class ZipArchiveThresholdInputStream extends FilterInputStream { try { entry = ((ZipArchiveInputStream) in).getNextZipEntry(); + if (guardState && entry != null) { + if (++entryCount > MAX_FILE_COUNT) { + throw new IOException(String.format(Locale.ROOT, MAX_FILE_COUNT_MSG, MAX_FILE_COUNT)); + } + } return entry; } catch (ZipException ze) { if (ze.getMessage().startsWith("Unexpected record signature")) { diff --git a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipSecureFile.java b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipSecureFile.java index 8c0bb32899..18509302d1 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipSecureFile.java +++ b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipSecureFile.java @@ -37,12 +37,23 @@ public class ZipSecureFile extends ZipFile { private static final Logger LOG = LogManager.getLogger(ZipSecureFile.class); /* package */ static double MIN_INFLATE_RATIO = 0.01d; /* package */ static final long DEFAULT_MAX_ENTRY_SIZE = 0xFFFFFFFFL; + /* package */ static final long DEFAULT_MAX_FILE_COUNT = 1000; + /* package */ static final long DEFAULT_GRACE_ENTRY_SIZE = 100*1024L; /* package */ static long MAX_ENTRY_SIZE = DEFAULT_MAX_ENTRY_SIZE; + /* package */ static long MAX_FILE_COUNT = DEFAULT_MAX_FILE_COUNT; + /* package */ static long GRACE_ENTRY_SIZE = DEFAULT_GRACE_ENTRY_SIZE; // The maximum chars of extracted text /* package */ static final long DEFAULT_MAX_TEXT_SIZE = 10*1024*1024L; private static long MAX_TEXT_SIZE = DEFAULT_MAX_TEXT_SIZE; + public static final String MAX_FILE_COUNT_MSG = + "The file appears to be potentially malicious. This file embeds more internal file entries than expected.\n" + + "This may indicates that the file could pose a security risk.\n" + + "You can adjust this limit via ZipSecureFile.setMaxFileCount() if you need to work with files which are very large.\n" + + "Limits: MAX_FILE_COUNT: %d"; + + private final String fileName; /** @@ -68,6 +79,29 @@ public class ZipSecureFile extends ZipFile { return MIN_INFLATE_RATIO; } + /** + * Returns the current maximum file count that is used. + * + * See setMaxFileCount() for details. + * + * @return The max accepted file count (i.e. the max number of files we allow inside zip files that we read - including OOXML files like xlsx, docx, pptx, etc.). + * @since POI 5.2.4 + */ + public static long getMaxFileCount() { + return MAX_FILE_COUNT; + } + + /** + * Sets the maximum file count that we allow inside zip files that we read - + * including OOXML files like xlsx, docx, pptx, etc. The default is 1000. + * + * @param maxFileCount The max accepted file count + * @since POI 5.2.4 + */ + public static void setMaxFileCount(final long maxFileCount) { + MAX_FILE_COUNT = maxFileCount; + } + /** * Sets the maximum file size of a single zip entry. It defaults to 4GB, * i.e. the 32-bit zip format maximum. @@ -98,6 +132,39 @@ public class ZipSecureFile extends ZipFile { return MAX_ENTRY_SIZE; } + /** + * Sets the grace entry size of a single zip entry. It defaults to 100Kb. + * + * When decompressed data in a zip entry is smaller than this size, the + * Minimum Inflation Ratio check is ignored. + * + * Setting this to a very small value may lead to more files being flagged + * as potential Zip Bombs are rejected as a result. + * + * @param graceEntrySize the grace entry size of a single zip entry + * @throws IllegalArgumentException for negative graceEntrySize + * @since POI 5.2.4 + */ + public static void setGraceEntrySize(long graceEntrySize) { + if (graceEntrySize < 0) { + throw new IllegalArgumentException("Grace entry size must be greater than or equal to zero"); + } + GRACE_ENTRY_SIZE = graceEntrySize; + } + + /** + * Returns the current threshold for decompressed data in zip entries that are regarded as too small + * to worry about from a Zip Bomb perspective (default is 100Kb). + * + * See setGraceEntrySize() for details. + * + * @return The current grace entry size + * @since POI 5.2.4 + */ + public static long getGraceEntrySize() { + return GRACE_ENTRY_SIZE; + } + /** * Sets the maximum number of characters of text that are * extracted before an exception is thrown during extracting diff --git a/poi-ooxml/src/test/java/org/apache/poi/openxml4j/TestOPCPackageFileLimit.java b/poi-ooxml/src/test/java/org/apache/poi/openxml4j/TestOPCPackageFileLimit.java new file mode 100644 index 0000000000..017de0a10e --- /dev/null +++ b/poi-ooxml/src/test/java/org/apache/poi/openxml4j/TestOPCPackageFileLimit.java @@ -0,0 +1,64 @@ +/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + +package org.apache.poi.openxml4j; + +import org.apache.poi.hssf.HSSFTestDataSamples; +import org.apache.poi.openxml4j.exceptions.InvalidFormatException; +import org.apache.poi.openxml4j.opc.OPCPackage; +import org.apache.poi.openxml4j.util.ZipSecureFile; +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +class TestOPCPackageFileLimit { + @Test + void testWithReducedFileLimit() throws InvalidFormatException { + final long defaultLimit = ZipSecureFile.getMaxFileCount(); + ZipSecureFile.setMaxFileCount(5); + try (InputStream is = HSSFTestDataSamples.openSampleFileStream("HeaderFooterComplexFormats.xlsx")) { + OPCPackage opcPackage = OPCPackage.open(is); + fail("expected IOException"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("ZipSecureFile.setMaxFileCount()"), + "unexpected exception message: " + e.getMessage()); + } finally { + ZipSecureFile.setMaxFileCount(defaultLimit); + } + } + + @Test + void testFileWithReducedFileLimit() { + final File file = HSSFTestDataSamples.getSampleFile("HeaderFooterComplexFormats.xlsx"); + final long defaultLimit = ZipSecureFile.getMaxFileCount(); + ZipSecureFile.setMaxFileCount(5); + try { + OPCPackage opcPackage = OPCPackage.open(file); + fail("expected InvalidFormatException"); + } catch (InvalidFormatException e) { + assertTrue(e.getMessage().contains("ZipSecureFile.setMaxFileCount()"), + "unexpected exception message: " + e.getMessage()); + } finally { + ZipSecureFile.setMaxFileCount(defaultLimit); + } + } +} diff --git a/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipSecureFile.java b/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipSecureFile.java index e639e2fee8..064b0825f3 100644 --- a/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipSecureFile.java +++ b/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipSecureFile.java @@ -79,4 +79,25 @@ class TestZipSecureFile { ZipSecureFile.setMaxTextSize(ZipSecureFile.DEFAULT_MAX_TEXT_SIZE); } } + + @Test + void testSettingGraceEntrySize() { + long approx8G = ZipSecureFile.MAX_ENTRY_SIZE * 2; + try { + ZipSecureFile.setGraceEntrySize(approx8G); + assertEquals(approx8G, ZipSecureFile.getGraceEntrySize()); + } finally { + ZipSecureFile.setGraceEntrySize(ZipSecureFile.DEFAULT_GRACE_ENTRY_SIZE); + } + } + + @Test + void testSettingMaxFileCount() { + try { + ZipSecureFile.setMaxFileCount(123456789); + assertEquals(123456789, ZipSecureFile.getMaxFileCount()); + } finally { + ZipSecureFile.setMaxFileCount(ZipSecureFile.DEFAULT_MAX_FILE_COUNT); + } + } } diff --git a/poi-ooxml/src/test/java/org/apache/poi/xssf/TestXSSFFileChecks.java b/poi-ooxml/src/test/java/org/apache/poi/xssf/TestXSSFFileChecks.java new file mode 100644 index 0000000000..2fc978bd35 --- /dev/null +++ b/poi-ooxml/src/test/java/org/apache/poi/xssf/TestXSSFFileChecks.java @@ -0,0 +1,100 @@ +/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + +package org.apache.poi.xssf; + +import org.apache.poi.hssf.HSSFTestDataSamples; +import org.apache.poi.openxml4j.exceptions.InvalidFormatException; +import org.apache.poi.openxml4j.util.ZipSecureFile; +import org.apache.poi.xssf.usermodel.XSSFWorkbook; +import org.junit.jupiter.api.Test; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +class TestXSSFFileChecks { + @Test + void testWithReducedFileLimit() { + final long defaultLimit = ZipSecureFile.getMaxFileCount(); + ZipSecureFile.setMaxFileCount(5); + try (InputStream is = HSSFTestDataSamples.openSampleFileStream("HeaderFooterComplexFormats.xlsx")) { + XSSFWorkbook xssfWorkbook = new XSSFWorkbook(is); + fail("expected IOException"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("ZipSecureFile.setMaxFileCount()"), + "unexpected exception message: " + e.getMessage()); + } finally { + ZipSecureFile.setMaxFileCount(defaultLimit); + } + } + + @Test + void testFileWithReducedFileLimit() throws IOException { + final File file = HSSFTestDataSamples.getSampleFile("HeaderFooterComplexFormats.xlsx"); + final long defaultLimit = ZipSecureFile.getMaxFileCount(); + ZipSecureFile.setMaxFileCount(5); + try { + XSSFWorkbook xssfWorkbook = new XSSFWorkbook(file); + fail("expected InvalidFormatException"); + } catch (InvalidFormatException e) { + assertTrue(e.getMessage().contains("ZipSecureFile.setMaxFileCount()"), + "unexpected exception message: " + e.getMessage()); + } finally { + ZipSecureFile.setMaxFileCount(defaultLimit); + } + } + + @Test + void testWithGraceEntrySize() throws IOException { + final double defaultInflateRatio = ZipSecureFile.getMinInflateRatio(); + // setting MinInflateRatio but the default GraceEntrySize will mean this is ignored + // this exception will not happen with the default GraceEntrySize + ZipSecureFile.setMinInflateRatio(0.50); + try (InputStream is = HSSFTestDataSamples.openSampleFileStream("HeaderFooterComplexFormats.xlsx")) { + XSSFWorkbook xssfWorkbook = new XSSFWorkbook(is); + assertNotNull(xssfWorkbook); + } finally { + ZipSecureFile.setMinInflateRatio(defaultInflateRatio); + } + } + + @Test + void testWithReducedGraceEntrySize() { + final long defaultGraceSize = ZipSecureFile.getGraceEntrySize(); + final double defaultInflateRatio = ZipSecureFile.getMinInflateRatio(); + ZipSecureFile.setGraceEntrySize(0); + // setting MinInflateRatio to cause an exception + // this exception will not happen with the default GraceEntrySize + ZipSecureFile.setMinInflateRatio(0.50); + try (InputStream is = HSSFTestDataSamples.openSampleFileStream("HeaderFooterComplexFormats.xlsx")) { + XSSFWorkbook xssfWorkbook = new XSSFWorkbook(is); + fail("expected IOException"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("ZipSecureFile.setMinInflateRatio()"), + "unexpected exception message: " + e.getMessage()); + } finally { + ZipSecureFile.setMinInflateRatio(defaultInflateRatio); + ZipSecureFile.setGraceEntrySize(defaultGraceSize); + } + } + +} diff --git a/poi-ooxml/src/test/java9/module-info.java b/poi-ooxml/src/test/java9/module-info.java index 7ed78a0e75..d385eb2142 100644 --- a/poi-ooxml/src/test/java9/module-info.java +++ b/poi-ooxml/src/test/java9/module-info.java @@ -81,6 +81,7 @@ module org.apache.poi.ooxml { exports org.apache.poi.poifs.crypt.temp; opens org.apache.poi.openxml4j.opc to org.apache.poi.poi, org.junit.platform.commons; + opens org.apache.poi.openxml4j to org.apache.poi.ooxml, org.junit.platform.commons; /* optional dependencies for xml signatures - you need to add a require entry your module-info