| Message ID | 20181213025301.191821-1-chcunningham@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
Hi, > On Dec 12, 2018, at 6:53 PM, chcunningham <chcunningham@chromium.org> wrote: > > Chromium fuzzing produced a whacky file with extra tkhds. This caused > an AVStream that was already in use to be corrupted by assigning it a > new id, which blows up later in mov_read_trun because the > MOVFragmentStreamInfo.index_entry now points OOB. > --- > libavformat/isom.h | 3 ++- > libavformat/mov.c | 7 +++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libavformat/isom.h b/libavformat/isom.h > index e629663949..e14d670f2f 100644 > --- a/libavformat/isom.h > +++ b/libavformat/isom.h > @@ -230,7 +230,8 @@ typedef struct MOVStreamContext { > > uint32_t format; > > - int has_sidx; // If there is an sidx entry for this stream. > + int has_sidx; ///< If there is a sidx entry for this stream. > + int has_tkhd; ///< If there is a tkhd entry for this stream. > struct { > struct AVAESCTR* aes_ctr; > unsigned int per_sample_iv_size; // Either 0, 8, or 16. > diff --git a/libavformat/mov.c b/libavformat/mov.c > index ec57a05803..47c53d7992 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -4438,6 +4438,12 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) > st = c->fc->streams[c->fc->nb_streams-1]; > sc = st->priv_data; > > + // Each stream (trak) should have exactly 1 tkhd. This catches bad files and > + // avoids corrupting AVStreams mapped to an earlier tkhd. > + if (sc->has_tkhd) > + return AVERROR_INVALIDDATA; > + sc->has_tkhd = 1; > + > Could we just check that st->id is already set ? IIRC 0 is not a valid value Thanks a lot! — Baptiste
diff --git a/libavformat/isom.h b/libavformat/isom.h index e629663949..e14d670f2f 100644 --- a/libavformat/isom.h +++ b/libavformat/isom.h @@ -230,7 +230,8 @@ typedef struct MOVStreamContext { uint32_t format; - int has_sidx; // If there is an sidx entry for this stream. + int has_sidx; ///< If there is a sidx entry for this stream. + int has_tkhd; ///< If there is a tkhd entry for this stream. struct { struct AVAESCTR* aes_ctr; unsigned int per_sample_iv_size; // Either 0, 8, or 16. diff --git a/libavformat/mov.c b/libavformat/mov.c index ec57a05803..47c53d7992 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -4438,6 +4438,12 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom) st = c->fc->streams[c->fc->nb_streams-1]; sc = st->priv_data; + // Each stream (trak) should have exactly 1 tkhd. This catches bad files and + // avoids corrupting AVStreams mapped to an earlier tkhd. + if (sc->has_tkhd) + return AVERROR_INVALIDDATA; + sc->has_tkhd = 1; + version = avio_r8(pb); flags = avio_rb24(pb); st->disposition |= (flags & MOV_TKHD_FLAG_ENABLED) ? AV_DISPOSITION_DEFAULT : 0; @@ -4704,6 +4710,7 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, MOVAtom atom) break; } } + av_assert0(index_entry_pos <= st->nb_index_entries); avio_r8(pb); /* version */ flags = avio_rb24(pb);