You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@poi.apache.org by fa...@apache.org on 2026/09/22 12:35:56 UTC
(poi) branch trunk updated: Bounds check untrusted 32-bit counts before narrowing them to int (#1345)
This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/poi.git
The following commit(s) were added to refs/heads/trunk by this push:
new 941f09ecce Bounds check untrusted 32-bit counts before narrowing them to int (#1345)
941f09ecce is described below
commit 941f09ecce1d061feb5f22f4d57b410faff17ddc
Author: PJ Fanning <pj...@users.noreply.github.com>
AuthorDate: Tue Sep 22 13:35:51 2026 +0100
Bounds check untrusted 32-bit counts before narrowing them to int (#1345)
Several parsers read an unsigned 32-bit count or size field, narrow it with
Math.toIntExact, and only then range check it. For a field above
Integer.MAX_VALUE the narrowing throws ArithmeticException first, so the check
that follows never runs and the caller sees an unrelated exception instead of
the RecordFormatException the guards are written to produce.
Keep these values as longs until after the check. The IOUtils/ArrayUtil
checkers already take a long and already reject both negative and
over-Integer.MAX_VALUE lengths with a descriptive message.
Sites changed:
* HemfDraw.EmfPolyBezier / EmfPolygon - the spec says extra points MUST be
ignored and the code caps the count at 16K, but the narrowing ran first, so
an oversized count was rejected instead of clamped.
* HemfDraw.EmfPolyDraw - narrowing pre-empted HemfPicture.safelyAllocateCheck.
* HemfComment.EmfCommentDataBeginGroup / EmfCommentDataWMF - narrowing
pre-empted IOUtils.safelyAllocate.
* hpsf Section.readDictionary - the oversize test sits outside the try/catch
that records a corrupted dictionary, so the narrowing threw
ArithmeticException out of the Section constructor rather than taking the
corrupted-dictionary path.
Reported for the EMF records by a security researcher. The EMF paths are not
themselves an availability problem: HemfRecordIterator already wraps any
RuntimeException into RecordFormatException, and the allocation guards
themselves were added in #1104, #1107 and #1114. This corrects the ordering so
the guards report what they were meant to report.
Co-authored-by: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../apache/poi/hemf/record/emf/HemfComment.java | 4 +-
.../org/apache/poi/hemf/record/emf/HemfDraw.java | 13 ++---
.../apache/poi/hemf/usermodel/TestHemfPicture.java | 58 ++++++++++++++++++++++
poi/src/main/java/org/apache/poi/hpsf/Section.java | 5 +-
.../org/apache/poi/hpsf/TestOverflowHardening.java | 29 +++++++++++
5 files changed, 99 insertions(+), 10 deletions(-)
diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfComment.java b/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfComment.java
index 62a70a6fc3..e966cdcf6f 100644
--- a/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfComment.java
+++ b/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfComment.java
@@ -408,7 +408,7 @@ public class HemfComment {
HemfDraw.readRectL(leis, bounds);
// The number of Unicode characters in the optional description string that follows.
- int nDescription = Math.toIntExact(leis.readUInt());
+ long nDescription = leis.readUInt();
byte[] buf = IOUtils.safelyAllocate(nDescription * 2L, getMaxRecordLength(),
"HemfComment.setMaxRecordLength()");
@@ -627,7 +627,7 @@ public class HemfComment {
// A 32-bit unsigned integer that specifies the size, in bytes, of the
// WMF metafile in the WinMetafile field.
- int winMetafileSize = Math.toIntExact(leis.readUInt());
+ long winMetafileSize = leis.readUInt();
wmfData = IOUtils.safelyAllocate(winMetafileSize, getMaxRecordLength(),
"HemfComment.setMaxRecordLength()");
diff --git a/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfDraw.java b/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfDraw.java
index 440f8931a2..6b8ed94c57 100644
--- a/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfDraw.java
+++ b/poi-scratchpad/src/main/java/org/apache/poi/hemf/record/emf/HemfDraw.java
@@ -160,8 +160,8 @@ public final class HemfDraw {
*
* Any extra points MUST be ignored.
*/
- final int count = Math.toIntExact(leis.readUInt());
- final int points = Math.min(count, 16384);
+ final long count = leis.readUInt();
+ final int points = (int)Math.min(count, 16384);
size += LittleEndianConsts.INT_SIZE;
poly = new Path2D.Double(Path2D.WIND_EVEN_ODD, points+2);
@@ -292,8 +292,8 @@ public final class HemfDraw {
long size = readRectL(leis, bounds);
// see PolyBezier about limits
- final int count = Math.toIntExact(leis.readUInt());
- final int points = Math.min(count, 16384);
+ final long count = leis.readUInt();
+ final int points = (int)Math.min(count, 16384);
size += LittleEndianConsts.INT_SIZE;
poly = new Path2D.Double(Path2D.WIND_EVEN_ODD, points);
@@ -962,9 +962,10 @@ public final class HemfDraw {
@Override
public long init(LittleEndianInputStream leis, long recordSize, long recordId) throws IOException {
long size = readRectL(leis, bounds);
- int count = Math.toIntExact(leis.readUInt());
+ long countLong = leis.readUInt();
size += LittleEndianConsts.INT_SIZE;
- HemfPicture.safelyAllocateCheck(count);
+ HemfPicture.safelyAllocateCheck(countLong);
+ int count = (int)countLong;
Point2D[] points = new Point2D[count];
for (int i=0; i<count; i++) {
points[i] = new Point2D.Double();
diff --git a/poi-scratchpad/src/test/java/org/apache/poi/hemf/usermodel/TestHemfPicture.java b/poi-scratchpad/src/test/java/org/apache/poi/hemf/usermodel/TestHemfPicture.java
index dd4699c2d3..374e21059c 100644
--- a/poi-scratchpad/src/test/java/org/apache/poi/hemf/usermodel/TestHemfPicture.java
+++ b/poi-scratchpad/src/test/java/org/apache/poi/hemf/usermodel/TestHemfPicture.java
@@ -51,6 +51,7 @@ import org.apache.poi.hwmf.usermodel.HwmfEmbedded;
import org.apache.poi.hwmf.usermodel.HwmfEmbeddedType;
import org.apache.poi.hwmf.usermodel.HwmfPicture;
import org.apache.poi.util.IOUtils;
+import org.apache.poi.util.LittleEndian;
import org.apache.poi.util.LittleEndianInputStream;
import org.apache.poi.util.LittleEndianOutputStream;
import org.apache.poi.util.RecordFormatException;
@@ -277,6 +278,63 @@ public class TestHemfPicture {
}
}
+ /**
+ * A Count field is an unsigned 32-bit value, so it has to be bounds checked while it is
+ * still a long. Narrowing it first made the conversion fail before the check could run.
+ */
+ @Test
+ void testEmfPolyDrawCountAboveIntMax() throws Exception {
+ HemfDraw.EmfPolyDraw record = new HemfDraw.EmfPolyDraw();
+ byte[] data = new byte[20];
+ LittleEndian.putUInt(data, 16, 0xFFFFFFFFL);
+ try (LittleEndianInputStream leis = new LittleEndianInputStream(new ByteArrayInputStream(data))) {
+ assertThrows(RecordFormatException.class,
+ () -> record.init(leis, data.length, HemfRecordType.polyDraw.id));
+ }
+ }
+
+ /**
+ * EMR_POLYGON and EMR_POLYBEZIER cap the point count at 16K and ignore the remainder, so a
+ * count above Integer.MAX_VALUE has to be clamped rather than rejected.
+ */
+ @Test
+ void testEmfPolygonCountAboveIntMaxIsClamped() throws Exception {
+ assertPointCountClamped(new HemfDraw.EmfPolygon(), HemfRecordType.polygon.id);
+ assertPointCountClamped(new HemfDraw.EmfPolyBezier(), HemfRecordType.polyBezier.id);
+ }
+
+ private static void assertPointCountClamped(HemfRecord record, long recordId) throws Exception {
+ // 16 bytes of bounds, a 4 byte count, then room for the 16K points the cap allows
+ byte[] data = new byte[16 + 4 + 16384 * 8];
+ LittleEndian.putUInt(data, 16, 0xFFFFFFFFL);
+ try (LittleEndianInputStream leis = new LittleEndianInputStream(new ByteArrayInputStream(data))) {
+ long size = record.init(leis, data.length, recordId);
+ assertTrue(size <= data.length, "read " + size + " bytes of a " + data.length + " byte record");
+ }
+ }
+
+ @Test
+ void testEmfCommentWmfSizeAboveIntMax() throws Exception {
+ HemfComment.EmfCommentDataWMF record = new HemfComment.EmfCommentDataWMF();
+ byte[] data = new byte[20];
+ LittleEndian.putUInt(data, 0, HemfComment.HemfCommentRecordType.emfWMF.id & 0xFFFFFFFFL);
+ LittleEndian.putUInt(data, 16, 0xFFFFFFFFL);
+ try (LittleEndianInputStream leis = new LittleEndianInputStream(new ByteArrayInputStream(data))) {
+ assertThrows(RecordFormatException.class, () -> record.init(leis, data.length));
+ }
+ }
+
+ @Test
+ void testEmfCommentBeginGroupDescriptionAboveIntMax() throws Exception {
+ HemfComment.EmfCommentDataBeginGroup record = new HemfComment.EmfCommentDataBeginGroup();
+ byte[] data = new byte[24];
+ LittleEndian.putUInt(data, 0, HemfComment.HemfCommentRecordType.emfBeginGroup.id & 0xFFFFFFFFL);
+ LittleEndian.putUInt(data, 20, 0xFFFFFFFFL);
+ try (LittleEndianInputStream leis = new LittleEndianInputStream(new ByteArrayInputStream(data))) {
+ assertThrows(RecordFormatException.class, () -> record.init(leis, data.length));
+ }
+ }
+
@Test
void nestedWmfEmf() throws Exception {
try (InputStream is = sl_samples.openResourceAsStream("nested_wmf.emf")) {
diff --git a/poi/src/main/java/org/apache/poi/hpsf/Section.java b/poi/src/main/java/org/apache/poi/hpsf/Section.java
index 26ad00cdd7..094ef3820a 100644
--- a/poi/src/main/java/org/apache/poi/hpsf/Section.java
+++ b/poi/src/main/java/org/apache/poi/hpsf/Section.java
@@ -819,12 +819,13 @@ public class Section {
/* Read the string - Strip 0x00 characters from the end of the string. */
int cp = (codepage == -1) ? Property.DEFAULT_CODEPAGE : codepage;
- int nrBytes = Math.toIntExact(((sLength-1) * (cp == CodePageUtil.CP_UNICODE ? 2 : 1)));
- if (nrBytes > 0xFFFFFF) {
+ long nrBytesLong = (sLength-1) * (cp == CodePageUtil.CP_UNICODE ? 2 : 1);
+ if (nrBytesLong > 0xFFFFFF) {
LOG.atWarn().log(errMsg);
isCorrupted = true;
break;
}
+ int nrBytes = (int)nrBytesLong;
try {
byte[] buf = IOUtils.safelyAllocate(nrBytes, CodePageString.getMaxRecordLength(),
diff --git a/poi/src/test/java/org/apache/poi/hpsf/TestOverflowHardening.java b/poi/src/test/java/org/apache/poi/hpsf/TestOverflowHardening.java
index 299200b09d..76f1c036f7 100644
--- a/poi/src/test/java/org/apache/poi/hpsf/TestOverflowHardening.java
+++ b/poi/src/test/java/org/apache/poi/hpsf/TestOverflowHardening.java
@@ -16,8 +16,11 @@
==================================================================== */
package org.apache.poi.hpsf;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import org.apache.poi.hpsf.wellknown.PropertyIDMap;
import org.apache.poi.util.LittleEndian;
import org.apache.poi.util.LittleEndianByteArrayInputStream;
import org.apache.poi.util.RecordFormatException;
@@ -76,4 +79,30 @@ class TestOverflowHardening {
Array a = new Array();
assertThrows(RecordFormatException.class, () -> a.read(lei));
}
+
+ /**
+ * A dictionary entry's length is an unsigned 32-bit field, so the byte count derived from it
+ * has to be range checked while it is still a long. Narrowing first made the conversion throw
+ * {@link ArithmeticException} out of the {@code Section} constructor, bypassing the
+ * corrupted-dictionary path that the oversize guard exists to take.
+ */
+ @Test
+ void dictionaryEntryLengthAboveIntMaxIsTreatedAsCorrupt() {
+ // ClassID (16) + section offset (4), then the section itself: size, property count,
+ // one property entry pointing at the dictionary, and a dictionary holding a single
+ // entry whose length is too large to narrow to an int
+ byte[] data = new byte[48];
+ LittleEndian.putUInt(data, 16, 20); // offset of the section
+ LittleEndian.putUInt(data, 20, 28); // section size
+ LittleEndian.putUInt(data, 24, 1); // property count
+ LittleEndian.putUInt(data, 28, PropertyIDMap.PID_DICTIONARY); // property id
+ LittleEndian.putUInt(data, 32, 16); // property offset
+ LittleEndian.putUInt(data, 36, 1); // dictionary entry count
+ LittleEndian.putUInt(data, 40, 1); // entry key
+ LittleEndian.putUInt(data, 44, 0xFFFFFFFFL); // entry length
+
+ Section s = assertDoesNotThrow(() -> new Section(data, 0));
+ assertTrue(s.getDictionary() == null || s.getDictionary().isEmpty(),
+ "a dictionary entry with a bogus length should be discarded, not returned");
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@poi.apache.org
For additional commands, e-mail: commits-help@poi.apache.org