| Message ID | 20211220204609.199-1-ffmpeg@gyani.pro |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avformat/mov: abort reading truncated stts | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
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
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
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
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
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 [...]
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 [...]
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
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".
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);