TIKA-4812 - improve media file 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

@@ -22,6 +22,7 @@
 import java.io.InputStream;
 import java.io.PushbackInputStream;
 import java.io.UnsupportedEncodingException;
+import java.util.Arrays;
 import java.util.Iterator;
 
 import org.apache.tika.parser.mp3.ID3Tags.ID3Comment;
@@ -180,9 +181,10 @@ protected static byte[] readFully(InputStream inp, int length, boolean shortData
                     throw new IOException("Tried to read " + length + " bytes, but only " + pos +
                             " bytes present");
                 } else {
-                    // Give them what we found
-                    // TODO Log the short read
-                    return b;
+                    // truncated stream: return only the bytes actually read, not the
+                    // zero-padded full-length array, so callers (e.g. cover-art
+                    // extraction) don't emit padding as data. TIKA-4812
+                    return Arrays.copyOf(b, pos);
                 }
             }
             pos += read;
@@ -668,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

@@ -31,7 +31,6 @@
 import java.util.Optional;
 import java.util.Set;
 
-import com.drew.imaging.mp4.Mp4Reader;
 import com.drew.metadata.Directory;
 import com.drew.metadata.MetadataException;
 import com.drew.metadata.mp4.Mp4BoxHandler;
@@ -77,6 +76,14 @@ public class MP4Parser implements Parser {
     private static final MediaType AUDIO_MP4 = MediaType.audio("mp4");
 
     private static final int MAX_ERROR_MESSAGES = 100;
+
+    //an accepted MP4 box whose declared payload exceeds this is skipped rather than
+    //loaded, so a crafted box size cannot force a multi-GB allocation. Cover art and
+    //other legitimate metadata boxes are well under this; configurable if a real file
+    //needs more. See TikaMp4Reader and TIKA-4812.
+    private static final long DEFAULT_MAX_BOX_SIZE = 100L * 1024L * 1024L;
+
+    private long maxBoxSize = DEFAULT_MAX_BOX_SIZE;
     static {
         // All types should be 4 bytes long, space padded as needed
         typesMap.put(MediaType.audio("mp4"), Arrays.asList("M4A ", "M4B ", "F4A ", "F4B "));
@@ -95,6 +102,22 @@ public Set<MediaType> getSupportedTypes(ParseContext context) {
         return SUPPORTED_TYPES;
     }
 
+    /**
+     * The maximum declared payload, in bytes, of an accepted MP4 box that will be
+     * read into memory; larger boxes are skipped. Guards against a crafted box size
+     * forcing a multi-GB allocation.
+     */
+    public long getMaxBoxSize() {
+        return maxBoxSize;
+    }
+
+    public void setMaxBoxSize(long maxBoxSize) {
+        if (maxBoxSize <= 0) {
+            throw new IllegalArgumentException("maxBoxSize must be positive: " + maxBoxSize);
+        }
+        this.maxBoxSize = maxBoxSize;
+    }
+
     public void parse(TikaInputStream tis, ContentHandler handler, Metadata metadata,
                       ParseContext context) throws IOException, SAXException, TikaException {
 
@@ -105,8 +128,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 {
-            Mp4Reader.extract(tis, boxHandler);
+            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

@@ -0,0 +1,149 @@
+/*
+ * 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.tika.parser.mp4;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import com.drew.imaging.mp4.Mp4Handler;
+import com.drew.lang.StreamReader;
+import com.drew.metadata.mp4.Mp4BoxHandler;
+import com.drew.metadata.mp4.Mp4Context;
+import com.drew.metadata.mp4.Mp4MediaHandler;
+
+/**
+ * A size-bounded reimplementation of com.drew.imaging.mp4.Mp4Reader.
+ * <p>
+ * The metadata-extractor reader eagerly does {@code new byte[(int) boxSize - 8]}
+ * for every box a handler accepts, with {@code boxSize} attacker-controlled and
+ * capped only at {@code Integer.MAX_VALUE} (~2GB), and {@code StreamReader.getBytes}
+ * allocates before checking how much data is actually present. A single crafted
+ * box header therefore forces a multi-GB allocation. This reader is identical to
+ * the library's box walk except that an accepted box whose payload exceeds
+ * {@code maxBoxSize} is skipped (a lazy stream advance, no allocation) instead of
+ * being read. Boxes the handler does not accept were already skipped by the
+ * library, so this only bounds the boxes we opt into. See TIKA-4812.
+ */
+final class TikaMp4Reader {
+
+    private TikaMp4Reader() {
+    }
+
+    //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, inputLength, 0);
+    }
+
+    private static void processBoxes(StreamReader reader, long atomEnd, Mp4Handler<?> handler,
+                                     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);
+                //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 < headerSize) {
+                    handler.addError("Box size too small.");
+                    break;
+                }
+
+                long payloadLength = boxSize - headerSize;
+                if (acceptContainer(handler, boxType)) {
+                    processBoxes(reader, reader.getPosition() + payloadLength,
+                            processBox(handler, boxType, null, boxSize, context), context,
+                            maxBoxSize, inputLength, depth + 1);
+                } else if (acceptBox(handler, boxType)) {
+                    //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 "
+                                + (tooLarge ? "maximum of " + maxBoxSize + " bytes" : "input size")
+                                + "; skipping.");
+                        reader.skip(payloadLength);
+                    } else {
+                        handler = processBox(handler, boxType,
+                                reader.getBytes((int) payloadLength), boxSize, context);
+                    }
+                } else {
+                    reader.skip(payloadLength);
+                }
+            }
+        } catch (IOException e) {
+            handler.addError(e.getMessage() == null ? "IOException reading MP4 boxes"
+                    : e.getMessage());
+        }
+    }
+
+    //the box walk holds handlers as Mp4Handler, whose accept/process methods are
+    //protected; every concrete handler in play (Mp4BoxHandler-rooted, or an
+    //Mp4MediaHandler track handler swapped in on 'hdlr') widens them to public,
+    //so dispatch through whichever of the two families the instance belongs to.
+    //A container's handler is obtained with processBox(type, null, ...), which is
+    //exactly what the library's protected processContainer does.
+
+    private static boolean acceptContainer(Mp4Handler<?> handler, String type) {
+        return handler instanceof Mp4BoxHandler
+                ? ((Mp4BoxHandler) handler).shouldAcceptContainer(type)
+                : ((Mp4MediaHandler<?>) handler).shouldAcceptContainer(type);
+    }
+
+    private static boolean acceptBox(Mp4Handler<?> handler, String type) {
+        return handler instanceof Mp4BoxHandler
+                ? ((Mp4BoxHandler) handler).shouldAcceptBox(type)
+                : ((Mp4MediaHandler<?>) handler).shouldAcceptBox(type);
+    }
+
+    private static Mp4Handler<?> processBox(Mp4Handler<?> handler, String type, byte[] payload,
+                                            long boxSize, Mp4Context context) throws IOException {
+        return handler instanceof Mp4BoxHandler
+                ? ((Mp4BoxHandler) handler).processBox(type, payload, boxSize, context)
+                : ((Mp4MediaHandler<?>) handler).processBox(type, payload, boxSize, context);
+    }
+}

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);
                 }
@@ -103,12 +103,21 @@ private static int soundEntrySize(int version) {
         return 36;
     }
 
+    //real files nest 'wave' at most one level; this only bounds crafted input,
+    //where a deep chain of nested 'wave' boxes would otherwise recurse until the
+    //stack overflows (an uncaught Error, not caught by Mp4Reader or CompositeParser).
+    //See TIKA-4812.
+    private static final int MAX_BOX_DEPTH = 10;
+
     /**
      * Scans the child boxes of a sample entry for an 'esds' box and returns
      * 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) {
+    private static int findEsdsAverageBitRate(byte[] b, int pos, int end, int depth) {
+        if (depth > MAX_BOX_DEPTH) {
+            return 0;
+        }
         while (pos >= 0 && pos + 8 <= end) {
             long size = EndianUtils.getUIntBE(b, pos);
             if (size < 8 || size > end - pos) {
@@ -119,7 +128,7 @@ private static int findEsdsAverageBitRate(byte[] b, int pos, int end) {
                 return readEsdsAverageBitRate(b, pos + 8, pos + (int) size);
             }
             if ("wave".equals(type)) {
-                int nested = findEsdsAverageBitRate(b, pos + 8, pos + (int) size);
+                int nested = findEsdsAverageBitRate(b, pos + 8, pos + (int) size, depth + 1);
                 if (nested > 0) {
                     return nested;
                 }