[
https://issues.apache.org/jira/browse/TIKA-4812?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103454#comment-18103454
]
ASF GitHub Bot commented on TIKA-4812:
--------------------------------------
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.
```
> Harden multimedia parsing
> -------------------------
>
> Key: TIKA-4812
> URL: https://issues.apache.org/jira/browse/TIKA-4812
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Minor
>
> With recent work on multimedia parsers, I looked over the existing code for
> those and found some areas for improvement with the existing code.
> Let's harden those parsers from a resource perspective where we easily can.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
