diff mbox

[FFmpeg-devel] lavf/mov.c: Avoid heap allocation wraps and OOB in mov_read_{senc, saiz, udta_string}()

Message ID CAADho6NyKpkPwN6wbdQLJUqVFQBj6MGWr9t36wWC0gBUnuzP6g@mail.gmail.com
State Superseded
Headers show

Commit Message

Matthew Wolenetz Feb. 8, 2017, 11:48 p.m. UTC 
I've separated and updated the mov_read_udta_string() patch, attached.
It prevents accessing MOVContext.meta_keys[0] in that method. That array is
1-based.

On Wed, Dec 14, 2016 at 5:40 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 15.12.2016 00:37, Matthew Wolenetz wrote:
> > From 8622f9398e7c89a664c4c2ceff9d35b89ff17bb5 Mon Sep 17 00:00:00 2001
> > From: Matt Wolenetz <wolenetz@chromium.org>
> > Date: Tue, 6 Dec 2016 12:54:23 -0800
> > Subject: [PATCH] lavf/mov.c: Avoid heap allocation wraps and OOB in
> >  mov_read_{senc,saiz,udta_string}()
> >
> > Core of patch is from paul@paulmehta.com
> > Reference https://crbug.com/643952
> > ---
> >  libavformat/mov.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index e506d20..87ad91a 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -404,7 +404,7 @@ retry:
> >                  return ret;
> >              } else if (!key && c->found_hdlr_mdta && c->meta_keys) {
> >                  uint32_t index = AV_RB32(&atom.type);
> > -                if (index < c->meta_keys_count) {
> > +                if (index < c->meta_keys_count && index > 0) {
>
> This should be in a separate patch.
>
> >                      key = c->meta_keys[index];
> >                  } else {
> >                      av_log(c->fc, AV_LOG_WARNING,
> > @@ -4502,8 +4502,8 @@ static int mov_read_senc(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >
> >      avio_rb32(pb);        /* entries */
> >
> > -    if (atom.size < 8) {
> > -        av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" too
> small\n", atom.size);
> > +    if (atom.size < 8 || atom.size > UINT_MAX) {
> > +        av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64"
> invalid\n", atom.size);
> >          return AVERROR_INVALIDDATA;
> >      }
> >
> > @@ -4571,6 +4571,11 @@ static int mov_read_saiz(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >          return 0;
> >      }
> >
> > +    if (atom.size > UINT_MAX) {
> > +        av_log(c->fc, AV_LOG_ERROR, "saiz atom auxiliary_info_sizes
> size %"PRId64" invalid\n", atom.size);
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> >      /* save the auxiliary info sizes as is */
> >      data_size = atom.size - atom_header_size;
> >
>
> And these should also check for SIZE_MAX.
>
> Best regards,
> Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Comments

Michael Niedermayer Feb. 10, 2017, 11:11 a.m. UTC | #1 
On Wed, Feb 08, 2017 at 03:48:19PM -0800, Matthew Wolenetz wrote:
> I've separated and updated the mov_read_udta_string() patch, attached.
> It prevents accessing MOVContext.meta_keys[0] in that method. That array is
> 1-based.

applied

thx

[...]
diff mbox

Patch

From 1a1ad08dfdb4d3c76c64fc3d569ad360b737b0d6 Mon Sep 17 00:00:00 2001
From: Matt Wolenetz <wolenetz@google.com>
Date: Wed, 8 Feb 2017 15:40:46 -0800
Subject: [PATCH] lavf/mov.c: Avoid OOB in mov_read_udta_string()

Core of patch is from paul@paulmehta.com
Reference https://crbug.com/643952 (udta_string portion)

Signed-off-by: Matt Wolenetz <wolenetz@chromium.org>
---
 libavformat/mov.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index ca49786ea2..f804614a50 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -407,11 +407,11 @@  retry:
                 return ret;
             } else if (!key && c->found_hdlr_mdta && c->meta_keys) {
                 uint32_t index = AV_RB32(&atom.type);
-                if (index < c->meta_keys_count) {
+                if (index < c->meta_keys_count && index > 0) {
                     key = c->meta_keys[index];
                 } else {
                     av_log(c->fc, AV_LOG_WARNING,
-                           "The index of 'data' is out of range: %d >= %d.\n",
+                           "The index of 'data' is out of range: %d < 1 or >= %d.\n",
                            index, c->meta_keys_count);
                 }
             }
-- 
2.11.0.483.g087da7b7c-goog