| Message ID | 20211220195310.5633-1-ffmpeg@gyani.pro |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] 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 > --- > > 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); > >
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
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
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);