From 3a8f40db7c1e294548d1f226b65938518c544272 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Sat, 17 Oct 2015 14:40:36 +0000 Subject: [PATCH] Bug 58499: Don't report Zip-Bomb for small files which should not cause memory issues anyway Also make error message a bit more specific and list classname in Zip-Bomb-Error to make it easier for users what the problem is and how to find out where the static methods are git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1709180 13f79535-47bb-0310-9956-ffa450edef68 --- .../poi/openxml4j/util/ZipSecureFile.java | 38 ++++- .../poi/xssf/streaming/TestSXSSFWorkbook.java | 140 +++++++++--------- .../poi/hssf/usermodel/TestHSSFWorkbook.java | 13 -- .../poi/ss/usermodel/BaseTestWorkbook.java | 49 +++++- 4 files changed, 146 insertions(+), 94 deletions(-) diff --git a/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java b/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java index 5423ea4d4..b06ddcb25 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java @@ -46,6 +46,9 @@ public class ZipSecureFile extends ZipFile { private static double MIN_INFLATE_RATIO = 0.01d; private static long MAX_ENTRY_SIZE = 0xFFFFFFFFl; + + // don't alert for expanded sizes smaller than 100k + private static long GRACE_ENTRY_SIZE = 100*1024; /** * Sets the ratio between de- and inflated bytes to detect zipbomb. @@ -183,16 +186,37 @@ public class ZipSecureFile extends ZipFile { public void advance(int advance) throws IOException { counter += advance; + // check the file size first, in case we are working on uncompressed streams - if (counter < MAX_ENTRY_SIZE) { - if (cis == null) return; - double ratio = (double)cis.counter/(double)counter; - if (ratio >= MIN_INFLATE_RATIO) return; + if(counter > MAX_ENTRY_SIZE) { + throw new IOException("Zip bomb detected! The file would exceed the max size of the expanded data in the zip-file. " + + "This may indicates that the file is used to inflate memory usage and thus could pose a security risk. " + + "You can adjust this limit via ZipSecureFile.setMaxEntrySize() if you need to work with files which are very large. " + + "Counter: " + counter + ", cis.counter: " + (cis == null ? 0 : cis.counter) + + "Limits: MAX_ENTRY_SIZE: " + MAX_ENTRY_SIZE); } - throw new IOException("Zip bomb detected! The file would exceed certain limits which usually indicate that the file is used to inflate memory usage and thus could pose a security risk. " - + "You can adjust these limits via setMinInflateRatio() and setMaxEntrySize() if you need to work with files which exceed these limits. " + + // no expanded size? + if (cis == null) { + return; + } + + // don't alert for small expanded size + if (counter <= GRACE_ENTRY_SIZE) { + return; + } + + double ratio = (double)cis.counter/(double)counter; + if (ratio >= MIN_INFLATE_RATIO) { + return; + } + + // one of the limits was reached, report it + throw new IOException("Zip bomb detected! The file would exceed the max. ratio of compressed file size to the size of the expanded data. " + + "This may indicate that the file is used to inflate memory usage and thus could pose a security risk. " + + "You can adjust this limit via ZipSecureFile.setMinInflateRatio() if you need to work with files which exceed this limit. " + "Counter: " + counter + ", cis.counter: " + (cis == null ? 0 : cis.counter) + ", ratio: " + (cis == null ? 0 : ((double)cis.counter)/counter) - + "Limits: MIN_INFLATE_RATIO: " + MIN_INFLATE_RATIO + ", MAX_ENTRY_SIZE: " + MAX_ENTRY_SIZE); + + "Limits: MIN_INFLATE_RATIO: " + MIN_INFLATE_RATIO); } public ZipEntry getNextEntry() throws IOException { diff --git a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java index 2cbca1552..6540e049d 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java @@ -28,7 +28,6 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.FileOutputStream; import java.io.IOException; -import java.io.OutputStream; import java.lang.reflect.Field; import org.apache.poi.ss.usermodel.BaseTestWorkbook; @@ -209,7 +208,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook { } @Test - public void sheetdataWriter(){ + public void sheetdataWriter() throws IOException{ SXSSFWorkbook wb = new SXSSFWorkbook(); SXSSFSheet sh = wb.createSheet(); SheetDataWriter wr = sh.getSheetDataWriter(); @@ -218,6 +217,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook { assertTrue(tmp.getName().startsWith("poi-sxssf-sheet")); assertTrue(tmp.getName().endsWith(".xml")); assertTrue(wb.dispose()); + wb.close(); wb = new SXSSFWorkbook(); wb.setCompressTempFiles(true); @@ -228,6 +228,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook { assertTrue(tmp.getName().startsWith("poi-sxssf-sheet-xml")); assertTrue(tmp.getName().endsWith(".gz")); assertTrue(wb.dispose()); + wb.close(); //Test escaping of Unicode control characters wb = new SXSSFWorkbook(); @@ -237,6 +238,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook { assertEquals("value?", cell.getStringCellValue()); assertTrue(wb.dispose()); + wb.close(); } @@ -329,80 +331,74 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook { // currently writing the same sheet multiple times is not supported... @Ignore - public void bug53515() throws Exception { - Workbook wb = new SXSSFWorkbook(10); - populateWorkbook(wb); - saveTwice(wb); - wb = new XSSFWorkbook(); - populateWorkbook(wb); - saveTwice(wb); - } + public void bug53515() throws Exception { + Workbook wb = new SXSSFWorkbook(10); + populateWorkbook(wb); + saveTwice(wb); + wb = new XSSFWorkbook(); + populateWorkbook(wb); + saveTwice(wb); + } - // Crashes the JVM because of documented JVM behavior with concurrent writing/reading of zip-files - // See http://www.oracle.com/technetwork/java/javase/documentation/overview-156328.html + // Crashes the JVM because of documented JVM behavior with concurrent writing/reading of zip-files + // See http://www.oracle.com/technetwork/java/javase/documentation/overview-156328.html @Ignore - public void bug53515a() throws Exception { - File out = new File("Test.xlsx"); - out.delete(); - for (int i = 0; i < 2; i++) { - System.out.println("Iteration " + i); - final SXSSFWorkbook wb; - if (out.exists()) { - wb = new SXSSFWorkbook( - (XSSFWorkbook) WorkbookFactory.create(out)); - } else { - wb = new SXSSFWorkbook(10); - } + public void bug53515a() throws Exception { + File out = new File("Test.xlsx"); + out.delete(); + for (int i = 0; i < 2; i++) { + System.out.println("Iteration " + i); + final SXSSFWorkbook wb; + if (out.exists()) { + wb = new SXSSFWorkbook( + (XSSFWorkbook) WorkbookFactory.create(out)); + } else { + wb = new SXSSFWorkbook(10); + } - try { - FileOutputStream outSteam = new FileOutputStream(out); - if (i == 0) { - populateWorkbook(wb); - } else { - System.gc(); - System.gc(); - System.gc(); - } + try { + FileOutputStream outSteam = new FileOutputStream(out); + if (i == 0) { + populateWorkbook(wb); + } else { + System.gc(); + System.gc(); + System.gc(); + } - wb.write(outSteam); - // assertTrue(wb.dispose()); - outSteam.close(); - } finally { - assertTrue(wb.dispose()); - } - } - out.delete(); - } + wb.write(outSteam); + // assertTrue(wb.dispose()); + outSteam.close(); + } finally { + assertTrue(wb.dispose()); + } + } + out.delete(); + } - private static void populateWorkbook(Workbook wb) { - Sheet sh = wb.createSheet(); - for (int rownum = 0; rownum < 100; rownum++) { - Row row = sh.createRow(rownum); - for (int cellnum = 0; cellnum < 10; cellnum++) { - Cell cell = row.createCell(cellnum); - String address = new CellReference(cell).formatAsString(); - cell.setCellValue(address); - } - } - } + private static void populateWorkbook(Workbook wb) { + Sheet sh = wb.createSheet(); + for (int rownum = 0; rownum < 100; rownum++) { + Row row = sh.createRow(rownum); + for (int cellnum = 0; cellnum < 10; cellnum++) { + Cell cell = row.createCell(cellnum); + String address = new CellReference(cell).formatAsString(); + cell.setCellValue(address); + } + } + } - private static void saveTwice(Workbook wb) throws Exception { - for (int i = 0; i < 2; i++) { - try { - NullOutputStream out = new NullOutputStream(); - wb.write(out); - out.close(); - } catch (Exception e) { - throw new Exception("ERROR: failed on " + (i + 1) - + "th time calling " + wb.getClass().getName() - + ".write() with exception " + e.getMessage(), e); - } - } - } - - private static class NullOutputStream extends OutputStream { - @Override - public void write(int b) throws IOException { - } - } + private static void saveTwice(Workbook wb) throws Exception { + for (int i = 0; i < 2; i++) { + try { + NullOutputStream out = new NullOutputStream(); + wb.write(out); + out.close(); + } catch (Exception e) { + throw new Exception("ERROR: failed on " + (i + 1) + + "th time calling " + wb.getClass().getName() + + ".write() with exception " + e.getMessage(), e); + } + } + } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java index f40ce344f..4b92b30bc 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java @@ -81,19 +81,6 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { return wb.getWorkbook(); } - @Test - public void windowOneDefaults() throws IOException { - HSSFWorkbook b = new HSSFWorkbook( ); - try { - assertEquals(b.getActiveSheetIndex(), 0); - assertEquals(b.getFirstVisibleTab(), 0); - } catch (NullPointerException npe) { - fail("WindowOneRecord in Workbook is probably not initialized"); - } - - b.close(); - } - /** * Tests for {@link HSSFWorkbook#isHidden()} etc * @throws IOException diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java index 4a9dd65c0..ca69fa626 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java +++ b/src/testcases/org/apache/poi/ss/usermodel/BaseTestWorkbook.java @@ -26,15 +26,16 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.IOException; +import java.io.OutputStream; import java.util.ConcurrentModificationException; import java.util.Iterator; -import junit.framework.AssertionFailedError; - import org.apache.poi.ss.ITestDataProvider; import org.apache.poi.ss.util.CellRangeAddress; import org.junit.Test; +import junit.framework.AssertionFailedError; + /** * @author Yegor Kozlov */ @@ -753,4 +754,48 @@ public abstract class BaseTestWorkbook { sheets[i], wb.getSheetAt(i).getSheetName()); } } + + protected static class NullOutputStream extends OutputStream { + public NullOutputStream() { + } + + @Override + public void write(int b) throws IOException { + } + } + + @Test + public void test58499() throws IOException { + Workbook workbook = _testDataProvider.createWorkbook(); + Sheet sheet = workbook.createSheet(); + for (int i = 0; i < 900; i++) { + Row r = sheet.createRow(i); + Cell c = r.createCell(0); + CellStyle cs = workbook.createCellStyle(); + c.setCellStyle(cs); + c.setCellValue("AAA"); + } + OutputStream os = new NullOutputStream(); + try { + workbook.write(os); + } finally { + os.close(); + } + //workbook.dispose(); + workbook.close(); + } + + + @Test + public void windowOneDefaults() throws IOException { + Workbook b = _testDataProvider.createWorkbook(); + try { + assertEquals(b.getActiveSheetIndex(), 0); + assertEquals(b.getFirstVisibleTab(), 0); + } catch (NullPointerException npe) { + fail("WindowOneRecord in Workbook is probably not initialized"); + } + + b.close(); + } }