[bug-66245] HWPF: no ArrayIndexOutOfBoundsException from truncated SPRMs

poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/SectionSprmUncompressor.java

@@ -46,7 +46,14 @@ public static SectionProperties uncompressSEP(byte[] grpprl, int offset)
     while (sprmIt.hasNext())
     {
       SprmOperation sprm = sprmIt.next();
-      unCompressSEPOperation(newProperties, sprm);
+      try
+      {
+        unCompressSEPOperation(newProperties, sprm);
+      }
+      catch (Exception exc)
+      {
+        LOG.atError().withThrowable(exc).log("Unable to apply SPRM operation '{}'", box(sprm.getOperation()));
+      }
     }
 
     return newProperties;

poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/SprmOperation.java

@@ -19,19 +19,25 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 
 import java.util.Arrays;
 
+import org.apache.logging.log4j.Logger;
+import org.apache.poi.logging.PoiLogManager;
 import org.apache.poi.util.BitField;
 import org.apache.poi.util.BitFieldFactory;
 import org.apache.poi.util.Internal;
 import org.apache.poi.util.LittleEndian;
 import org.apache.poi.util.LittleEndianConsts;
 
+import static org.apache.logging.log4j.util.Unbox.box;
+
 /**
  * This class is used to represent a sprm operation from a Word 97/2000/XP
  * document.
  */
 @Internal(since="3.8 beta 4")
 public final class SprmOperation
 {
+    private static final Logger LOG = PoiLogManager.getLogger(SprmOperation.class);
+
     private static final BitField BITFIELD_OP = BitFieldFactory
             .getInstance( 0x1ff );
     private static final BitField BITFIELD_SIZECODE = BitFieldFactory
@@ -104,13 +110,18 @@ public int getOperand()
         case 3:
             return LittleEndian.getInt( _grpprl, _gOffset );
         case 6:
+            if ( _gOffset + 1 >= _grpprl.length )
+            {
+                // truncated SPRM, no operand present
+                return 0;
+            }
             // surely shorter than an int...
-            byte operandLength = _grpprl[_gOffset + 1];
+            int operandLength = Math.min( _grpprl[_gOffset + 1], LittleEndianConsts.INT_SIZE );
 
             // initialized to zeros by JVM
             byte[] codeBytes = new byte[LittleEndianConsts.INT_SIZE];
             for ( int i = 0; i < operandLength; i++ )
-                if ( _gOffset + i < _grpprl.length )
+                if ( _gOffset + 1 + i < _grpprl.length )
                     codeBytes[i] = _grpprl[_gOffset + 1 + i];
 
             return LittleEndian.getInt( codeBytes, 0 );
@@ -169,11 +180,19 @@ private int initSize( short sprm )
             int offset = _gOffset;
             if ( sprm == SPRM_LONG_TABLE || sprm == SPRM_LONG_PARAGRAPH )
             {
+                if ( offset + LittleEndianConsts.SHORT_SIZE > _grpprl.length )
+                {
+                    return truncatedSize( sprm );
+                }
                 int retVal = ( 0x0000ffff &
                         LittleEndian.getShort( _grpprl, offset ) ) + 3;
                 _gOffset += 2;
                 return retVal;
             }
+            if ( offset >= _grpprl.length )
+            {
+                return truncatedSize( sprm );
+            }
             return ( 0x000000ff & _grpprl[_gOffset++] ) + 3;
         case 7:
             return 5;
@@ -183,6 +202,19 @@ private int initSize( short sprm )
         }
     }
 
+    /**
+     * A variable-length SPRM whose operand-length byte(s) lie beyond the end
+     * of the grpprl: the buffer is truncated (or padded with junk), so treat
+     * whatever is left as the size of this SPRM rather than throwing an
+     * {@link ArrayIndexOutOfBoundsException}. The iterator then stops here.
+     */
+    private int truncatedSize( short sprm )
+    {
+        LOG.atWarn().log( "SPRM 0x{} at offset {} is truncated, grpprl length is {}",
+                Integer.toHexString( sprm & 0xffff ), box(_offset), box(_grpprl.length) );
+        return _grpprl.length - _offset;
+    }
+
     public int size()
     {
         return _size;

poi-scratchpad/src/main/java/org/apache/poi/hwpf/sprm/TableSprmUncompressor.java

@@ -41,10 +41,9 @@ public static TableProperties uncompressTAP( SprmBuffer sprmBuffer ) {
         }
 
         SprmOperation sprmOperation = sprmBuffer.findSprm( (short) 0xd608 );
-        if ( sprmOperation != null ) {
-            byte[] grpprl = sprmOperation.getGrpprl();
-            int offset = sprmOperation.getGrpprlOffset();
-            short itcMac = grpprl[offset];
+        byte[] grpprl = sprmOperation == null ? null : sprmOperation.getGrpprl();
+        if ( grpprl != null && sprmOperation.getGrpprlOffset() < grpprl.length ) {
+            short itcMac = grpprl[sprmOperation.getGrpprlOffset()];
             tableProperties = new TableProperties( itcMac );
         } else {
             LOG.atWarn().log("Some table rows didn't specify number of columns in SPRMs");

poi-scratchpad/src/test/java/org/apache/poi/hwpf/sprm/TestSprmOperation.java

@@ -0,0 +1,126 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hwpf.sprm;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.poi.hwpf.usermodel.ParagraphProperties;
+import org.apache.poi.hwpf.usermodel.SectionProperties;
+import org.apache.poi.hwpf.usermodel.TableProperties;
+import org.junit.jupiter.api.Test;
+
+class TestSprmOperation {
+
+    /**
+     * Bug 66245: a variable-length SPRM (size code 6) at the very end of a
+     * grpprl, with no room for its operand-length byte, must not throw an
+     * {@link ArrayIndexOutOfBoundsException}. It consumes the remaining bytes
+     * so that the iterator terminates.
+     */
+    @Test
+    void truncatedVariableLengthSprm() {
+        // sprmPJc (0x2403, 1 byte operand) followed by a size-code-6 PAP sprm
+        // opcode (0xC61D) with no length byte
+        byte[] grpprl = { 0x03, 0x24, 0x01, 0x1D, (byte) 0xC6 };
+
+        SprmIterator it = new SprmIterator(grpprl, 0);
+        assertTrue(it.hasNext());
+        SprmOperation first = it.next();
+        assertEquals(3, first.size());
+        assertEquals(1, first.getOperand());
+
+        assertTrue(it.hasNext());
+        SprmOperation truncated = it.next();
+        assertEquals(6, truncated.getSizeCode());
+        assertEquals(2, truncated.size());
+        assertFalse(it.hasNext());
+
+        ParagraphProperties pap = ParagraphSprmUncompressor.uncompressPAP(
+                new ParagraphProperties(), grpprl, 0);
+        assertNotNull(pap);
+        assertEquals(1, pap.getJc());
+    }
+
+    /**
+     * Same as above for sprmPChgTabs (0xC615), whose operand length is a
+     * two-byte value: only one of the two bytes is present.
+     */
+    @Test
+    void truncatedLongParagraphSprm() {
+        byte[] grpprl = { 0x03, 0x24, 0x01, 0x15, (byte) 0xC6, 0x10 };
+
+        SprmIterator it = new SprmIterator(grpprl, 0);
+        assertEquals(3, it.next().size());
+        assertTrue(it.hasNext());
+        SprmOperation truncated = it.next();
+        assertEquals(3, truncated.size());
+        assertFalse(it.hasNext());
+
+        ParagraphProperties pap = ParagraphSprmUncompressor.uncompressPAP(
+                new ParagraphProperties(), grpprl, 0);
+        assertEquals(1, pap.getJc());
+    }
+
+    /**
+     * The operand of a variable-length SPRM must not be read past the end of
+     * the grpprl, nor past the 4 bytes that fit into an int.
+     */
+    @Test
+    void variableLengthOperandBounds() {
+        // length byte says 2, but the "operand length" the code reads from the
+        // second operand byte claims 0x7F bytes: only what is there is used
+        byte[] grpprl = { 0x1D, (byte) 0xC6, 0x02, 0x05, 0x7F };
+        SprmOperation sprm = new SprmOperation(grpprl, 0);
+        assertEquals(5, sprm.size());
+        assertEquals(0x7F, sprm.getOperand());
+
+        // no operand at all
+        SprmOperation truncated = new SprmOperation(new byte[] { 0x1D, (byte) 0xC6 }, 0);
+        assertEquals(2, truncated.size());
+        assertEquals(0, truncated.getOperand());
+        assertNotNull(truncated.toString());
+    }
+
+    /**
+     * A SEPX whose last SPRM is missing part of its operand: the preceding
+     * SPRMs are still applied, the broken one is logged and skipped.
+     */
+    @Test
+    void truncatedSectionSprm() {
+        // sprmSCnsPgn (0x3000) = 1, then a 2-byte-operand SEP sprm (0x5001)
+        // with only one operand byte
+        byte[] grpprl = { 0x00, 0x30, 0x01, 0x01, 0x50, 0x02 };
+        SectionProperties sep = SectionSprmUncompressor.uncompressSEP(grpprl, 0);
+        assertEquals(1, sep.getCnsPgn());
+    }
+
+    /**
+     * A TAPX whose sprmTDefTable (0xD608) declares an operand that is not
+     * actually present.
+     */
+    @Test
+    void truncatedTableDefinitionSprm() {
+        byte[] grpprl = { 0x00, 0x00, 0x08, (byte) 0xD6, 0x01, 0x00 };
+        TableProperties tap = TableSprmUncompressor.uncompressTAP(new SprmBuffer(grpprl, 2));
+        assertNotNull(tap);
+        assertEquals(1, tap.getItcMac());
+    }
+}