diff mbox

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

Message ID CAGD_KHeoYjjed5MPao+NFXj6Zc+1DoGddcD3SkGGf+wi8FXupw@mail.gmail.com
State New
Headers show

Commit Message

Sasi Inguva Oct. 27, 2017, 3:51 a.m. UTC 
On Tue, Oct 24, 2017 at 2:48 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> > Signed-off-by: Sasi Inguva <isasi@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
>
> Done. Attaching the corrected patch.

>
> [...]
>
> --
> 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.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>

Comments

Michael Niedermayer Oct. 28, 2017, 6:26 p.m. UTC | #1 
On Thu, Oct 26, 2017 at 08:51:50PM -0700, Sasi Inguva wrote:
> On Tue, Oct 24, 2017 at 2:48 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > On Mon, Oct 23, 2017 at 04:18:28PM -0700, Sasi Inguva wrote:
> > > Signed-off-by: Sasi Inguva <isasi@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
> >
> > Done. Attaching the corrected patch.
> 
> >
> > [...]
> >
> > --
> > 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.
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >

>  libavformat/mov.c                           |   21 +++++++++-
>  tests/fate/mov.mak                          |    4 +
>  tests/ref/fate/mov-invalid-elst-entry-count |   57 ++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 1 deletion(-)
> c553340f66797876d039f408f83574a40c54d17b  0001-lavf-mov.c-Fix-parsing-of-edit-list-atoms-with-inval.patch
> From 188602e662868a79c7f85e9193e9aedc9ba1a170 Mon Sep 17 00:00:00 2001
> From: Sasi Inguva <isasi@google.com>
> Date: Wed, 18 Oct 2017 20:11:16 -0700
> Subject: [PATCH] lavf/mov.c: Fix parsing of edit list atoms with invalid elst
>  entry count.

applied

thanks

[...]
diff mbox

Patch

From 188602e662868a79c7f85e9193e9aedc9ba1a170 Mon Sep 17 00:00:00 2001
From: Sasi Inguva <isasi@google.com>
Date: Wed, 18 Oct 2017 20:11:16 -0700
Subject: [PATCH] lavf/mov.c: Fix parsing of edit list atoms with invalid elst
 entry count.

Signed-off-by: Sasi Inguva <isasi@google.com>
---
 libavformat/mov.c                           | 21 ++++++++++-
 tests/fate/mov.mak                          |  4 ++
 tests/ref/fate/mov-invalid-elst-entry-count | 57 +++++++++++++++++++++++++++++
 3 files changed, 81 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..bd019317bf 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,21 @@  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) {
+        if (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;
+        } else {
+            edit_count = atom.size / elst_entry_size;
+            if (edit_count * elst_entry_size != atom.size) {
+                av_log(c->fc, AV_LOG_WARNING, "ELST atom of %"PRId64" bytes, bigger than %d entries.", atom.size, edit_count);
+            }
+        }
+    }
 
     if (!edit_count)
         return 0;
@@ -4617,17 +4633,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);
 
diff --git a/tests/fate/mov.mak b/tests/fate/mov.mak
index cfdada7a2e..ef250a12d8 100644
--- a/tests/fate/mov.mak
+++ b/tests/fate/mov.mak
@@ -6,6 +6,7 @@  FATE_MOV = fate-mov-3elist \
            fate-mov-1elist-ends-last-bframe \
            fate-mov-2elist-elist1-ends-bframe \
            fate-mov-3elist-encrypted \
+           fate-mov-invalid-elst-entry-count \
            fate-mov-gpmf-remux \
 
 FATE_MOV_FFPROBE = fate-mov-aac-2048-priming \
@@ -39,6 +40,9 @@  fate-mov-1elist-ends-last-bframe: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-1e
 # Makes sure that we handle timestamps of packets in case of multiple edit lists with one of them ending on a B-frame correctly.
 fate-mov-2elist-elist1-ends-bframe: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/mov-2elist-elist1-ends-bframe.mov
 
+# Makes sure that we handle invalid edit list entry count correctly.
+fate-mov-invalid-elst-entry-count: CMD = framemd5 -i $(TARGET_SAMPLES)/mov/invalid_elst_entry_count.mov
+
 fate-mov-aac-2048-priming: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_packets -print_format compact $(TARGET_SAMPLES)/mov/aac-2048-priming.mov
 
 fate-mov-zombie: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_streams -show_packets -show_frames -bitexact -print_format compact $(TARGET_SAMPLES)/mov/white_zombie_scrunch-part.mov
diff --git a/tests/ref/fate/mov-invalid-elst-entry-count b/tests/ref/fate/mov-invalid-elst-entry-count
new file mode 100644
index 0000000000..13b575816b
--- /dev/null
+++ b/tests/ref/fate/mov-invalid-elst-entry-count
@@ -0,0 +1,57 @@ 
+#format: frame checksums
+#version: 2
+#hash: MD5
+#tb 0: 1/24
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 640x480
+#sar 0: 1/1
+#stream#, dts,        pts, duration,     size, hash
+0,          0,          0,        1,   460800, 549730883a0b56e6accaf021903daecf
+0,          1,          1,        1,   460800, 783389b4342d4be925fc5244791e760a
+0,          2,          2,        1,   460800, 8384af6426d94a2077930c93013e09ad
+0,          3,          3,        1,   460800, 9380a1d9ecacf5b3105383c1c8083188
+0,          4,          4,        1,   460800, eb28174acfceb868b9058757bed049c5
+0,          5,          5,        1,   460800, 9732bd4a58884dbf2be48d819433130f
+0,          6,          6,        1,   460800, 0c553fb530cf042eb84f5b13817a96a6
+0,          7,          7,        1,   460800, 621f02aded5e35fa1f373afd3ed283bd
+0,          8,          8,        1,   460800, c76167c6bda91f657708c88252ea315d
+0,          9,          9,        1,   460800, 872df2d8c522e2440ddd04bca7dce497
+0,         10,         10,        1,   460800, 6ee9154e48c5132ad4ba122b255bd2bb
+0,         11,         11,        1,   460800, 362e61629795702ebe9183ce3786d7f2
+0,         12,         12,        1,   460800, f3ec59e6fc4e3c2e75f42bef34ca73b5
+0,         13,         13,        1,   460800, 68d9caea8697736dd716cba43b614919
+0,         14,         14,        1,   460800, 4a4efb0201a64236db4330725758c139
+0,         15,         15,        1,   460800, f32f8997dcdd87ad910dea886a0de17d
+0,         16,         16,        1,   460800, 51a8549d7b4aaacaf6050bc07a82b440
+0,         17,         17,        1,   460800, 5145aa05bbb0c3faab40fc8fa233af1d
+0,         18,         18,        1,   460800, bbfcbe3c9600b2a0f413057d7959e9e7
+0,         19,         19,        1,   460800, 02cfd4a350fa274e12fce8352001bf21
+0,         20,         20,        1,   460800, 20dd372da9e656add433f31e3e9c1fb8
+0,         21,         21,        1,   460800, 3b885593f8b42676ce40c329a63f62bf
+0,         22,         22,        1,   460800, c38b453b56c3ea14f7d8691d83752486
+0,         23,         23,        1,   460800, 79643132988dabc9dc1ba3af0aeaebc5
+0,         24,         24,        1,   460800, 60a099be31244b2f69ca6107cdbd7e06
+0,         25,         25,        1,   460800, 1de6ff4e0aa81216e4b7b1c8e74fb143
+0,         26,         26,        1,   460800, 5223a81e6964c28cf42593f259397aa1
+0,         27,         27,        1,   460800, 2dfcf01c86aa712cd6f1c7656eeb17db
+0,         28,         28,        1,   460800, 8c86ee0f02fabccaed8d8fc8babd031e
+0,         29,         29,        1,   460800, b3ea1983f7efeec11306445d9ae5d477
+0,         30,         30,        1,   460800, 86a4cc9fa7db5ff5ca2be69ad191179f
+0,         31,         31,        1,   460800, 8194715afe23ae34a019797a53a6ee2c
+0,         32,         32,        1,   460800, 447a153f1c6bb703eff62edfd14e08e0
+0,         33,         33,        1,   460800, 092257082789b898dbb14d1f19e79347
+0,         34,         34,        1,   460800, d6320d204a119cfeef5645a4118bc600
+0,         35,         35,        1,   460800, 2ee710deae4bb0d156528797ad1c4981
+0,         36,         36,        1,   460800, 1256eac030985c04c4501ad5a72e9d66
+0,         37,         37,        1,   460800, f16ad8c1aa572be7666c7907ce4aebbc
+0,         38,         38,        1,   460800, 865088cbd47d0151b62a45d5426c8216
+0,         39,         39,        1,   460800, 26c78ca43d93c6da153f3dea5d945e0e
+0,         40,         40,        1,   460800, d775d6705c965401ccc143d5ae432938
+0,         41,         41,        1,   460800, f9837d514753c59e6776452d9043524f
+0,         42,         42,        1,   460800, 8463f5172914828abcc770f888bfd183
+0,         43,         43,        1,   460800, 3108557748cfb7965b33b16b35359de0
+0,         44,         44,        1,   460800, 477d596944e028dd72c207bb6e6b22de
+0,         45,         45,        1,   460800, 69e4ffbd600c8d8bc070d7d7324ee2b1
+0,         46,         46,        1,   460800, 2211c57bc9ec1788217002900f9102b1
+0,         47,         47,        1,   460800, bcfb5f0a7f88da3ec8c6b447ed4b67eb
-- 
2.15.0.rc2.357.g7e34df9404-goog