New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Security

Blocking:
issue 591845



Sign in to add a comment
link

Issue 643952: Security: FFMPEG MP4 Decoder - Non-exploitable issues (3 Issues: 2 heap allocation wraps, and ~out-of-bounds access)

Reported by p...@paulmehta.com, Sep 3 2016

Issue description

I've batched these 3 together as they aren't going to be exploitable.

- chrome_child!mov_read_senc - mov.c - Allocation wrap
- chrome_child!mov_read_saiz - mov.c - Allocation wrap
    see attached:  mov_read_senc-mov_read_saiz-allocation_wrap_not_exploitable-x2-pmehta.readme.txt

- chrome_child!mov_read_udta_string - Out of bounds array access
    see attached:  mov_read_udta_string-oob_index-pmehta.readme.txt

A pointer is read from out of bounds at c->meta_keys[0], because the array allocated and based at 1. I looked into it, and I really doubt it's exploitable. Anyways, I noticed it, so I thought it should be reported. Small gripe, but is unintended out-of-bounds access, and this memory can contain dangling pointers when combined mov_read_keys allocation wraps (ie. doesn't clear memory).
 
mov_read_senc-mov_read_saiz-allocation_wrap_not_exploitable-x2-pmehta.readme.txt
3.3 KB View Download
mov_read_udta_string-oob_index-pmehta.readme.txt
5.1 KB View Download

Comment 1 by vakh@chromium.org, Sep 4 2016

Cc: xhw...@chromium.org ddorwin@chromium.org wolenetz@chromium.org
Components: Internals>Media>FFmpeg
Labels: Security_Severity-Low Security_Impact-Stable
Owner: dalecur...@chromium.org
dalecurtis@chromium.org: Assigning three FFmpeg issues to you. Please triage and assign appropriately.

Comment 2 by vakh@chromium.org, Sep 4 2016

Labels: OS-Linux OS-Mac OS-Windows

Comment 3 by sheriffbot@chromium.org, Sep 4 2016

Project Member
Labels: Pri-2

Comment 4 by sheriffbot@chromium.org, Sep 4 2016

Project Member
Status: Assigned (was: Unconfirmed)

Comment 5 by dalecur...@chromium.org, Sep 6 2016

Cc: -wolenetz@chromium.org dalecur...@chromium.org
Owner: wolenetz@chromium.org

Comment 6 by wolenetz@chromium.org, Sep 6 2016

Blocking: 591845

Comment 7 by p...@paulmehta.com, Sep 9 2016

Issue 1) mov_read_senc - zero heap allocation

Patch:
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mov.c?rcl=0&l=3959

3958
-       if (atom.size < 8) {
+       if (atom.size < 8 || atom.size > UINT_MAX) {
-           av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" too small\n", atom.size);
+           av_log(c->fc, AV_LOG_ERROR, "senc atom size %"PRId64" invalid\n", atom.size);
3961        return AVERROR_INVALIDDATA;
3962    }
3963
3964    /* save the auxiliary info as is */
3965    auxiliary_info_size = atom.size - 8;
3966
3967    sc->cenc.auxiliary_info = av_malloc(auxiliary_info_size);


 Issue 2 ) mov_read_saiz  - memory leak / zero sized heap allocation

Patch:

        if (atom.size <= atom_header_size) {
            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;
        
        sc->cenc.auxiliary_info_sizes = av_malloc(data_size); // <---------------------------------------- zero-allocation 
        if (!sc->cenc.auxiliary_info_sizes) {
            return AVERROR(ENOMEM);
        }
        
        
 Issue 3 ) mov_read_udta_string

Patch:
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mov.c?rcl=0&l=382

382                uint32_t index = AV_RB32(&atom.type);
-                  if (index < c->meta_keys_count) {
+                  if (index < c->meta_keys_count && index > 0) {
384                    key = c->meta_keys[index];
385                } else {
386                    av_log(c->fc, AV_LOG_WARNING,
387                           "The index of 'data' is out of range: %d >= %d.\n",
388                           index, c->meta_keys_count);
389                }

Comment 8 by wolenetz@chromium.org, Dec 6 2016

Thanks Paul for reporting.

Current ffmpeg ToT looks like it is still broken.

I'll prepare a downstream patch shortly. Since this predated the recent FFmpeg M56 roll ( issue 591845 ), this may well need to go to M55, too.

Comment 9 by bugdroid1@chromium.org, Dec 6 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/8622f9398e7c89a664c4c2ceff9d35b89ff17bb5

commit 8622f9398e7c89a664c4c2ceff9d35b89ff17bb5
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Dec 06 20:54:23 2016

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

Core of patch is from paul@paulmehta.com

BUG= 643952 
R=dalecurtis@chromium.org

Change-Id: Ie464d4d0df044725fcb0a6d2fa49847580de2731
Reviewed-on: https://chromium-review.googlesource.com/417161
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>

[modify] https://crrev.com/8622f9398e7c89a664c4c2ceff9d35b89ff17bb5/libavformat/mov.c
[modify] https://crrev.com/8622f9398e7c89a664c4c2ceff9d35b89ff17bb5/chromium/patches/README

Comment 10 by bugdroid1@chromium.org, Dec 7 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/324c32a13dbdf0e585f96d6e3bd010fb890414af

commit 324c32a13dbdf0e585f96d6e3bd010fb890414af
Author: wolenetz <wolenetz@chromium.org>
Date: Wed Dec 07 02:34:55 2016

Roll src/third_party/ffmpeg/ 16cdcb08b..26be2ced9 (8 commits).

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/16cdcb08bb1c..26be2ced9076

$ git log 16cdcb08b..26be2ced9 --date=short --no-merges --format='%ad %ae %s'
2016-12-06 wolenetz avcodec/flacdec: Fix undefined shift in decode_subframe()
2016-12-06 wolenetz avcodec/get_bits: Fix get_sbits_long(0)
2016-12-06 wolenetz avformat/utils: Check start/end before computing duration in update_stream_timings()
2016-12-06 wolenetz avformat/oggparsespeex: Check frames_per_packet and packet_size
2016-12-06 wolenetz avcodec/flacdec: Fix signed integer overflow in decode_subframe_fixed()
2016-12-06 wolenetz avcodec/flacdsp_template: Fix undefined shift in flac_decorrelate_indep_c
2016-12-06 wolenetz lavf/mov.c: Avoid heap allocation wraps and OOB in mov_read_{senc,saiz,udta_string}()
2016-12-06 wolenetz lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid

BUG= 643951 , 643952 ,668346,640912,635422,637428,640889,639961
TBR=dalecurtis@chromium.org

Review-Url: https://codereview.chromium.org/2557953002
Cr-Commit-Position: refs/heads/master@{#436843}

[modify] https://crrev.com/324c32a13dbdf0e585f96d6e3bd010fb890414af/DEPS

Comment 11 by wolenetz@chromium.org, Dec 7 2016

Labels: M-57
Status: Fixed (was: Assigned)
Merge request to M56 is pending stabilization in next Canary.

Comment 12 by sheriffbot@chromium.org, Dec 8 2016

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 13 by wolenetz@chromium.org, Dec 8 2016

Labels: Merge-Request-56
In today's Canary. Requesting M56 merge; would be a buildspec-only merge (to get #9 into M56 ffmpeg DEPS)

Comment 14 by dimu@chromium.org, Dec 8 2016

Labels: -Merge-Request-56 Merge-Review-56 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.

Comment 15 by bustamante@google.com, Dec 13 2016

Labels: -Merge-Review-56 Merge-Approved-56
This change meets the bar and is approved for merge in M56

Comment 16 by bugdroid1@chromium.org, Dec 13 2016

Project Member

Comment 17 by wolenetz@chromium.org, Dec 13 2016

Labels: -Merge-Approved-56 merge-merged-2924 M-56
M56 branch 2924 buildspec containing fix's merge to M56 landed in #16. Manually updating labels...

Comment 18 by awhalley@chromium.org, Jan 24 2017

Labels: Release-0-M56

Comment 19 by wolenetz@chromium.org, Feb 10 2017

Updated version of the downstream patch is out for review upstream at https://patchwork.ffmpeg.org/patch/2459/ and https://patchwork.ffmpeg.org/patch/2471/

Comment 20 by wolenetz@chromium.org, Feb 11 2017

Cc: hubbe@chromium.org

Comment 21 by bugdroid1@chromium.org, Feb 13 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/c0f755d3b1dbdf36f93518150c200bd29173aaf7

commit c0f755d3b1dbdf36f93518150c200bd29173aaf7
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Mon Feb 13 19:56:57 2017

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>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
(cherry picked from commit 36aba43bd5fae8595dd9a566fbcfbbea63f0fca3)

This is upstream's slightly updated version of the
mov_read_{senc,saiz}() portion of the previous downstream patch
8622f9398e7c89a664c4c2ceff9d35b89ff17bb5

BUG= 643952 

Change-Id: I915ddda3187f09640a5740e6bd4584304ec92318
Reviewed-on: https://chromium-review.googlesource.com/441272
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>

[modify] https://crrev.com/c0f755d3b1dbdf36f93518150c200bd29173aaf7/libavformat/mov.c
[modify] https://crrev.com/c0f755d3b1dbdf36f93518150c200bd29173aaf7/chromium/patches/README

Comment 22 by bugdroid1@chromium.org, Feb 13 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/4b0afde9bdb5202726d66864a4d422ee4facbd54

commit 4b0afde9bdb5202726d66864a4d422ee4facbd54
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Mon Feb 13 21:49:06 2017

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>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
(cherry picked from commit 9bbdf5d921ef57e1698f64981e4ea04db7c56fb5)

This is upstream's slightly updated version of the udta_string portion
of the previous downstream patch
8622f9398e7c89a664c4c2ceff9d35b89ff17bb5

BUG= 643952 

Change-Id: I27b9bded293a9220eb8d5cc86e43d99036846f9d
Reviewed-on: https://chromium-review.googlesource.com/441110
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>

[modify] https://crrev.com/4b0afde9bdb5202726d66864a4d422ee4facbd54/libavformat/mov.c
[modify] https://crrev.com/4b0afde9bdb5202726d66864a4d422ee4facbd54/chromium/patches/README

Comment 23 by bugdroid1@chromium.org, Feb 14 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/518d37c3c167ec8f0a790a4eda037b6e643ebc8f

commit 518d37c3c167ec8f0a790a4eda037b6e643ebc8f
Author: wolenetz <wolenetz@chromium.org>
Date: Tue Feb 14 00:54:01 2017

Roll src/third_party/ffmpeg/ a77cdbfeb..38d84d205 (4 commits).

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/a77cdbfeb7b6..38d84d205cd8 (4 commits).

$ git log a77cdbfeb..38d84d205 --date=short --no-merges --format='%ad %ae %s'
2017-02-13 wolenetz Remove cherry-picks from chromium/patches/README
2017-02-13 wolenetz lavf/mov.c: Avoid OOB in mov_read_udta_string()
2017-02-10 wolenetz lavf/mov.c: Avoid heap allocation wraps in mov_read_{senc,saiz}()
2017-02-10 dalecurtis h264dec: handle zero-sized NAL units in get_last_needed_nal()

Created with:
  roll-dep src/third_party/ffmpeg

R=dalecurtis@chromium.org
TBR=hubbe@chromium.org

BUG= 643952 , 690184 

Review-Url: https://codereview.chromium.org/2694913002
Cr-Commit-Position: refs/heads/master@{#450178}

[modify] https://crrev.com/518d37c3c167ec8f0a790a4eda037b6e643ebc8f/DEPS

Comment 24 by sheriffbot@chromium.org, Mar 16 2017

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment