dschmidt commented on code in PR #3003: URL: https://github.com/apache/tika/pull/3003#discussion_r3751680984
########## 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,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.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() { + } + + static void extract(InputStream inputStream, Mp4BoxHandler handler, long maxBoxSize) { + StreamReader reader = new StreamReader(inputStream); + reader.setMotorolaByteOrder(true); + processBoxes(reader, -1, handler, new Mp4Context(), maxBoxSize); + } + + private static void processBoxes(StreamReader reader, long atomEnd, Mp4Handler<?> handler, + Mp4Context context, long maxBoxSize) { + try { + while (atomEnd == -1 || reader.getPosition() < atomEnd) { + long boxSize = reader.getUInt32(); + String boxType = reader.getString(4); + boolean isLargeSize = boxSize == 1; + if (isLargeSize) { + boxSize = reader.getInt64(); + } + if (boxSize > Integer.MAX_VALUE) { + handler.addError("Box size too large."); + break; + } + if (boxSize < 8) { + handler.addError("Box size too small."); + break; + } + + if (acceptContainer(handler, boxType)) { Review Comment: Claude says: ``` processBoxes recurses once per accepted container with no depth cap, which is the same StackOverflowError bug class this PR fixes in TikaMp4SoundHandler and FLVParser. Mp4BoxHandler.shouldAcceptContainer accepts moov/trak/mdia/udta/meta by fourCC alone, so a crafted file that is a chain of nested "moov" headers (8 bytes per level) drives one stack frame per level; a few hundred KB of input overflows the default JVM stack. StackOverflowError is an Error, so neither the IOException catch here nor CompositeParser will contain it. Since Tika now owns this walk, threading a depth counter through processBoxes and doing addError+break past a small limit would close it. ``` ########## 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,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.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() { + } + + static void extract(InputStream inputStream, Mp4BoxHandler handler, long maxBoxSize) { + StreamReader reader = new StreamReader(inputStream); + reader.setMotorolaByteOrder(true); + processBoxes(reader, -1, handler, new Mp4Context(), maxBoxSize); + } + + private static void processBoxes(StreamReader reader, long atomEnd, Mp4Handler<?> handler, + Mp4Context context, long maxBoxSize) { + try { + while (atomEnd == -1 || reader.getPosition() < atomEnd) { + long boxSize = reader.getUInt32(); + String boxType = reader.getString(4); + boolean isLargeSize = boxSize == 1; + if (isLargeSize) { + boxSize = reader.getInt64(); + } + if (boxSize > Integer.MAX_VALUE) { + handler.addError("Box size too large."); + break; + } + if (boxSize < 8) { + handler.addError("Box size too small."); + break; + } + + if (acceptContainer(handler, boxType)) { + processBoxes(reader, boxSize + reader.getPosition() - 8, + processBox(handler, boxType, null, boxSize, context), context, + maxBoxSize); + } else if (acceptBox(handler, boxType)) { + long payloadLength = boxSize - 8; Review Comment: Claude says: ``` For 64-bit largesize boxes (boxSize == 1), 16 header bytes have been consumed, but the accepted-box path computes payloadLength = boxSize - 8 and the container path computes atomEnd = boxSize + position - 8; both overrun into the next sibling box by 8 bytes. Only the unaccepted-box branch uses the correct boxSize - 16. This faithfully mirrors a metadata-extractor bug, but now that the walk is Tika-owned it seems worth fixing: track headerSize (8 or 16) and use boxSize - headerSize in all three places. As is, a legal file using largesize encoding gets its remaining boxes misparsed ("Box size too small/large") and silently loses metadata from that point on. ``` ########## 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,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.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() { + } + + static void extract(InputStream inputStream, Mp4BoxHandler handler, long maxBoxSize) { + StreamReader reader = new StreamReader(inputStream); + reader.setMotorolaByteOrder(true); + processBoxes(reader, -1, handler, new Mp4Context(), maxBoxSize); + } + + private static void processBoxes(StreamReader reader, long atomEnd, Mp4Handler<?> handler, + Mp4Context context, long maxBoxSize) { + try { + while (atomEnd == -1 || reader.getPosition() < atomEnd) { + long boxSize = reader.getUInt32(); + String boxType = reader.getString(4); + boolean isLargeSize = boxSize == 1; + if (isLargeSize) { + boxSize = reader.getInt64(); + } + if (boxSize > Integer.MAX_VALUE) { + handler.addError("Box size too large."); + break; + } + if (boxSize < 8) { + handler.addError("Box size too small."); + break; + } + + if (acceptContainer(handler, boxType)) { + processBoxes(reader, boxSize + reader.getPosition() - 8, + processBox(handler, boxType, null, boxSize, context), context, + maxBoxSize); + } else if (acceptBox(handler, boxType)) { + long payloadLength = boxSize - 8; + if (payloadLength > maxBoxSize) { + handler.addError("MP4 box '" + boxType + "' payload (" + payloadLength Review Comment: Claude says: ``` Minor consistency point: other parsers (BPGParser, PSDParser, ICNSParser, AppleSingleFileParser) throw TikaMemoryLimitException when a declared length exceeds a configured max, while this branch records a string in the Mp4Directory error collection (itself capped by MAX_ERROR_MESSAGES) and continues. Skip-and-continue may well be the right call for robustness here, but if so it might deserve a comment documenting the deliberate deviation; otherwise throwing TikaMemoryLimitException would match the convention and make truncated extraction observable to operators. ``` ########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/boxes/TikaUserDataBox.java: ########## @@ -185,6 +191,11 @@ private void processIList(SequentialReader reader, long totalLen) } 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) { Review Comment: Claude says: ``` This cpil fix is correct, but the underlying issue is general: processIList aligns per-branch from fieldLen instead of realigning to recordStart + recordLen after each record (pre-existing, same bug class this fix addresses). Any ilst record whose data atom is followed by a trailing sub-atom (e.g. a 12-byte "name" atom, which iTunes-style writers can emit) leaves bytes unconsumed; the next iteration misreads them as a record header and the toSkip <= 0 path silently drops all remaining ilst metadata. Recording the position at the loop top and skipping to recordStart + recordLen after each branch would make every branch alignment-safe at once. ``` ########## 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,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.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() { + } + + static void extract(InputStream inputStream, Mp4BoxHandler handler, long maxBoxSize) { + StreamReader reader = new StreamReader(inputStream); + reader.setMotorolaByteOrder(true); + processBoxes(reader, -1, handler, new Mp4Context(), maxBoxSize); + } + + private static void processBoxes(StreamReader reader, long atomEnd, Mp4Handler<?> handler, + Mp4Context context, long maxBoxSize) { + try { + while (atomEnd == -1 || reader.getPosition() < atomEnd) { + long boxSize = reader.getUInt32(); + String boxType = reader.getString(4); + boolean isLargeSize = boxSize == 1; + if (isLargeSize) { + boxSize = reader.getInt64(); + } + if (boxSize > Integer.MAX_VALUE) { + handler.addError("Box size too large."); + break; + } + if (boxSize < 8) { + handler.addError("Box size too small."); + break; + } + + if (acceptContainer(handler, boxType)) { + processBoxes(reader, boxSize + reader.getPosition() - 8, + processBox(handler, boxType, null, boxSize, context), context, + maxBoxSize); + } else if (acceptBox(handler, boxType)) { + long payloadLength = boxSize - 8; + if (payloadLength > maxBoxSize) { + handler.addError("MP4 box '" + boxType + "' payload (" + payloadLength + + " bytes) exceeds the maximum of " + maxBoxSize + + " bytes; skipping."); + reader.skip(payloadLength); + } else { + handler = processBox(handler, boxType, + reader.getBytes((int) payloadLength), boxSize, context); Review Comment: Claude says: ``` StreamReader.getBytes allocates the full buffer before reading any data, so an 8-byte crafted header still forces a maxBoxSize-sized allocation (100MB by default) from a near-empty stream, and a batch of such tiny files creates sustained GC pressure in tika-server. Legitimate metadata boxes are well under 1MB. Options: clamp payloadLength against the bytes plausibly available before allocating, or default the cap much lower (a few MB) since it is configurable. ``` ########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java: ########## @@ -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); Review Comment: Claude says: ``` The zero-padding this removes was load-bearing for the ID3v2 tag body read, which uses shortDataIsFatal=false (line 68). RawTagIterator.hasNext() only checks offset < data.length && data[offset] != 0, and the RawTag constructor then reads the name, size, and flag bytes unconditionally. With the array shortened, an MP3 whose tag is cut off mid-frame-header (e.g. an interrupted download ending with "TI" of "TIT2") now throws ArrayIndexOutOfBoundsException out of the ID3v2x handler constructors, which Mp3Parser.parse does not catch. Previously such files parsed cleanly with partial metadata. Suggestion: keep the truncated copy, but make RawTagIterator stop iterating when fewer than a full frame header's bytes remain (or bounds-check the header reads). ``` ########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/video/FLVParser.java: ########## @@ -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 { Review Comment: Claude says: ``` The depth bound fixes the nesting case, but these loops remain width-unbounded: readAMFStrictArray/readAMFEcmaArray take count/size from readUInt32 (up to ~4 billion) and grow an ArrayList/HashMap per iteration with no validation against remaining input. A single flat array with a huge declared count backed by cheap repeated bytes runs to OutOfMemoryError without ever hitting MAX_AMF_DEPTH. Capping count against a max-element constant (mirroring the depth bound) would close the width half too. ``` ########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/TikaMp4SoundHandler.java: ########## @@ -103,12 +103,25 @@ 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) { Review Comment: Claude says: ``` Tiny nit: this 3-arg wrapper exists only to pass depth 0 and has exactly one caller (unlike FLVParser's pair, where a test needs the short signature). Passing 0 at the single call site and deleting the wrapper would keep the javadoc and the implementation on the same method. ``` ########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp4/boxes/TikaUserDataBox.java: ########## @@ -123,6 +123,12 @@ private void parseUserDataBox(SequentialReader reader, String handlerType, //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. + if (len < 8L || len >= Integer.MAX_VALUE) { Review Comment: Claude says: ``` Two notes on this guard. First, the len >= Integer.MAX_VALUE half silently returns where the old skip-to-EOF path threw an EOFException that Mp4Reader caught and recorded as a parse error; the outer while (reader.getPosition() < length) loop in the constructor then resumes at an unaligned offset inside the meta payload and can misparse garbage as box headers (worst case emitting bogus geo coordinates via the "(c)xyz" branch). Recording an error and/or aborting the udta walk would preserve the old observability. Second, this near-duplicates the pre-loop check at line 118 (len >= Integer.MAX_VALUE || len <= 0) with a different lower bound, so len in [1,7] passes one guard and fails the other. Restructuring so each length is validated exactly once right after it is read would keep the two from drifting apart. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
