[FFmpeg-devel,09/10] avformat/matroskadec: Improve length check

Submitted by Andreas Rheinhardt on March 8, 2019, 9:26 a.m.

Details

Message ID 20190308092604.3752-10-andreas.rheinhardt@googlemail.com
State New
Headers show

Commit Message

Andreas Rheinhardt March 8, 2019, 9:26 a.m.
The earlier code had three flaws:

1. The case of an unknown-sized element inside a finite-sized element
(which is against the specifications) was not caught.

2. The error message wasn't helpful: It compared the length of the child
with the offset of the end of the parent and claimed that the first
exceeds the latter, although that is not necessarily true.

3. Unknown-sized elements that are not parsed can't be skipped. Given
that according to the Matroska specifications only the segment and the
clusters can be of unknown-size, this is handled by not allowing any
other units to have infinite size whereas the earlier code would seek
back by 1 byte upon encountering an infinite-size element that ought
to be skipped.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
---
 libavformat/matroskadec.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Michael Niedermayer March 8, 2019, 11:45 p.m.
On Fri, Mar 08, 2019 at 10:26:03AM +0100, Andreas Rheinhardt wrote:
> The earlier code had three flaws:
> 
> 1. The case of an unknown-sized element inside a finite-sized element
> (which is against the specifications) was not caught.
> 
> 2. The error message wasn't helpful: It compared the length of the child
> with the offset of the end of the parent and claimed that the first
> exceeds the latter, although that is not necessarily true.
> 
> 3. Unknown-sized elements that are not parsed can't be skipped. Given
> that according to the Matroska specifications only the segment and the
> clusters can be of unknown-size, this is handled by not allowing any
> other units to have infinite size whereas the earlier code would seek
> back by 1 byte upon encountering an infinite-size element that ought
> to be skipped.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com>
> ---
>  libavformat/matroskadec.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

i think this patch is ok but there are people around who know matroska better
than i do ...

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d899ee5744..6c796e8a4a 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1172,11 +1172,22 @@  static int ebml_parse_elem(MatroskaDemuxContext *matroska,
         if (matroska->num_levels > 0) {
             MatroskaLevel *level = &matroska->levels[matroska->num_levels - 1];
             int64_t pos = avio_tell(pb);
-            if (level->length != EBML_UNKNOWN_LENGTH &&
+            if (length == EBML_UNKNOWN_LENGTH &&
+                level->length != EBML_UNKNOWN_LENGTH) {
+                av_log(matroska->ctx, AV_LOG_ERROR, "Unknown-sized element "
+                       "inside parent with finite size\n");
+                return AVERROR_INVALIDDATA;
+            } else if (level->length != EBML_UNKNOWN_LENGTH &&
                 (pos + length) > (level->start + level->length)) {
                 av_log(matroska->ctx, AV_LOG_ERROR,
-                       "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in parent\n",
-                       length, level->start + level->length);
+                       "Element ending at 0x%"PRIx64" exceeds containing "
+                       "master element ending at 0x%"PRIx64"\n",
+                        pos + length, level->start + level->length);
+                return AVERROR_INVALIDDATA;
+            } else if (length == EBML_UNKNOWN_LENGTH && id != MATROSKA_ID_CLUSTER) {
+                av_log(matroska->ctx, AV_LOG_ERROR,
+                       "Found unknown-sized element other than a cluster"
+                       " in a segment. Dropping the invalid element.\n");
                 return AVERROR_INVALIDDATA;
             }
         }