From 1e9efb5562517c657293e49d57654f3bd55fc3a5 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Sun, 5 Dec 2021 17:34:44 +0000 Subject: [PATCH] Fix issues found when fuzzing Apache POI via Jazzer Add some null-checks and report more meaningful exceptions This provides a bit more information than simple NullPointExceptions git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1895600 13f79535-47bb-0310-9956-ffa450edef68 --- .../PackagePropertiesUnmarshaller.java | 29 ++++++----- .../poi/xdgf/usermodel/XDGFPageContents.java | 3 ++ .../poi/xdgf/usermodel/XmlVisioDocument.java | 14 +++--- .../apache/poi/xssf/usermodel/XSSFSheet.java | 6 ++- .../poi/xssf/usermodel/XSSFWorkbook.java | 48 +++++++++++-------- .../poi/hslf/usermodel/HSLFSlideMaster.java | 15 ++++-- .../poi/hslf/usermodel/HSLFSlideShow.java | 15 +++++- .../poi/hslf/usermodel/HSLFSlideShowImpl.java | 7 +++ .../poi/hslf/usermodel/HSLFTextParagraph.java | 20 ++++---- .../poi/poifs/crypt/agile/AgileDecryptor.java | 4 ++ .../crypt/agile/AgileEncryptionVerifier.java | 11 +++-- .../poi/poifs/filesystem/POIFSMiniStore.java | 4 ++ .../poi/poifs/property/PropertyTable.java | 4 +- 13 files changed, 123 insertions(+), 57 deletions(-) diff --git a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/internal/unmarshallers/PackagePropertiesUnmarshaller.java b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/internal/unmarshallers/PackagePropertiesUnmarshaller.java index f3fac329d1..bc2d36ba73 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/internal/unmarshallers/PackagePropertiesUnmarshaller.java +++ b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/opc/internal/unmarshallers/PackagePropertiesUnmarshaller.java @@ -240,45 +240,52 @@ public final class PackagePropertiesUnmarshaller implements PartUnmarshaller { for (int i = 0; i < namedNodeCount; i++) { Attr attr = (Attr)namedNodeMap.item(0); - if (attr.getNamespaceURI().equals(XMLConstants.XMLNS_ATTRIBUTE_NS_URI)) { + if (attr != null && XMLConstants.XMLNS_ATTRIBUTE_NS_URI.equals(attr.getNamespaceURI())) { // Rule M4.2 - if (attr.getValue().equals(PackageNamespaces.MARKUP_COMPATIBILITY)) + if (PackageNamespaces.MARKUP_COMPATIBILITY.equals(attr.getValue())) { throw new InvalidFormatException( "OPC Compliance error [M4.2]: A format consumer shall consider the use of the Markup Compatibility namespace to be an error."); - + } } } // Rule M4.3 String elName = el.getLocalName(); - if (el.getNamespaceURI().equals(PackageProperties.NAMESPACE_DCTERMS)) - if (!(elName.equals(KEYWORD_CREATED) || elName.equals(KEYWORD_MODIFIED))) + if (PackageProperties.NAMESPACE_DCTERMS.equals(el.getNamespaceURI())) { + if (!(KEYWORD_CREATED.equals(elName) || KEYWORD_MODIFIED.equals(elName))) { throw new InvalidFormatException( "OPC Compliance error [M4.3]: Producers shall not create a document element that contains refinements to the Dublin Core elements, except for the two specified in the schema: and Consumers shall consider a document element that violates this constraint to be an error."); + } + } // Rule M4.4 - if (el.getAttributeNodeNS(XMLConstants.XML_NS_URI, "lang") != null) + if (el.getAttributeNodeNS(XMLConstants.XML_NS_URI, "lang") != null) { throw new InvalidFormatException( "OPC Compliance error [M4.4]: Producers shall not create a document element that contains the xml:lang attribute. Consumers shall consider a document element that violates this constraint to be an error."); + } // Rule M4.5 - if (el.getNamespaceURI().equals(PackageProperties.NAMESPACE_DCTERMS)) { + if (PackageProperties.NAMESPACE_DCTERMS.equals(el.getNamespaceURI())) { // DCTerms namespace only use with 'created' and 'modified' elements - if (!(elName.equals(KEYWORD_CREATED) || elName.equals(KEYWORD_MODIFIED))) + if (!(elName.equals(KEYWORD_CREATED) || elName.equals(KEYWORD_MODIFIED))) { throw new InvalidFormatException("Namespace error : " + elName + " shouldn't have the following naemspace -> " + PackageProperties.NAMESPACE_DCTERMS); + } // Check for the 'xsi:type' attribute Attr typeAtt = el.getAttributeNodeNS(XMLConstants.W3C_XML_SCHEMA_INSTANCE_NS_URI, "type"); - if (typeAtt == null) + if (typeAtt == null) { throw new InvalidFormatException("The element '" + elName + "' must have the 'xsi:type' attribute present !"); + } // Check for the attribute value => 'dcterms:W3CDTF' - if (!typeAtt.getValue().equals(el.getPrefix() + ":W3CDTF")) + if (!typeAtt.getValue().equals(el.getPrefix() + ":W3CDTF")) { throw new InvalidFormatException("The element '" + elName - + "' must have the 'xsi:type' attribute with the value '" + el.getPrefix() + ":W3CDTF', but had '" + typeAtt.getValue() + "' !"); + + "' must have the 'xsi:type' attribute with the value '" + el.getPrefix() + ":W3CDTF', but had '" + + typeAtt.getValue() + "' !"); + } } // Check its children diff --git a/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XDGFPageContents.java b/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XDGFPageContents.java index 658c2ca985..4f9303db83 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XDGFPageContents.java +++ b/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XDGFPageContents.java @@ -55,6 +55,9 @@ public class XDGFPageContents extends XDGFBaseContents { //throw new POIXMLException("Unexpected page relation: " + part); XDGFMaster master = ((XDGFMasterContents)part).getMaster(); + if (master == null) { + throw new POIXMLException("Master entry is missing in XDGFPageContents"); + } _masters.put(master.getID(), master); } diff --git a/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XmlVisioDocument.java b/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XmlVisioDocument.java index f0242f074d..a1534f7c27 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XmlVisioDocument.java +++ b/poi-ooxml/src/main/java/org/apache/poi/xdgf/usermodel/XmlVisioDocument.java @@ -38,18 +38,18 @@ import com.microsoft.schemas.office.visio.x2012.main.VisioDocumentType; /** * This is your high-level starting point for working with Visio XML * documents (.vsdx). - * + * * Currently, only read support has been implemented, and the API is * not mature and is subject to change. - * + * * For more information about the visio XML format (with an XSD 1.0 * schema), you can find documentation at * https://msdn.microsoft.com/en-us/library/hh645006(v=office.12).aspx - * + * * That document lacks in some areas, but you can find additional * documentation and an updated XSD 1.1 schema at * https://msdn.microsoft.com/en-us/library/office/jj684209(v=office.15).aspx - * + * * Each provides different details, but the SharePoint reference * has better documentation and is more useful. */ @@ -101,7 +101,9 @@ public class XmlVisioDocument extends POIXMLDocument { _masters.onDocumentRead(); } - _pages.onDocumentRead(); + if (_pages != null) { + _pages.onDocumentRead(); + } } /** @@ -115,7 +117,7 @@ public class XmlVisioDocument extends POIXMLDocument { // // Useful public API goes here // - + /** * @return pages ordered by page number */ diff --git a/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java b/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java index b2696643ca..886d25b563 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java +++ b/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java @@ -87,7 +87,7 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet, OoxmlSheetEx private static final double DEFAULT_MARGIN_BOTTOM = 0.75; private static final double DEFAULT_MARGIN_LEFT = 0.7; private static final double DEFAULT_MARGIN_RIGHT = 0.7; - + //TODO make the two variable below private! protected CTSheet sheet; protected CTWorksheet worksheet; @@ -192,6 +192,10 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet, OoxmlSheetEx } private void initRows(CTWorksheet worksheetParam) { + if (worksheetParam.getSheetData() == null || worksheetParam.getSheetData().getRowArray() == null) { + throw new IllegalArgumentException("Had empty sheet data when initializing the sheet"); + } + _rows.clear(); tables = new TreeMap<>(); sharedFormulas = new HashMap<>(); diff --git a/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java b/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java index 55bcfbd1d2..2803ffe381 100644 --- a/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java +++ b/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFWorkbook.java @@ -403,6 +403,12 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su // Load individual sheets. The order of sheets is defined by the order // of CTSheet elements in the workbook sheets = new ArrayList<>(shIdMap.size()); + + if (this.workbook == null || this.workbook.getSheets() == null || + this.workbook.getSheets().getSheetArray() == null) { + throw new POIXMLException("Cannot read a workbook without sheets"); + } + for (CTSheet ctSheet : this.workbook.getSheets().getSheetArray()) { parseSheet(shIdMap, ctSheet); } @@ -671,22 +677,22 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su clonedDg.getCTDrawing().set(dg.getCTDrawing().copy()); // Clone drawing relations - XSSFDrawing drawingPatriarch = srcSheet.getDrawingPatriarch(); - if (drawingPatriarch != null) { - List srcRels = drawingPatriarch.getRelationParts(); - for (RelationPart rp : srcRels) { - POIXMLDocumentPart r = rp.getDocumentPart(); - if (r instanceof XSSFChart) { - // Replace chart relation part with new relationship, cloning the chart's content - RelationPart chartPart = clonedDg.createChartRelationPart(); - XSSFChart chart = chartPart.getDocumentPart(); - chart.importContent((XSSFChart) r); - chart.replaceReferences(clonedSheet); - } else { - addRelation(rp, clonedDg); - } - } - } + XSSFDrawing drawingPatriarch = srcSheet.getDrawingPatriarch(); + if (drawingPatriarch != null) { + List srcRels = drawingPatriarch.getRelationParts(); + for (RelationPart rp : srcRels) { + POIXMLDocumentPart r = rp.getDocumentPart(); + if (r instanceof XSSFChart) { + // Replace chart relation part with new relationship, cloning the chart's content + RelationPart chartPart = clonedDg.createChartRelationPart(); + XSSFChart chart = chartPart.getDocumentPart(); + chart.importContent((XSSFChart) r); + chart.replaceReferences(clonedSheet); + } else { + addRelation(rp, clonedDg); + } + } + } } return clonedSheet; } @@ -875,11 +881,11 @@ public class XSSFWorkbook extends POIXMLDocument implements Workbook, Date1904Su if(sheetname.length() > MAX_SENSITIVE_SHEET_NAME_LEN) { String trimmedSheetname = sheetname.substring(0, MAX_SENSITIVE_SHEET_NAME_LEN); - // we still need to warn about the trimming as the original sheet name won't be available - // e.g. when referenced by formulas - LOG.atWarn().log("Sheet '{}' will be added with a trimmed name '{}' for MS Excel compliance.", - sheetname, trimmedSheetname); - sheetname = trimmedSheetname; + // we still need to warn about the trimming as the original sheet name won't be available + // e.g. when referenced by formulas + LOG.atWarn().log("Sheet '{}' will be added with a trimmed name '{}' for MS Excel compliance.", + sheetname, trimmedSheetname); + sheetname = trimmedSheetname; } WorkbookUtil.validateSheetName(sheetname); diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideMaster.java b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideMaster.java index dbd5079118..513c8319c3 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideMaster.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideMaster.java @@ -143,14 +143,23 @@ public final class HSLFSlideMaster extends HSLFMasterSheet { assert (_txmaster == null); _txmaster = new TxMasterStyleAtom[9]; + if (getSlideShow() == null || getSlideShow().getDocumentRecord() == null || + getSlideShow().getDocumentRecord().getEnvironment() == null) { + throw new IllegalStateException("Did not find a TxMasterStyleAtom in the current slide show"); + } + TxMasterStyleAtom txdoc = getSlideShow().getDocumentRecord().getEnvironment().getTxMasterStyleAtom(); + if (txdoc == null) { + throw new IllegalStateException("Did not find a TxMasterStyleAtom in the current slide show"); + } + _txmaster[txdoc.getTextType()] = txdoc; TxMasterStyleAtom[] txrec = ((MainMaster)getSheetContainer()).getTxMasterStyleAtoms(); - for (int i = 0; i < txrec.length; i++) { - int txType = txrec[i].getTextType(); + for (TxMasterStyleAtom txMasterStyleAtom : txrec) { + int txType = txMasterStyleAtom.getTextType(); if (txType < _txmaster.length && _txmaster[txType] == null) { - _txmaster[txType] = txrec[i]; + _txmaster[txType] = txMasterStyleAtom; } } diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java index 466f780336..f7c7d4fa7e 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSlideShow.java @@ -276,7 +276,9 @@ public final class HSLFSlideShow extends POIDocument implements SlideShow recordMap, Map offset2id) { while (usrOffset != 0) { UserEditAtom usr = (UserEditAtom) Record.buildRecordAtOffset(docstream, usrOffset); + if (usr == null) { + throw new CorruptPowerPointFileException("Powerpoint document contains no user edit atom"); + } + recordMap.put(usrOffset, usr); int psrOffset = usr.getPersistPointersOffset(); PersistPtrHolder ptr = (PersistPtrHolder) Record.buildRecordAtOffset(docstream, psrOffset); + if (ptr == null) { + throw new CorruptPowerPointFileException("Powerpoint document is missing a PersistPtrHolder at " + psrOffset); + } recordMap.put(psrOffset, ptr); for (Map.Entry entry : ptr.getSlideLocationsLookup().entrySet()) { diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFTextParagraph.java b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFTextParagraph.java index 318b0ffded..98218e2d23 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFTextParagraph.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFTextParagraph.java @@ -1322,14 +1322,18 @@ public final class HSLFTextParagraph implements TextParagraph> findTextParagraphs(PPDrawing ppdrawing, HSLFSheet sheet) { - List> runsV = new ArrayList<>(); - for (EscherTextboxWrapper wrapper : ppdrawing.getTextboxWrappers()) { - List p = findTextParagraphs(wrapper, sheet); - if (p != null) { - runsV.add(p); - } - } - return runsV; + if (ppdrawing == null) { + throw new IllegalArgumentException("Did not receive a valid drawing for sheet " + sheet._getSheetNumber()); + } + + List> runsV = new ArrayList<>(); + for (EscherTextboxWrapper wrapper : ppdrawing.getTextboxWrappers()) { + List p = findTextParagraphs(wrapper, sheet); + if (p != null) { + runsV.add(p); + } + } + return runsV; } /** diff --git a/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileDecryptor.java b/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileDecryptor.java index 898670eaba..a6881bc5bc 100644 --- a/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileDecryptor.java +++ b/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileDecryptor.java @@ -196,6 +196,10 @@ public class AgileDecryptor extends Decryptor { Cipher cipher = getCipher(skey, cipherAlgo, chainMode, iv, cipherMode); byte[] hashFinal; + if (inputKey == null) { + throw new EncryptedDocumentException("Cannot has input without inputKey"); + } + try { inputKey = getBlock0(inputKey, getNextBlockSize(inputKey.length, blockSize)); hashFinal = cipher.doFinal(inputKey); diff --git a/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileEncryptionVerifier.java b/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileEncryptionVerifier.java index 6699b543c4..1e7bdd9a44 100644 --- a/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileEncryptionVerifier.java +++ b/poi/src/main/java/org/apache/poi/poifs/crypt/agile/AgileEncryptionVerifier.java @@ -45,7 +45,7 @@ public class AgileEncryptionVerifier extends EncryptionVerifier { } if (keyData == null) { - throw new NullPointerException("encryptedKey not set"); + throw new IllegalArgumentException("encryptedKey not set"); } setCipherAlgorithm(keyData.getCipherAlgorithm()); @@ -64,14 +64,17 @@ public class AgileEncryptionVerifier extends EncryptionVerifier { keyData.getHashAlgorithm() + " @ " + hashSize + " bytes"); } - setSpinCount(keyData.getSpinCount()); + Integer spinCount = keyData.getSpinCount(); + if (spinCount != null) { + setSpinCount(spinCount); + } setEncryptedVerifier(keyData.getEncryptedVerifierHashInput()); setSalt(keyData.getSaltValue()); setEncryptedKey(keyData.getEncryptedKeyValue()); setEncryptedVerifierHash(keyData.getEncryptedVerifierHashValue()); - int saltSize = keyData.getSaltSize(); - if (saltSize != getSalt().length) { + Integer saltSize = keyData.getSaltSize(); + if (saltSize == null || saltSize != getSalt().length) { throw new EncryptedDocumentException("Invalid salt size"); } diff --git a/poi/src/main/java/org/apache/poi/poifs/filesystem/POIFSMiniStore.java b/poi/src/main/java/org/apache/poi/poifs/filesystem/POIFSMiniStore.java index db49e5b814..18cdfd58f7 100644 --- a/poi/src/main/java/org/apache/poi/poifs/filesystem/POIFSMiniStore.java +++ b/poi/src/main/java/org/apache/poi/poifs/filesystem/POIFSMiniStore.java @@ -29,6 +29,7 @@ import org.apache.poi.poifs.property.RootProperty; import org.apache.poi.poifs.storage.BATBlock; import org.apache.poi.poifs.storage.BATBlock.BATBlockAndIndex; import org.apache.poi.poifs.storage.HeaderBlock; +import org.apache.poi.util.RecordFormatException; /** * This class handles the MiniStream (small block store) @@ -43,6 +44,9 @@ public class POIFSMiniStore extends BlockStore { POIFSMiniStore(POIFSFileSystem filesystem, RootProperty root, List sbats, HeaderBlock header) { + if (root == null) { + throw new RecordFormatException("Invalid argument to POIFSMiniStore: root is null"); + } this._filesystem = filesystem; this._sbat_blocks = sbats; this._header = header; diff --git a/poi/src/main/java/org/apache/poi/poifs/property/PropertyTable.java b/poi/src/main/java/org/apache/poi/poifs/property/PropertyTable.java index 42f380f341..d7a54c38c9 100644 --- a/poi/src/main/java/org/apache/poi/poifs/property/PropertyTable.java +++ b/poi/src/main/java/org/apache/poi/poifs/property/PropertyTable.java @@ -105,7 +105,9 @@ public final class PropertyTable implements BATManaged { PropertyFactory.convertToProperties(data, _properties); } - populatePropertyTree( (DirectoryProperty)_properties.get(0)); + if (_properties.get(0) != null) { + populatePropertyTree((DirectoryProperty) _properties.get(0)); + } }