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

[Paul B Mahol]

On 12/13/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Wed, Dec 12, 2018 at 08:08:53PM -0800, Baptiste Coudurier wrote:
>> Hi,
>>
>> > On Dec 12, 2018, at 6:53 PM, chcunningham <chcunningham at 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
>
> I have at least 2 files which have a id of 0
> Iam not sure where they are from so iam not sure i can share them


Haha.


> found with:
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4450,6 +4450,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>          avio_rb32(pb); /* modification time */
>      }
>      st->id = (int)avio_rb32(pb); /* track id (NOT 0 !)*/
> +    av_assert0(st->id);
>      avio_rb32(pb); /* reserved */
>
>      /* highlevel (considering edits) duration in movie timebase */
>
>
> Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '/home/michael/videos/qtrle/Earth
> Spin 1-bit qtrle.mov':
>   Metadata:
>     creation_time   : 2015-12-20T17:51:06.000000Z
>     copyright       : ©Aroborescence, San Francisco All Rights Reserved.
>     copyright-eng   : ©Aroborescence, San Francisco All Rights Reserved.
>     comment         : Not for reuse or resale
>     comment-eng     : Not for reuse or resale
>     date            : Monday, May 6, 1991
>     date-eng        : Monday, May 6, 1991
>     original_format : Digitized
>     original_format-eng: Digitized
>     director        :
>     director-eng    :
>     producer        :
>     producer-eng    :
>     composer        :
>     composer-eng    :
>     performers      :
>     performers-eng  :
>     original_source :
>     original_source-eng:
>   Duration: 00:00:03.60, start: 0.000000, bitrate: 203 kb/s
>     Stream #0:0(eng): Video: qtrle (rle  / 0x20656C72), pal8, 186x186, 197
> kb/s, 10 fps, 10 tbr, 600 tbn, 600 tbc (default)
>     Metadata:
>       rotate          : 0
>       creation_time   : 2015-12-20T17:51:06.000000Z
>       handler_name    : Apple Alias Data Handler
>       encoder         : Animation - RLE
>     Side data:
>       displaymatrix: rotation of -0.00 degrees
>
>
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> When the tyrant has disposed of foreign enemies by conquest or treaty, and
> there is nothing more to fear from them, then he is always stirring up
> some war or other, in order that the people may require a leader. -- Plato
>

Indeed.