improve media parser 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;

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

@@ -96,7 +96,18 @@ private int readUInt24(DataInputStream input) throws IOException {
         return uint;
     }
 
-    private Object readAMFData(DataInputStream input, int type) throws IOException {
+    //AMF objects/arrays nest recursively; cap the depth so a crafted metadata
+    //blob of deeply nested containers cannot overflow the stack (an uncaught Error)
+    private static final int MAX_AMF_DEPTH = 64;
+
+    Object readAMFData(DataInputStream input, int type) throws IOException {
+        return readAMFData(input, type, 0);
+    }
+
+    private Object readAMFData(DataInputStream input, int type, int depth) throws IOException {
+        if (depth > MAX_AMF_DEPTH) {
+            throw new IOException("AMF nesting exceeds the maximum depth of " + MAX_AMF_DEPTH);
+        }
         if (type == -1) {
             type = input.readUnsignedByte();
         }
@@ -108,11 +119,11 @@ private Object readAMFData(DataInputStream input, int type) throws IOException {
             case 2:
                 return readAMFString(input);
             case 3:
-                return readAMFObject(input);
+                return readAMFObject(input, depth);
             case 8:
-                return readAMFEcmaArray(input);
+                return readAMFEcmaArray(input, depth);
             case 10:
-                return readAMFStrictArray(input);
+                return readAMFStrictArray(input, depth);
             case 11:
                 final Date date = new Date((long) input.readDouble());
                 input.readShort(); // time zone
@@ -124,11 +135,11 @@ private Object readAMFData(DataInputStream input, int type) throws IOException {
         }
     }
 
-    private Object readAMFStrictArray(DataInputStream input) throws IOException {
+    private Object readAMFStrictArray(DataInputStream input, int depth) throws IOException {
         long count = readUInt32(input);
         ArrayList<Object> list = new ArrayList<>();
         for (int i = 0; i < count; i++) {
-            list.add(readAMFData(input, -1));
+            list.add(readAMFData(input, -1, depth + 1));
         }
         return list;
     }
@@ -141,26 +152,26 @@ private String readAMFString(DataInputStream input) throws IOException {
         return new String(chars, UTF_8);
     }
 
-    private Object readAMFObject(DataInputStream input) throws IOException {
+    private Object readAMFObject(DataInputStream input, int depth) throws IOException {
         HashMap<String, Object> array = new HashMap<>();
         while (true) {
             String key = readAMFString(input);
             int dataType = input.read();
             if (dataType == 9) { // object end marker
                 break;
             }
-            array.put(key, readAMFData(input, dataType));
+            array.put(key, readAMFData(input, dataType, depth + 1));
         }
         return array;
     }
 
-    private Object readAMFEcmaArray(DataInputStream input) throws IOException {
+    private Object readAMFEcmaArray(DataInputStream input, int depth) throws IOException {
         long size = readUInt32(input);
         HashMap<String, Object> array = new HashMap<>();
         for (int i = 0; i < size; i++) {
             String key = readAMFString(input);
             int dataType = input.read();
-            array.put(key, readAMFData(input, dataType));
+            array.put(key, readAMFData(input, dataType, depth + 1));
         }
         return array;
     }

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/ID3v2FrameTest.java

@@ -20,9 +20,11 @@
 import static java.nio.charset.StandardCharsets.UTF_16BE;
 import static java.nio.charset.StandardCharsets.UTF_16LE;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 
+import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.nio.charset.Charset;
@@ -48,6 +50,17 @@ public class ID3v2FrameTest {
     private static final String LATIN = "Test Copyright";
     private static final String CJK = "日本語";
 
+    @Test
+    public void testReadFullyTruncatedReturnsActualBytes() throws IOException {
+        //a stream shorter than the declared length must yield only the bytes
+        //present, not a zero-padded full-length array, so a truncated frame's
+        //cover art is not emitted with trailing padding. TIKA-4812
+        byte[] present = "abcdefghij".getBytes(ISO_8859_1); //10 bytes
+        byte[] result = ID3v2Frame.readFully(new ByteArrayInputStream(present), 128, false);
+        assertArrayEquals(present, result);
+        assertEquals(present.length, result.length);
+    }
+
     private static byte[] frame(byte encodingFlag, byte[]... parts) {
         ByteArrayOutputStream out = new ByteArrayOutputStream();
         out.write(encodingFlag);

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/video/FLVParserTest.java

@@ -17,6 +17,12 @@
 package org.apache.tika.parser.video;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataInputStream;
+import java.io.IOException;
 
 import org.junit.jupiter.api.Test;
 
@@ -25,6 +31,22 @@
 
 public class FLVParserTest {
 
+    /**
+     * Deeply nested AMF objects used to recurse in readAMFData until the stack
+     * overflowed (an uncaught Error); the reader must bound the nesting depth.
+     */
+    @Test
+    public void testAmfNestingIsBounded() throws Exception {
+        ByteArrayOutputStream bos = new ByteArrayOutputStream();
+        bos.write(3); //top-level AMF object marker
+        for (int i = 0; i < 100_000; i++) {
+            bos.write(new byte[]{0, 0}); //empty key (uint16 length 0)
+            bos.write(3);                //value type = object -> recurse
+        }
+        DataInputStream dis = new DataInputStream(new ByteArrayInputStream(bos.toByteArray()));
+        assertThrows(IOException.class, () -> new FLVParser().readAMFData(dis, -1));
+    }
+
     @Test
     public void testFLV() throws Exception {
         String path = "/test-documents/testFLV.flv";