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

[Gyan Doshi]

Patch superseded by new patch using helper function.

On 2021-12-21 10:20 am, Gyan Doshi wrote:
>
>
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".