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

[Michael Niedermayer <michael@niedermayer.cc>]

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

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++) {
            /* STTS sample offsets are uint32 but some files store it as int32
             * with negative values used to correct DTS delays.
               There may be abnormally large values as well. */
            if (sample_duration > c->max_stts_delta) {
                // assume high delta is a negative correction if greater than c->max_stts_delta
                int32_t delta_magnitude = *((int32_t *)&sample_duration);
                sc->stts_data[i].duration = 1;
                dts_correction += (delta_magnitude < 0 ? delta_magnitude - 1 : 0);
            }
            current_dts += sc->stts_data[i].duration;

            if (!dts_correction || current_dts + dts_correction > last_dts) {
                current_dts += dts_correction;
                if (!j)
                    sc->stts_data[i].duration += dts_correction/sample_count;
                dts_correction = 0;
            } else {
                /* Avoid creating non-monotonous DTS */
                dts_correction += current_dts - last_dts - 1;
                current_dts = last_dts + 1;
            }
            last_dts = current_dts;
        }


above you are taking the sample_count read from the bitstream and then
iterate based on that. The value can be INT_MAX everytime its read
and that would be too slow
                          
Iam not sure if this loop is correct as it is, does this produce the
same output as before all patches for files which use the logic ?
If its correct it can probably be optimized alot this does not
go over any array (all indexes are constants) 

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.

[-- 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".