[FFmpeg-devel] [PATCH 13/21] avformat/matroskadec: Improve length check

[Steve Lhomme]

On 3/27/2019 12:18 PM, Andreas Rheinhardt via ffmpeg-devel 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

This restriction is rather new. There might be old files that use 
infinite sizes in other places.

> 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 at googlemail.com>
> ---
>   libavformat/matroskadec.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 6fa324c0cc..0179e5426e 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1180,11 +1180,29 @@ 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 &&
> -                (pos + length) > (level->start + level->length)) {
> +
> +            if (length != EBML_UNKNOWN_LENGTH &&
> +                level->length != EBML_UNKNOWN_LENGTH) {
> +                uint64_t elem_end = pos + length,
> +                        level_end = level->start + level->length;
> +
> +                if (level_end < elem_end) {
> +                    av_log(matroska->ctx, AV_LOG_ERROR,
> +                           "Element at 0x%"PRIx64" ending at 0x%"PRIx64" exceeds "
> +                           "containing master element ending at 0x%"PRIx64"\n",
> +                           pos, elem_end, level_end);
> +                    return AVERROR_INVALIDDATA;
> +                }
> +            } else if (level->length != EBML_UNKNOWN_LENGTH) {
> +                av_log(matroska->ctx, AV_LOG_ERROR, "Unknown-sized element "
> +                       "at 0x%"PRIx64" inside parent with finite size\n", pos);
> +                return AVERROR_INVALIDDATA;

IMO there's no reason to return an error here. You can log the error and 
still parse the data, it should end up fine (if the code is done as such 
that you can never read past your parents boundaries). At least libebml 
can deal with that.

> +            } else if (length == EBML_UNKNOWN_LENGTH && id != MATROSKA_ID_CLUSTER) {
> +                // According to the specifications only clusters and segments
> +                // are allowed to be unknown-sized.
>                   av_log(matroska->ctx, AV_LOG_ERROR,
> -                       "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in parent\n",
> -                       length, level->start + level->length);
> +                       "Found unknown-sized element other than a cluster at "
> +                       "0x%"PRIx64". Dropping the invalid element.\n", pos);
>                   return AVERROR_INVALIDDATA;
>               }
>           }
> -- 
> 2.19.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".