diff mbox

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

Message ID CAADho6N=7_KOkfA=be3wmLMAG0S13zkoV+5Fk2AoXp6At8gV4g@mail.gmail.com
State New
Headers show

Commit Message

Matthew Wolenetz Feb. 10, 2017, 12:03 a.m. UTC 
I've separated and updated the mov_read_{senc,saiz}() patch, attached.
It avoids allocation wraps in those two functions.

On Wed, Feb 8, 2017 at 3:48 PM, Matthew Wolenetz <wolenetz@chromium.org>
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.
>
> 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:13 a.m. UTC | #1 
On Thu, Feb 09, 2017 at 04:03:57PM -0800, Matthew Wolenetz wrote:
> I've separated and updated the mov_read_{senc,saiz}() patch, attached.
> It avoids allocation wraps in those two functions.

applied

thanks

[...]
diff mbox

Patch

From bba91f9adf4d875814ec4e418e02316cbcf63f6f Mon Sep 17 00:00:00 2001
From: Matt Wolenetz <wolenetz@google.com>
Date: Wed, 14 Dec 2016 15:27:49 -0800
Subject: [PATCH] lavf/mov.c: Avoid heap allocation wraps in
 mov_read_{senc,saiz}()

Core of patch is from paul@paulmehta.com
Reference https://crbug.com/643952 (senc,saiz portions)

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

diff --git a/libavformat/mov.c b/libavformat/mov.c
index ca49786ea2..36b65b3b08 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4968,8 +4968,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 > FFMIN(INT_MAX, SIZE_MAX)) {
+        av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" invalid\n", atom.size);
         return AVERROR_INVALIDDATA;
     }
 
@@ -5037,6 +5037,11 @@  static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         return 0;
     }
 
+    if (atom.size > FFMIN(INT_MAX, SIZE_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;
 
-- 
2.11.0.483.g087da7b7c-goog