[FFmpeg-devel] [PATCH v2] avformat/mov: abort reading truncated stts

[Gyan Doshi]

On 2021-12-21 03:06 am, Michael Niedermayer wrote:
> On Mon, Dec 20, 2021 at 10:21:53PM +0100, Michael Niedermayer wrote:
>> On Tue, Dec 21, 2021 at 02:31:33AM +0530, Gyan Doshi wrote:
>>>
>>> On 2021-12-21 02:24 am, Andreas Rheinhardt wrote:
>>>> Gyan Doshi:
>>>>> On 2021-12-21 02:18 am, Andreas Rheinhardt wrote:
>>>>>> Gyan Doshi:
>>>>>>> Avoids overreading the box and ingesting absurd values into stts_data
>>>>>>> ---
>>>>>>>     libavformat/mov.c | 5 +++++
>>>>>>>     1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>>>>> index 2aed6e80ef..5a7209837f 100644
>>>>>>> --- a/libavformat/mov.c
>>>>>>> +++ b/libavformat/mov.c
>>>>>>> @@ -2935,6 +2935,11 @@ static int mov_read_stts(MOVContext *c,
>>>>>>> AVIOContext *pb, MOVAtom atom)
>>>>>>>         avio_rb24(pb); /* flags */
>>>>>>>         entries = avio_rb32(pb);
>>>>>>>     +    if (atom.size < 8 + (int64_t)entries*8) {
>>>>>>> +        av_log(c->fc, AV_LOG_ERROR, "Truncated STTS box for st
>>>>>>> %d.\n", c->fc->nb_streams-1);
>>>>>>> +        return AVERROR_INVALIDDATA;
>>>>>>> +    }
>>>>>>> +
>>>>>>>         av_log(c->fc, AV_LOG_TRACE, "track[%u].stts.entries = %u\n",
>>>>>>>                 c->fc->nb_streams-1, entries);
>>>>>> This might fix the issue with the fuzzer sample Michael gave you, but
>>>>>> what would stop the fuzzer (or a malicious adversary) from simply using
>>>>>> a gigantic atom size?
>>>>> Do you want the comparison to switch to a strict inequality?
>>>>>
>>>> No, because it might be that the adversary just uses the expected size,
>>>> so this would not fix anything.
>>> There are real world multi-hour files with large stts boxes, so there is no
>>> robust solution for that, only heuristics.
>>
>> lets take a closer look at the loop you are adding
>>
>>          sample_count    = avio_rb32(pb);
>>          sample_duration = avio_rb32(pb);
>>
>>          sc->stts_data[i].count= sample_count;
>>          sc->stts_data[i].duration= sample_duration;
>>
>>          for (int j = 0; j < sample_count; j++) {
> This also adds undefined behavior as j overflows when sample_count > INT_MAX

I'll try to optimize by getting rid of the loop if I can, but this 
discussion belongs to the patch for max_stts_delta.

How's this patch?

Regards,
Gyan