[FFmpeg-devel] lavf/mov: ensure only one tkhd per trak

Submitted by chcunningham@chromium.org on Dec. 13, 2018, 2:53 a.m.

Details

Message ID 20181213025301.191821-1-chcunningham@chromium.org
State Accepted
Headers show

Commit Message

chcunningham@chromium.org Dec. 13, 2018, 2:53 a.m.
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(-)

Comments

Baptiste Coudurier Dec. 13, 2018, 4:08 a.m.
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

Patch hide | download patch | download mbox

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);