diff mbox series

[FFmpeg-devel] avformat/mov: check offset for overflow in mov_probe()

Message ID 20210404214445.24140-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avformat/mov: check offset for overflow in mov_probe()
Related show

Checks

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

Commit Message

Michael Niedermayer April 4, 2021, 9:44 p.m. UTC
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(+)

Comments

James Almer April 4, 2021, 10:18 p.m. UTC | #1
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.
Michael Niedermayer April 5, 2021, 4:40 p.m. UTC | #2
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 mbox series

Patch

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) {