From 1ff1e84e4afcd4abdf454c584a909423c2a14b03 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Tue, 3 Jan 2023 19:52:03 +0000 Subject: [PATCH] Avoid some NullPointerException and ClassCastExceptions found when fuzzing Apache POI This mostly only makes thrown runtime-exceptions a bit more consistent and improves information in exceptions. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1906360 13f79535-47bb-0310-9956-ffa450edef68 --- .../poi/xdgf/usermodel/XmlVisioDocument.java | 4 +- .../org/apache/poi/hslf/record/ExObjList.java | 3 +- .../poi/hslf/usermodel/HSLFShapeFactory.java | 6 ++- .../apache/poi/hslf/usermodel/HSLFSheet.java | 3 ++ .../usermodel/HSLFSlideShowEncrypted.java | 6 +++ .../poi/hslf/usermodel/HSLFSlideShowImpl.java | 16 ++++-- .../org/apache/poi/hwpf/model/StyleSheet.java | 27 ++++++++-- .../poi/hwpf/sprm/TableSprmUncompressor.java | 3 ++ .../org/apache/poi/ddf/EscherBSERecord.java | 7 ++- .../poi/hssf/extractor/OldExcelExtractor.java | 17 ++++--- .../poi/hssf/model/InternalWorkbook.java | 6 ++- .../record/aggregates/CFRecordsAggregate.java | 6 ++- .../poi/hssf/usermodel/HSSFPatriarch.java | 50 ++++++++++++------- .../poi/hssf/usermodel/HSSFShapeFactory.java | 18 +++++-- .../poi/poifs/property/PropertyTable.java | 8 ++- 15 files changed, 138 insertions(+), 42 deletions(-) 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 12135ce1dc..f353b26848 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 @@ -122,11 +122,13 @@ public class XmlVisioDocument extends POIXMLDocument { * @return pages ordered by page number */ public Collection getPages() { + if (_pages == null) { + throw new IllegalStateException("No page-information available"); + } return _pages.getPageList(); } public XDGFStyleSheet getStyleById(long id) { return _document.getStyleById(id); } - } diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hslf/record/ExObjList.java b/poi-scratchpad/src/main/java/org/apache/poi/hslf/record/ExObjList.java index 6ccdae947b..160f6bd428 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hslf/record/ExObjList.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hslf/record/ExObjList.java @@ -117,7 +117,8 @@ public class ExObjList extends RecordContainer { for (Record child : _children) { if (child instanceof ExHyperlink) { ExHyperlink rec = (ExHyperlink) child; - if (rec.getExHyperlinkAtom().getNumber() == id) { + if (rec.getExHyperlinkAtom() != null && + rec.getExHyperlinkAtom().getNumber() == id) { return rec; } } diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFShapeFactory.java b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFShapeFactory.java index 41692b77fe..9993a9e357 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFShapeFactory.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFShapeFactory.java @@ -63,7 +63,11 @@ public final class HSLFShapeFactory { public static HSLFGroupShape createShapeGroup(EscherContainerRecord spContainer, ShapeContainer parent){ boolean isTable = false; - EscherContainerRecord ecr = (EscherContainerRecord)spContainer.getChild(0); + EscherRecord child = spContainer.getChild(0); + if (!(child instanceof EscherContainerRecord)) { + throw new RecordFormatException("Did not have a EscherContainerRecord: " + child); + } + EscherContainerRecord ecr = (EscherContainerRecord) child; EscherRecord opt = HSLFShape.getEscherChild(ecr, EscherRecordTypes.USER_DEFINED); if (opt != null) { diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSheet.java b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSheet.java index effe54e372..543ed41828 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSheet.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hslf/usermodel/HSLFSheet.java @@ -172,6 +172,9 @@ public abstract class HSLFSheet implements HSLFShapeContainer, Sheet 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"); + Record builtRecord = Record.buildRecordAtOffset(docstream, usrOffset); + if (!(builtRecord instanceof UserEditAtom)) { + throw new CorruptPowerPointFileException("Did not have a user edit atom: " + builtRecord); } + UserEditAtom usr = (UserEditAtom) builtRecord; recordMap.put(usrOffset, usr); @@ -500,6 +501,9 @@ public final class HSLFSlideShowImpl extends POIDocument implements Closeable { // records share an offset. Map> unmatchedRecords = new HashMap<>(); for (EscherRecord child : blipStore) { + if (!(child instanceof EscherBSERecord)) { + throw new CorruptPowerPointFileException("Did not have a EscherBSERecord: " + child); + } EscherBSERecord record = (EscherBSERecord) child; unmatchedRecords.computeIfAbsent(record.getOffset(), k -> new ArrayList<>()).add(record); } @@ -984,7 +988,13 @@ public final class HSLFSlideShowImpl extends POIDocument implements Closeable { private EscherContainerRecord getBlipStore() { Document documentRecord = null; for (Record record : _records) { + if (record == null) { + throw new CorruptPowerPointFileException("Did not have a valid record: " + record); + } if (record.getRecordType() == RecordTypes.Document.typeID) { + if (!(record instanceof Document)) { + throw new CorruptPowerPointFileException("Did not have a Document: " + record); + } documentRecord = (Document) record; break; } diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hwpf/model/StyleSheet.java b/poi-scratchpad/src/main/java/org/apache/poi/hwpf/model/StyleSheet.java index c9b8880f28..348c23677f 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hwpf/model/StyleSheet.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hwpf/model/StyleSheet.java @@ -62,7 +62,7 @@ public final class StyleSheet { /** * General information about a stylesheet */ - private Stshif _stshif; + private final Stshif _stshif; StyleDescription[] _styleDescriptions; @@ -205,6 +205,10 @@ public final class StyleSheet { @Deprecated private void createPap(int istd) { StyleDescription sd = _styleDescriptions[istd]; + if (sd == null) { + throw new IllegalStateException("Cannot create Pap, empty styleDescription, had : " + _styleDescriptions.length + " descriptions"); + } + ParagraphProperties pap = sd.getPAP(); byte[] papx = sd.getPAPX(); int baseIndex = sd.getBaseStyle(); @@ -212,7 +216,11 @@ public final class StyleSheet { ParagraphProperties parentPAP = new ParagraphProperties(); if (baseIndex != NIL_STYLE) { - parentPAP = _styleDescriptions[baseIndex].getPAP(); + StyleDescription styleDescription = _styleDescriptions[baseIndex]; + if (styleDescription == null) { + throw new IllegalStateException("Cannot create Pap, empty styleDescription, had : " + _styleDescriptions.length + " descriptions"); + } + parentPAP = styleDescription.getPAP(); if (parentPAP == null) { if (baseIndex == istd) { // Oh dear, style claims that it is its own parent @@ -220,7 +228,7 @@ public final class StyleSheet { } // Create the parent style createPap(baseIndex); - parentPAP = _styleDescriptions[baseIndex].getPAP(); + parentPAP = styleDescription.getPAP(); } } @@ -247,6 +255,10 @@ public final class StyleSheet { @Deprecated private void createChp(int istd) { StyleDescription sd = _styleDescriptions[istd]; + if (sd == null) { + throw new IllegalStateException("Cannot create Chp, empty styleDescription, had : " + _styleDescriptions.length + " descriptions"); + } + CharacterProperties chp = sd.getCHP(); byte[] chpx = sd.getCHPX(); int baseIndex = sd.getBaseStyle(); @@ -263,10 +275,15 @@ public final class StyleSheet { if (chp == null && chpx != null) { CharacterProperties parentCHP = new CharacterProperties(); if (baseIndex != NIL_STYLE) { - parentCHP = _styleDescriptions[baseIndex].getCHP(); + StyleDescription styleDescription = _styleDescriptions[baseIndex]; + if (styleDescription == null) { + throw new IllegalStateException("Cannot create Chp, empty styleDescription, had : " + _styleDescriptions.length + " descriptions"); + } + + parentCHP = styleDescription.getCHP(); if (parentCHP == null) { createChp(baseIndex); - parentCHP = _styleDescriptions[baseIndex].getCHP(); + parentCHP = styleDescription.getCHP(); } if (parentCHP == null) { parentCHP = new CharacterProperties(); diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/TableSprmUncompressor.java b/poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/TableSprmUncompressor.java index 54cab16890..267f6b6875 100644 --- a/poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/TableSprmUncompressor.java +++ b/poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/TableSprmUncompressor.java @@ -285,6 +285,9 @@ public final class TableSprmUncompressor extends SprmUncompressor { for (int c = itcFirst; c < itcLim; c++) { TableCellDescriptor tableCellDescriptor = newTAP.getRgtc()[c]; + if (tableCellDescriptor == null) { + throw new IllegalStateException("Cannot unCompress TAP, empty table cell descriptor, had : " + newTAP.getRgtc().length + " Rgtc"); + } if ((grfbrc & 0x01) != 0) { tableCellDescriptor.setFtsCellPaddingTop(ftsWidth); diff --git a/poi/src/main/java/org/apache/poi/ddf/EscherBSERecord.java b/poi/src/main/java/org/apache/poi/ddf/EscherBSERecord.java index 255fa212ec..4e37209115 100644 --- a/poi/src/main/java/org/apache/poi/ddf/EscherBSERecord.java +++ b/poi/src/main/java/org/apache/poi/ddf/EscherBSERecord.java @@ -111,8 +111,13 @@ public final class EscherBSERecord extends EscherRecord { int bytesRead = 0; if (bytesRemaining > 0) { + EscherRecord record = recordFactory.createRecord(data, pos + 36); + if (!(record instanceof EscherBlipRecord)) { + throw new IllegalArgumentException("Did not have a EscherBlipRecord: " + record); + } + // Some older escher formats skip this last record - field_12_blipRecord = (EscherBlipRecord) recordFactory.createRecord( data, pos + 36 ); + field_12_blipRecord = (EscherBlipRecord) record; bytesRead = field_12_blipRecord.fillFields( data, pos + 36, recordFactory ); } pos += 36 + bytesRead; diff --git a/poi/src/main/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java b/poi/src/main/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java index ac1ca9541c..f6deb15a10 100644 --- a/poi/src/main/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java +++ b/poi/src/main/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java @@ -44,6 +44,7 @@ import org.apache.poi.hssf.record.RecordInputStream; import org.apache.poi.hssf.usermodel.HSSFWorkbook; import org.apache.poi.poifs.filesystem.DirectoryNode; import org.apache.poi.poifs.filesystem.DocumentNode; +import org.apache.poi.poifs.filesystem.Entry; import org.apache.poi.poifs.filesystem.FileMagic; import org.apache.poi.poifs.filesystem.NotOLE2FileException; import org.apache.poi.poifs.filesystem.POIFSFileSystem; @@ -149,14 +150,18 @@ public class OldExcelExtractor implements POITextExtractor { private void open(DirectoryNode directory) throws IOException { DocumentNode book; try { - book = (DocumentNode)directory.getEntry(OLD_WORKBOOK_DIR_ENTRY_NAME); + Entry entry = directory.getEntry(OLD_WORKBOOK_DIR_ENTRY_NAME); + if (!(entry instanceof DocumentNode)) { + throw new IllegalArgumentException("Did not have an Excel 5/95 Book stream: " + entry); + } + book = (DocumentNode) entry; } catch (FileNotFoundException | IllegalArgumentException e) { // some files have "Workbook" instead - book = (DocumentNode)directory.getEntry(WORKBOOK_DIR_ENTRY_NAMES.get(0)); - } - - if (book == null) { - throw new IOException("No Excel 5/95 Book stream found"); + Entry entry = directory.getEntry(WORKBOOK_DIR_ENTRY_NAMES.get(0)); + if (!(entry instanceof DocumentNode)) { + throw new IllegalArgumentException("Did not have an Excel 5/95 Book stream: " + entry); + } + book = (DocumentNode) entry; } ris = new RecordInputStream(directory.createDocumentInputStream(book)); diff --git a/poi/src/main/java/org/apache/poi/hssf/model/InternalWorkbook.java b/poi/src/main/java/org/apache/poi/hssf/model/InternalWorkbook.java index 7ca3b143ee..003ad45c6b 100644 --- a/poi/src/main/java/org/apache/poi/hssf/model/InternalWorkbook.java +++ b/poi/src/main/java/org/apache/poi/hssf/model/InternalWorkbook.java @@ -830,7 +830,11 @@ public final class InternalWorkbook { xfptr += index; - return ( ExtendedFormatRecord ) records.get(xfptr); + Record record = records.get(xfptr); + if (!(record instanceof ExtendedFormatRecord)) { + throw new IllegalStateException("Did not have a ExtendedFormatRecord: " + record); + } + return (ExtendedFormatRecord) record; } /** diff --git a/poi/src/main/java/org/apache/poi/hssf/record/aggregates/CFRecordsAggregate.java b/poi/src/main/java/org/apache/poi/hssf/record/aggregates/CFRecordsAggregate.java index bcf80e6189..bbfe353873 100644 --- a/poi/src/main/java/org/apache/poi/hssf/record/aggregates/CFRecordsAggregate.java +++ b/poi/src/main/java/org/apache/poi/hssf/record/aggregates/CFRecordsAggregate.java @@ -125,7 +125,11 @@ public final class CFRecordsAggregate extends RecordAggregate implements Generic CFRuleBase[] rules = new CFRuleBase[nRules]; for (int i = 0; i < rules.length; i++) { - rules[i] = (CFRuleBase) rs.getNext(); + Record record = rs.getNext(); + if (!(record instanceof CFRuleBase)) { + throw new IllegalArgumentException("Did not have a CFRuleBase: " + record); + } + rules[i] = (CFRuleBase) record; } return new CFRecordsAggregate(header, rules); diff --git a/poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFPatriarch.java b/poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFPatriarch.java index 45ac29a9eb..876b6f245a 100644 --- a/poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFPatriarch.java +++ b/poi/src/main/java/org/apache/poi/hssf/usermodel/HSSFPatriarch.java @@ -32,6 +32,7 @@ import org.apache.poi.ddf.EscherContainerRecord; import org.apache.poi.ddf.EscherDgRecord; import org.apache.poi.ddf.EscherOptRecord; import org.apache.poi.ddf.EscherProperty; +import org.apache.poi.ddf.EscherRecord; import org.apache.poi.ddf.EscherSpRecord; import org.apache.poi.ddf.EscherSpgrRecord; import org.apache.poi.hssf.model.DrawingManager2; @@ -80,9 +81,24 @@ public final class HSSFPatriarch implements HSSFShapeContainer, Drawing