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

4 files changed, 147 insertions, 95 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 5423ea4d47..b06ddcb25a 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);

+            }

+

+            // no expanded size?

+            if (cis == null) {

+                return;

+            }

+            

+            // don't alert for small expanded size

+            if (counter <= GRACE_ENTRY_SIZE) {

+                return;

             }

-            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. "

+

+            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 2cbca15520..6540e049d5 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);
-			}
-
-			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();
-	}
+    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);
+            }
 
-	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);
-			}
-		}
-	}
+            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();
+    }
 
-	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 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 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 f40ce344f2..4b92b30bc2 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 4a9dd65c09..ca69fa6266 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();
+    }
 }