[FFmpeg-devel] avformat/mov: validate chunk_count vs stsc_data

Submitted by chcunningham@chromium.org on Feb. 5, 2019, 12:30 a.m.

Details

Message ID 20190205003029.1999-1-chcunningham@chromium.org
State New
Headers show

Commit Message

chcunningham@chromium.org Feb. 5, 2019, 12:30 a.m.
Bad content may contain stsc boxes with a first_chunk index that
exceeds stco.entries (chunk_count). This ammends the existing check to
include cases where chunk_count == 0.
---
 libavformat/mov.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer Feb. 6, 2019, 3:57 p.m.
On Mon, Feb 04, 2019 at 04:30:29PM -0800, chcunningham wrote:
> Bad content may contain stsc boxes with a first_chunk index that
> exceeds stco.entries (chunk_count). This ammends the existing check to
> include cases where chunk_count == 0.
> ---
>  libavformat/mov.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

This seems to break GF9720Repeal20the20Eighth20with20Helen20Linehan.m4a
Thats a sample from chromium issue 822666

thx


[...]
chcunningham@chromium.org Feb. 7, 2019, 10:58 p.m.
Thanks! Looking at that file, I notice the stsc refers to some unknown
chunks (stsco has no chunk offsets), but this is a non-issue since stts has
no samples. Next patch will detect this condition and simply clear out stsc
structures since they're not needed and contradict stco.

On Wed, Feb 6, 2019 at 7:57 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Feb 04, 2019 at 04:30:29PM -0800, chcunningham wrote:
> > Bad content may contain stsc boxes with a first_chunk index that
> > exceeds stco.entries (chunk_count). This ammends the existing check to
> > include cases where chunk_count == 0.
> > ---
> >  libavformat/mov.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
>
> This seems to break GF9720Repeal20the20Eighth20with20Helen20Linehan.m4a
> Thats a sample from chromium issue 822666
>
> thx
>
>
> [...]
> --
> 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
>

Patch hide | download patch | download mbox

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 9b9739f788..2f3ad38ac3 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2694,8 +2694,11 @@  static inline int64_t mov_get_stsc_samples(MOVStreamContext *sc, unsigned int in
 
     if (mov_stsc_index_valid(index, sc->stsc_count))
         chunk_count = sc->stsc_data[index + 1].first - sc->stsc_data[index].first;
-    else
+    else {
+        // Validation for stsc / stco  happens earlier in mov_read_stsc + mov_read_trak.
+        av_assert0(sc->stsc_data[index].first <= sc->chunk_count);
         chunk_count = sc->chunk_count - (sc->stsc_data[index].first - 1);
+    }
 
     return sc->stsc_data[index].count * (int64_t)chunk_count;
 }
@@ -4175,7 +4178,7 @@  static int mov_read_trak(MOVContext *c, AVIOContext *pb, MOVAtom atom)
                st->index);
         return 0;
     }
-    if (sc->chunk_count && sc->stsc_count && sc->stsc_data[ sc->stsc_count - 1 ].first > sc->chunk_count) {
+    if (sc->stsc_count && sc->stsc_data[ sc->stsc_count - 1 ].first > sc->chunk_count) {
         av_log(c->fc, AV_LOG_ERROR, "stream %d, contradictionary STSC and STCO\n",
                st->index);
         return AVERROR_INVALIDDATA;