TIKA-4812: address review feedback on media robustness

Author: tballison

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java

@@ -670,8 +670,12 @@ protected RawTagIterator(int nameLength, int sizeLength, int sizeMultiplier,
         }
 
         public boolean hasNext() {
-            // Check for padding at the end
-            return offset < data.length && data[offset] != 0;
+            // Stop at padding, and at a truncated tail too short for a full frame
+            // header: the RawTag constructor reads the header bytes unconditionally,
+            // so without the data.length no longer being zero-padded (TIKA-4812) a
+            // partial header would throw ArrayIndexOutOfBoundsException.
+            return offset + nameLength + sizeLength + flagLength <= data.length
+                    && data[offset] != 0;
         }
 
         public RawTag next() {

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/MP4Parser.java

@@ -125,8 +125,11 @@ public void parse(TikaInputStream tis, ContentHandler handler, Metadata metadata
         Mp4BoxHandler boxHandler = new TikaMp4BoxHandler(mp4Metadata, metadata, xhtml, context);
         //we used to spool to disk and then read from that with sannies parser.
         //we think that drewnoakes' parser streams the data so we don't need to spool
+        //when the length is known (file-backed), pass it so a box that claims more than
+        //the input holds is skipped rather than allocated
+        long inputLength = tis.hasLength() ? tis.getLength() : -1;
         try {
-            TikaMp4Reader.extract(tis, boxHandler, maxBoxSize);
+            TikaMp4Reader.extract(tis, boxHandler, maxBoxSize, inputLength);
         } catch (RuntimeSAXException e) {
             throw (SAXException) e.getCause();
         }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/TikaMp4Reader.java

@@ -43,53 +43,76 @@ final class TikaMp4Reader {
     private TikaMp4Reader() {
     }
 
-    static void extract(InputStream inputStream, Mp4BoxHandler handler, long maxBoxSize) {
+    //MP4 containers nest (moov/trak/mdia/minf/stbl/udta/meta); cap the recursion so a
+    //crafted chain of nested container headers cannot overflow the stack (an uncaught
+    //Error, caught by neither the IOException handler below nor CompositeParser). Real
+    //files nest well under this.
+    private static final int MAX_BOX_DEPTH = 100;
+
+    /**
+     * @param inputLength total input length in bytes, or -1 if unknown. When known, a box
+     *                    that declares more payload than the input holds is skipped rather
+     *                    than allocated (StreamReader.getBytes allocates before reading).
+     */
+    static void extract(InputStream inputStream, Mp4BoxHandler handler, long maxBoxSize,
+                        long inputLength) {
         StreamReader reader = new StreamReader(inputStream);
         reader.setMotorolaByteOrder(true);
-        processBoxes(reader, -1, handler, new Mp4Context(), maxBoxSize);
+        processBoxes(reader, -1, handler, new Mp4Context(), maxBoxSize, inputLength, 0);
     }
 
     private static void processBoxes(StreamReader reader, long atomEnd, Mp4Handler<?> handler,
-                                     Mp4Context context, long maxBoxSize) {
+                                     Mp4Context context, long maxBoxSize, long inputLength,
+                                     int depth) {
+        if (depth > MAX_BOX_DEPTH) {
+            handler.addError("MP4 box nesting exceeds the maximum depth of " + MAX_BOX_DEPTH);
+            return;
+        }
         try {
             while (atomEnd == -1 || reader.getPosition() < atomEnd) {
                 long boxSize = reader.getUInt32();
                 String boxType = reader.getString(4);
-                boolean isLargeSize = boxSize == 1;
-                if (isLargeSize) {
+                //4 bytes size + 4 bytes type, plus 8 more when a 64-bit largesize follows
+                int headerSize = boxSize == 1 ? 16 : 8;
+                if (headerSize == 16) {
                     boxSize = reader.getInt64();
                 }
                 if (boxSize > Integer.MAX_VALUE) {
                     handler.addError("Box size too large.");
                     break;
                 }
-                if (boxSize < 8) {
+                if (boxSize < headerSize) {
                     handler.addError("Box size too small.");
                     break;
                 }
 
+                long payloadLength = boxSize - headerSize;
                 if (acceptContainer(handler, boxType)) {
-                    processBoxes(reader, boxSize + reader.getPosition() - 8,
+                    processBoxes(reader, reader.getPosition() + payloadLength,
                             processBox(handler, boxType, null, boxSize, context), context,
-                            maxBoxSize);
+                            maxBoxSize, inputLength, depth + 1);
                 } else if (acceptBox(handler, boxType)) {
-                    long payloadLength = boxSize - 8;
-                    if (payloadLength > maxBoxSize) {
+                    //StreamReader.getBytes allocates the whole payload up front, so skip
+                    //(a lazy stream advance) any box over the cap, or one that claims more
+                    //than the input holds, instead of allocating it. Skip-and-continue is
+                    //deliberate: unlike the TikaMemoryLimitException other parsers throw,
+                    //this keeps the remaining boxes' metadata; the skip is recorded as a
+                    //warning via the directory's error list.
+                    boolean tooLarge = payloadLength > maxBoxSize;
+                    boolean beyondInput = inputLength >= 0
+                            && reader.getPosition() + payloadLength > inputLength;
+                    if (tooLarge || beyondInput) {
                         handler.addError("MP4 box '" + boxType + "' payload (" + payloadLength
-                                + " bytes) exceeds the maximum of " + maxBoxSize
-                                + " bytes; skipping.");
+                                + " bytes) exceeds the "
+                                + (tooLarge ? "maximum of " + maxBoxSize + " bytes" : "input size")
+                                + "; skipping.");
                         reader.skip(payloadLength);
                     } else {
                         handler = processBox(handler, boxType,
                                 reader.getBytes((int) payloadLength), boxSize, context);
                     }
-                } else if (isLargeSize) {
-                    if (boxSize < 16) {
-                        break;
-                    }
-                    reader.skip(boxSize - 16);
                 } else {
-                    reader.skip(boxSize - 8);
+                    reader.skip(payloadLength);
                 }
             }
         } catch (IOException e) {

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/TikaMp4SoundHandler.java

@@ -79,7 +79,7 @@ private void extractFromSampleDescriptions(byte[] b) {
                 //sample entry: 8 byte header, 6 reserved, 2 data ref index,
                 //then version-dependent fixed sound fields before child boxes
                 int version = EndianUtils.getUShortBE(b, pos + 16);
-                int bitRate = findEsdsAverageBitRate(b, pos + soundEntrySize(version), end);
+                int bitRate = findEsdsAverageBitRate(b, pos + soundEntrySize(version), end, 0);
                 if (bitRate > 0) {
                     tikaMetadata.set(Audio.BITRATE, bitRate);
                 }
@@ -114,10 +114,6 @@ private static int soundEntrySize(int version) {
      * its average bitrate, or 0 if there is none. QuickTime version 1/2
      * entries may nest the 'esds' inside a 'wave' extension box.
      */
-    private static int findEsdsAverageBitRate(byte[] b, int pos, int end) {
-        return findEsdsAverageBitRate(b, pos, end, 0);
-    }
-
     private static int findEsdsAverageBitRate(byte[] b, int pos, int end, int depth) {
         if (depth > MAX_BOX_DEPTH) {
             return 0;

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/boxes/TikaUserDataBox.java

@@ -115,28 +115,25 @@ private void parseUserDataBox(SequentialReader reader, String handlerType,
         int toSkip = lengthToStartOfList - read;
         reader.skip(toSkip);
         long len = reader.getUInt32();
-        if (len >= Integer.MAX_VALUE || len <= 0) {
-            //log
-            return;
-        }
         String subType = reader.getString(4, StandardCharsets.ISO_8859_1);
-        //this handles "free" types...not sure if there are others?
-        //will throw IOException if no ilist is found
-        while (! subType.equals(ILST)) {
-            //re-validate each re-read length: len < 8 makes skip(len - 8) negative,
-            //which throws IllegalArgumentException (not IOException, so it escapes
-            //MP4Reader). See TIKA-4812.
+        //walk the "free"-style sub-boxes to the ilst, validating each declared length
+        //once before it is used: a length below the 8-byte header would make skip(len - 8)
+        //negative (an IllegalArgumentException that escapes MP4Reader), and an oversize one
+        //would desync the walk. Raise a caught IOException instead, so the udta walk aborts
+        //and the problem is recorded as a parse error rather than silently mis-read. It
+        //also throws (EOFException) if no ilst is found. See TIKA-4812.
+        while (! ILST.equals(subType)) {
             if (len < 8L || len >= Integer.MAX_VALUE) {
-                return;
+                throw new IOException("Malformed box length in udta metadata: " + len);
             }
             reader.skip(len - 8);
             len = reader.getUInt32();
             subType = reader.getString(4, StandardCharsets.ISO_8859_1);
         }
-        if (ILST.equals(subType)) {
-            processIList(reader, len);
+        if (len < 8L || len >= Integer.MAX_VALUE) {
+            throw new IOException("Malformed ilst length in udta metadata: " + len);
         }
-
+        processIList(reader, len);
     }
 
 
@@ -146,104 +143,83 @@ private void processIList(SequentialReader reader, long totalLen)
 
         long totalRead = 0;
         while (totalRead < totalLen) {
+            long recordStart = reader.getPosition();
             long recordLen = reader.getUInt32();
+            if (recordLen < 16) {
+                //malformed record header; stop rather than loop or skip a negative span
+                return;
+            }
             String fieldName = reader.getString(4, StandardCharsets.ISO_8859_1);
             long fieldLen = reader.getUInt32();
             String typeName = reader.getString(4, StandardCharsets.ISO_8859_1);//data
-            totalRead += 16;
+            long recordEnd = recordStart + recordLen;
             if ("data".equals(typeName)) {
-                //1 byte version and 3 bytes flags; for the "well-known"
-                //types the flags hold the value type
+                //1 byte version and 3 bytes flags; for the "well-known" types the
+                //flags hold the value type
                 long valueType = reader.getUInt32() & 0xFFFFFF;
                 reader.skip(4L);//locale
-                totalRead += 8;
                 int toRead = (int) fieldLen - 16;
-                if (toRead <= 0) {
-                    //log?
-                    return;
-                }
-                if ("covr".equals(fieldName)) {
-                    //covr holds one image file (e.g. png or jpeg) per data
-                    //atom, and may repeat the data atom for further images
-                    handleCoverArt(reader, valueType, toRead);
-                    long remaining = recordLen - 8 - fieldLen;
-                    while (remaining >= 16) {
-                        long extraLen = reader.getUInt32();
-                        String extraTypeName = reader.getString(4, StandardCharsets.ISO_8859_1);
-                        long extraValueType = reader.getUInt32() & 0xFFFFFF;
-                        reader.skip(4L);//locale
-                        totalRead += 16;
-                        remaining -= 16;
-                        int extraToRead = (int) extraLen - 16;
-                        if (!"data".equals(extraTypeName) || extraToRead <= 0 ||
-                                extraToRead > remaining) {
-                            //malformed, skip the rest of the record
-                            break;
+                if (toRead > 0) {
+                    if ("covr".equals(fieldName)) {
+                        //covr holds one image per data atom and may repeat the data atom
+                        //for further images; the realign below consumes any leftover
+                        handleCoverArt(reader, valueType, toRead);
+                        while (reader.getPosition() + 16 <= recordEnd) {
+                            long extraLen = reader.getUInt32();
+                            String extraType =
+                                    reader.getString(4, StandardCharsets.ISO_8859_1);
+                            long extraValueType = reader.getUInt32() & 0xFFFFFF;
+                            reader.skip(4L);//locale
+                            int extraToRead = (int) extraLen - 16;
+                            if (!"data".equals(extraType) || extraToRead <= 0
+                                    || reader.getPosition() + extraToRead > recordEnd) {
+                                break;
+                            }
+                            handleCoverArt(reader, extraValueType, extraToRead);
                         }
-                        handleCoverArt(reader, extraValueType, extraToRead);
-                        totalRead += extraToRead;
-                        remaining -= extraToRead;
-                    }
-                    if (remaining > 0) {
-                        reader.skip(remaining);
-                        totalRead += remaining;
-                    }
-                } else if ("cpil".equals(fieldName)) {
-                    int compilationId = (int)reader.getByte();
-                    metadata.set(XMPDM.COMPILATION, compilationId);
-                    //consume the rest of the declared field: totalRead counts toRead,
-                    //but only 1 byte was read, so skip the remainder to stay aligned
-                    if (toRead > 1) {
-                        reader.skip(toRead - 1L);
-                    }
-                } else if ("trkn".equals(fieldName)) {
-                    if (toRead == 8) {
-                        long numA = reader.getUInt32();
-                        long numB = reader.getUInt32();
-                        metadata.set(XMPDM.TRACK_NUMBER, (int)numA);
-                        //2 bytes track total, 2 bytes reserved
-                        int trackCount = (int) (numB >>> 16);
-                        if (trackCount > 0) {
-                            metadata.set(Audio.TRACK_COUNT, trackCount);
+                    } else if ("cpil".equals(fieldName)) {
+                        metadata.set(XMPDM.COMPILATION, (int) reader.getByte());
+                    } else if ("trkn".equals(fieldName)) {
+                        if (toRead >= 8) {
+                            long numA = reader.getUInt32();
+                            long numB = reader.getUInt32();
+                            metadata.set(XMPDM.TRACK_NUMBER, (int) numA);
+                            //2 bytes track total, 2 bytes reserved
+                            int trackCount = (int) (numB >>> 16);
+                            if (trackCount > 0) {
+                                metadata.set(Audio.TRACK_COUNT, trackCount);
+                            }
                         }
-                    } else {
-                        //log
-                        reader.skip(toRead);
-                    }
-                } else if ("disk".equals(fieldName)) {
-                    //2 bytes reserved, 2 bytes disc, 2 bytes total; some encoders
-                    //pad to 8 bytes like trkn, so consume exactly toRead either way
-                    if (toRead >= 6) {
-                        int a = reader.getInt32();
-                        short b = reader.getInt16();
-                        metadata.set(XMPDM.DISC_NUMBER, a);
-                        if (b > 0) {
-                            metadata.set(Audio.DISC_COUNT, b);
+                    } else if ("disk".equals(fieldName)) {
+                        if (toRead >= 6) {
+                            //2 bytes reserved, 2 bytes disc, 2 bytes total
+                            int a = reader.getInt32();
+                            short b = reader.getInt16();
+                            metadata.set(XMPDM.DISC_NUMBER, a);
+                            if (b > 0) {
+                                metadata.set(Audio.DISC_COUNT, b);
+                            }
                         }
-                        reader.skip(toRead - 6);
                     } else {
-                        reader.skip(toRead);
-                    }
-                } else {
-                    String val = reader.getString(toRead, StandardCharsets.UTF_8);
-                    try {
-                        addMetadata(fieldName, val);
-                    } catch (SAXException e) {
-                        //need to punch through IOException catching in MP4Reader
-                        throw new RuntimeSAXException(e);
+                        String val = reader.getString(toRead, StandardCharsets.UTF_8);
+                        try {
+                            addMetadata(fieldName, val);
+                        } catch (SAXException e) {
+                            //need to punch through IOException catching in MP4Reader
+                            throw new RuntimeSAXException(e);
+                        }
                     }
                 }
-
-                totalRead += toRead;
-            } else {
-                int toSkip = (int) recordLen - 16;
-                if (toSkip <= 0) {
-                    //log?
-                    return;
-                }
-                reader.skip(toSkip);
-                totalRead += toSkip;
             }
+            //realign to the end of the record regardless of what the branch consumed, so a
+            //trailing sub-atom (e.g. a 'name' atom after 'data') can't desync the walk
+            long pos = reader.getPosition();
+            if (pos > recordEnd) {
+                //a branch read past the record end (malformed lengths); stop
+                return;
+            }
+            reader.skip(recordEnd - pos);
+            totalRead += recordLen;
         }
     }
 

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/video/FLVParser.java

@@ -100,6 +100,12 @@ private int readUInt24(DataInputStream input) throws IOException {
     //blob of deeply nested containers cannot overflow the stack (an uncaught Error)
     private static final int MAX_AMF_DEPTH = 64;
 
+    //cap the declared element count of an AMF array: it is a 32-bit field (up to
+    //~4 billion), and each element grows a collection, so a crafted count backed by
+    //cheap 1-byte elements would exhaust memory before EOF. Legitimate onMetaData
+    //arrays are tiny; this only bounds the crafted case (a short read still throws).
+    private static final int MAX_AMF_ELEMENTS = 100_000;
+
     Object readAMFData(DataInputStream input, int type) throws IOException {
         return readAMFData(input, type, 0);
     }
@@ -137,6 +143,10 @@ private Object readAMFData(DataInputStream input, int type, int depth) throws IO
 
     private Object readAMFStrictArray(DataInputStream input, int depth) throws IOException {
         long count = readUInt32(input);
+        if (count > MAX_AMF_ELEMENTS) {
+            throw new IOException("AMF array count " + count + " exceeds the maximum of "
+                    + MAX_AMF_ELEMENTS);
+        }
         ArrayList<Object> list = new ArrayList<>();
         for (int i = 0; i < count; i++) {
             list.add(readAMFData(input, -1, depth + 1));
@@ -167,6 +177,10 @@ private Object readAMFObject(DataInputStream input, int depth) throws IOExceptio
 
     private Object readAMFEcmaArray(DataInputStream input, int depth) throws IOException {
         long size = readUInt32(input);
+        if (size > MAX_AMF_ELEMENTS) {
+            throw new IOException("AMF array size " + size + " exceeds the maximum of "
+                    + MAX_AMF_ELEMENTS);
+        }
         HashMap<String, Object> array = new HashMap<>();
         for (int i = 0; i < size; i++) {
             String key = readAMFString(input);