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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2494 bytes --]

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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".