[FFmpeg-devel] [PATCH] lavf/mov.c: Fix parsing of edit list atoms with invalid elst entry count.

[Michael Niedermayer]

On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> Signed-off-by: Sasi Inguva <isasi at google.com>
> ---
>  libavformat/mov.c                           | 15 +++++++-
>  tests/fate/mov.mak                          |  4 ++
>  tests/ref/fate/mov-invalid-elst-entry-count | 57 +++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ref/fate/mov-invalid-elst-entry-count
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index b22a116140..424293ad93 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4597,6 +4597,7 @@ static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      MOVStreamContext *sc;
>      int i, edit_count, version;
> +    int64_t elst_entry_size;
>  
>      if (c->fc->nb_streams < 1 || c->ignore_editlist)
>          return 0;
> @@ -4605,6 +4606,15 @@ static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      version = avio_r8(pb); /* version */
>      avio_rb24(pb); /* flags */
>      edit_count = avio_rb32(pb); /* entries */
> +    atom.size -= 8;
> +
> +    elst_entry_size = version == 1 ? 20 : 12;
> +    if (atom.size != edit_count * elst_entry_size &&
> +        c->fc->strict_std_compliance >= FF_COMPLIANCE_STRICT) {
> +        av_log(c->fc, AV_LOG_ERROR, "Invalid edit list entry_count: %d for elst atom of size: %"PRId64" bytes.\n",
> +               edit_count, atom.size + 8);
> +        return AVERROR_INVALIDDATA;
> +    }
>  
>      if (!edit_count)
>          return 0;
> @@ -4617,17 +4627,20 @@ static int mov_read_elst(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>          return AVERROR(ENOMEM);
>  
>      av_log(c->fc, AV_LOG_TRACE, "track[%u].edit_count = %i\n", c->fc->nb_streams - 1, edit_count);
> -    for (i = 0; i < edit_count && !pb->eof_reached; i++) {
> +    for (i = 0; i < edit_count && atom.size > 0 && !pb->eof_reached; i++) {
>          MOVElst *e = &sc->elst_data[i];
>  
>          if (version == 1) {
>              e->duration = avio_rb64(pb);
>              e->time     = avio_rb64(pb);
> +            atom.size -= 16;
>          } else {
>              e->duration = avio_rb32(pb); /* segment duration */
>              e->time     = (int32_t)avio_rb32(pb); /* media time */
> +            atom.size -= 8;
>          }
>          e->rate = avio_rb32(pb) / 65536.0;
> +        atom.size -= 4;
>          av_log(c->fc, AV_LOG_TRACE, "duration=%"PRId64" time=%"PRId64" rate=%f\n",
>                 e->duration, e->time, e->rate);

it would be simpler to adjust edit_count in case of ineqality.
you already compute the elst_entry_size
this would also avoid allocating a larger than needed array


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171024/0cb77f8e/attachment.sig>