From 5833e6fe953deb70cccd9fea2acfc7d90edfcce2 Mon Sep 17 00:00:00 2001 From: Tim Allison Date: Wed, 15 Aug 2018 19:20:11 +0000 Subject: [PATCH] entity_expansion git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1838135 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/ooxml/util/DocumentHelper.java | 9 +++-- .../org/apache/poi/ooxml/util/SAXHelper.java | 4 +- .../apache/poi/ooxml/util/TestSAXHelper.java | 2 +- .../poi/xssf/usermodel/TestXSSFBugs.java | 38 ++++++++++++++++++- 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java b/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java index 835f76e351..684564c1b6 100644 --- a/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java +++ b/src/ooxml/java/org/apache/poi/ooxml/util/DocumentHelper.java @@ -99,7 +99,9 @@ public final class DocumentHelper { static { documentBuilderFactory.setNamespaceAware(true); documentBuilderFactory.setValidating(false); - + //this doesn't appear to work, and we still need to limit + //entity expansions to 1 in trySetXercesSecurityManager + documentBuilderFactory.setExpandEntityReferences(false); trySetSAXFeature(documentBuilderFactory, XMLConstants.FEATURE_SECURE_PROCESSING, true); trySetSAXFeature(documentBuilderFactory, POIXMLConstants.FEATURE_LOAD_DTD_GRAMMAR, false); trySetSAXFeature(documentBuilderFactory, POIXMLConstants.FEATURE_LOAD_EXTERNAL_DTD, false); @@ -125,7 +127,7 @@ public final class DocumentHelper { try { Object mgr = Class.forName(securityManagerClassName).newInstance(); Method setLimit = mgr.getClass().getMethod("setEntityExpansionLimit", Integer.TYPE); - setLimit.invoke(mgr, 4096); + setLimit.invoke(mgr, 1); dbf.setAttribute(POIXMLConstants.PROPERTY_SECURITY_MANAGER, mgr); // Stop once one can be setup without error return; @@ -137,7 +139,8 @@ public final class DocumentHelper { } // separate old version of Xerces not found => use the builtin way of setting the property - dbf.setAttribute(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT, 4096); + // Note: when entity_expansion_limit==0, there is no limit! + dbf.setAttribute(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT, 1); } /** diff --git a/src/ooxml/java/org/apache/poi/ooxml/util/SAXHelper.java b/src/ooxml/java/org/apache/poi/ooxml/util/SAXHelper.java index 38b1708159..2ebd9f8141 100644 --- a/src/ooxml/java/org/apache/poi/ooxml/util/SAXHelper.java +++ b/src/ooxml/java/org/apache/poi/ooxml/util/SAXHelper.java @@ -113,7 +113,7 @@ public final class SAXHelper { try { Object mgr = Class.forName(securityManagerClassName).newInstance(); Method setLimit = mgr.getClass().getMethod("setEntityExpansionLimit", Integer.TYPE); - setLimit.invoke(mgr, 0); + setLimit.invoke(mgr, 1); xmlReader.setProperty(POIXMLConstants.PROPERTY_SECURITY_MANAGER, mgr); // Stop once one can be setup without error return; @@ -130,7 +130,7 @@ public final class SAXHelper { // separate old version of Xerces not found => use the builtin way of setting the property try { - xmlReader.setProperty(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT, 4096); + xmlReader.setProperty(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT, 1); } catch (SAXException e) { // NOSONAR - also catch things like NoClassDefError here // throttle the log somewhat as it can spam the log otherwise if(System.currentTimeMillis() > lastLog + TimeUnit.MINUTES.toMillis(5)) { diff --git a/src/ooxml/testcases/org/apache/poi/ooxml/util/TestSAXHelper.java b/src/ooxml/testcases/org/apache/poi/ooxml/util/TestSAXHelper.java index 4a2fa80af4..1190c47612 100644 --- a/src/ooxml/testcases/org/apache/poi/ooxml/util/TestSAXHelper.java +++ b/src/ooxml/testcases/org/apache/poi/ooxml/util/TestSAXHelper.java @@ -36,7 +36,7 @@ public class TestSAXHelper { assertFalse(reader.getFeature(POIXMLConstants.FEATURE_LOAD_EXTERNAL_DTD)); assertEquals(SAXHelper.IGNORING_ENTITY_RESOLVER, reader.getEntityResolver()); assertNotNull(reader.getProperty(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT)); - assertEquals("4096", reader.getProperty(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT)); + assertEquals("1", reader.getProperty(POIXMLConstants.PROPERTY_ENTITY_EXPANSION_LIMIT)); assertNotNull(reader.getProperty(POIXMLConstants.PROPERTY_SECURITY_MANAGER)); reader.parse(new InputSource(new ByteArrayInputStream("".getBytes("UTF-8")))); diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java index 8d73577838..056c15a373 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFBugs.java @@ -44,7 +44,8 @@ import java.util.Set; import java.util.TimeZone; import java.util.TreeMap; -import org.apache.poi.EncryptedDocumentException; +import org.apache.commons.compress.archivers.zip.ZipArchiveEntry; +import org.apache.commons.compress.archivers.zip.ZipFile; import org.apache.poi.POIDataSamples; import org.apache.poi.ooxml.POIXMLDocumentPart; import org.apache.poi.ooxml.POIXMLDocumentPart.RelationPart; @@ -55,6 +56,8 @@ import org.apache.poi.hssf.HSSFITestDataProvider; import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator; import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ooxml.util.DocumentHelper; +import org.apache.poi.ooxml.util.SAXHelper; import org.apache.poi.openxml4j.exceptions.InvalidFormatException; import org.apache.poi.openxml4j.exceptions.InvalidOperationException; import org.apache.poi.openxml4j.exceptions.OpenXML4JException; @@ -104,6 +107,9 @@ import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTDefinedName; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTDefinedNames; import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTWorksheet; import org.openxmlformats.schemas.spreadsheetml.x2006.main.impl.CTFontImpl; +import org.xml.sax.InputSource; +import org.xml.sax.SAXParseException; +import org.xml.sax.XMLReader; public final class TestXSSFBugs extends BaseTestBugzillaIssues { public TestXSSFBugs() { @@ -1915,6 +1921,36 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues { wb.close(); } + @Test + public void test54764WithSAXHelper() throws Exception { + File testFile = XSSFTestDataSamples.getSampleFile("54764.xlsx"); + ZipFile zip = new ZipFile(testFile); + ZipArchiveEntry ze = zip.getEntry("xl/sharedStrings.xml"); + XMLReader reader = SAXHelper.newXMLReader(); + try { + reader.parse(new InputSource(zip.getInputStream(ze))); + fail("should have thrown SAXParseException"); + } catch (SAXParseException e) { + assertNotNull(e.getMessage()); + assertTrue(e.getMessage().contains("more than \"1\" entity")); + } + } + + @Test + public void test54764WithDocumentHelper() throws Exception { + File testFile = XSSFTestDataSamples.getSampleFile("54764.xlsx"); + ZipFile zip = new ZipFile(testFile); + ZipArchiveEntry ze = zip.getEntry("xl/sharedStrings.xml"); + try { + DocumentHelper.readDocument(zip.getInputStream(ze)); + fail("should have thrown SAXParseException"); + } catch (SAXParseException e) { + assertNotNull(e.getMessage()); + e.printStackTrace(); + assertTrue(e.getMessage().contains("more than \"1\" entity")); + } + } + /** * CTDefinedNamesImpl should be included in the smaller * poi-ooxml-schemas jar -- 2.39.5