| Message ID | 20210404214445.24140-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mov: check offset for overflow in mov_probe() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 4/4/2021 6:44 PM, Michael Niedermayer wrote: > Fixes: Invalid read of size 4 > Fixes: ASAN_Deadlysignal.zip > > Found-by: Hardik Shah <hardik05@gmail.com> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mov.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 7805330bf9..ef73f3199d 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -7161,6 +7161,8 @@ static int mov_probe(const AVProbeData *p) > score = FFMAX(score, AVPROBE_SCORE_EXTENSION); > break; > } > + if ((uint64_t)size + 8 > INT64_MAX - offset) > + break; > offset += size; > } > if (score > AVPROBE_SCORE_MAX - 50 && moov_offset != -1) { Would something like this also work? > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 1974498d1e..cd9d9996b3 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -7119,7 +7119,7 @@ static int mov_probe(const AVProbeData *p) > int64_t size; > int minsize = 8; > /* ignore invalid offset */ > - if ((offset + 8) > (unsigned int)p->buf_size) > + if ((offset + 8ULL) > (unsigned int)p->buf_size) > break; > size = AV_RB32(p->buf + offset); > if (size == 1 && offset + 16 <= (unsigned int)p->buf_size) { > @@ -7166,6 +7166,8 @@ static int mov_probe(const AVProbeData *p) > score = FFMAX(score, AVPROBE_SCORE_EXTENSION); > break; > } > + if (size > INT64_MAX - offset) > + break; > offset += size; > } > if (score > AVPROBE_SCORE_MAX - 50 && moov_offset != -1) { I think it conveys what it's trying to do more clearly than your version, where the + 8 on top of size is not immediately clear where it comes from.
On Sun, Apr 04, 2021 at 07:18:22PM -0300, James Almer wrote: > On 4/4/2021 6:44 PM, Michael Niedermayer wrote: > > Fixes: Invalid read of size 4 > > Fixes: ASAN_Deadlysignal.zip > > > > Found-by: Hardik Shah <hardik05@gmail.com> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/mov.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > index 7805330bf9..ef73f3199d 100644 > > --- a/libavformat/mov.c > > +++ b/libavformat/mov.c > > @@ -7161,6 +7161,8 @@ static int mov_probe(const AVProbeData *p) > > score = FFMAX(score, AVPROBE_SCORE_EXTENSION); > > break; > > } > > + if ((uint64_t)size + 8 > INT64_MAX - offset) > > + break; > > offset += size; > > } > > if (score > AVPROBE_SCORE_MAX - 50 && moov_offset != -1) { > > Would something like this also work? > > > diff --git a/libavformat/mov.c b/libavformat/mov.c > > index 1974498d1e..cd9d9996b3 100644 > > --- a/libavformat/mov.c > > +++ b/libavformat/mov.c > > @@ -7119,7 +7119,7 @@ static int mov_probe(const AVProbeData *p) > > int64_t size; > > int minsize = 8; > > /* ignore invalid offset */ > > - if ((offset + 8) > (unsigned int)p->buf_size) > > + if ((offset + 8ULL) > (unsigned int)p->buf_size) > > break; > > size = AV_RB32(p->buf + offset); > > if (size == 1 && offset + 16 <= (unsigned int)p->buf_size) { > > @@ -7166,6 +7166,8 @@ static int mov_probe(const AVProbeData *p) > > score = FFMAX(score, AVPROBE_SCORE_EXTENSION); > > break; > > } > > + if (size > INT64_MAX - offset) > > + break; > > offset += size; > > } > > if (score > AVPROBE_SCORE_MAX - 50 && moov_offset != -1) { > > I think it conveys what it's trying to do more clearly than your version, > where the + 8 on top of size is not immediately clear where it comes from. yes that works, i will apply this thx [...]
diff --git a/libavformat/mov.c b/libavformat/mov.c index 7805330bf9..ef73f3199d 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -7161,6 +7161,8 @@ static int mov_probe(const AVProbeData *p) score = FFMAX(score, AVPROBE_SCORE_EXTENSION); break; } + if ((uint64_t)size + 8 > INT64_MAX - offset) + break; offset += size; } if (score > AVPROBE_SCORE_MAX - 50 && moov_offset != -1) {
Fixes: Invalid read of size 4 Fixes: ASAN_Deadlysignal.zip Found-by: Hardik Shah <hardik05@gmail.com> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mov.c | 2 ++ 1 file changed, 2 insertions(+)