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

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

[Gyan Doshi]

Avoids overreading the box and ingesting absurd values into stts_data
---

Fixes prolonged demuxing for fuzzer-generated files in the loop added in
patch for max_stts_delta

 libavformat/mov.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2aed6e80ef..8d88119b29 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 + 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);
 
-- 
2.33.0

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

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

[Andreas Rheinhardt]

Gyan Doshi:
> Avoids overreading the box and ingesting absurd values into stts_data
> ---
> 
> Fixes prolonged demuxing for fuzzer-generated files in the loop added in
> patch for max_stts_delta
> 
>  libavformat/mov.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 2aed6e80ef..8d88119b29 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 + entries*8) {

This can overflow.

> +        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);
>  
> 

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

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

[Gyan Doshi]

On 2021-12-21 01:27 am, Andreas Rheinhardt wrote:
> Gyan Doshi:
>> Avoids overreading the box and ingesting absurd values into stts_data
>> ---
>>
>> Fixes prolonged demuxing for fuzzer-generated files in the loop added in
>> patch for max_stts_delta
>>
>>   libavformat/mov.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 2aed6e80ef..8d88119b29 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 + entries*8) {
> This can overflow.

Can you illustrate?

atom.size is int64; entries is uint32.

And cppreference says,

"If the signed type can represent all values of the unsigned type, then 
the operand with the unsigned type is implicitly converted to the signed 
type. "

Gyan
_______________________________________________
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".

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

[Andreas Rheinhardt]

Gyan Doshi:
> 
> 
> On 2021-12-21 01:27 am, Andreas Rheinhardt wrote:
>> Gyan Doshi:
>>> Avoids overreading the box and ingesting absurd values into stts_data
>>> ---
>>>
>>> Fixes prolonged demuxing for fuzzer-generated files in the loop added in
>>> patch for max_stts_delta
>>>
>>>   libavformat/mov.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 2aed6e80ef..8d88119b29 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 + entries*8) {
>> This can overflow.
> 
> Can you illustrate?
> 
> atom.size is int64; entries is uint32.
> 
> And cppreference says,
> 
> "If the signed type can represent all values of the unsigned type, then
> the operand with the unsigned type is implicitly converted to the signed
> type. "
> 

8 + entries * 8 is calculated using unsigned with potential (defined)
wraparound; only afterwards is the result converted to int64_t
(presuming you have 32bit unsigned as usual) for the comparison.

- Andreas
_______________________________________________
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".

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

[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);
 
-- 
2.33.0

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

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

[Andreas Rheinhardt]

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?

- Andreas
_______________________________________________
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".

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

[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?

Regards,
Gyan
_______________________________________________
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".

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

[Andreas Rheinhardt]

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.

- Andreas
_______________________________________________
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".

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

[Gyan Doshi]

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.

In any case, handling/recognizing the sample count values that led to a 
prolonged demux in Michael's sample is not germane to this patch.

Regards,
Gyan
_______________________________________________
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".

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

[Michael Niedermayer]

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

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

[Michael Niedermayer]

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

Re: [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
_______________________________________________
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".

Re: [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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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