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).
, Sep 4 2016
, Sep 4 2016Project Member
, Sep 4 2016Project Member
, Sep 6 2016
, Sep 6 2016
, Sep 9 2016Issue 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 } , Dec 6 2016Thanks 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. , Dec 6 2016Project MemberThe 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 , Dec 7 2016Project MemberThe 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 , Dec 7 2016
Merge request to M56 is pending stabilization in next Canary. , Dec 8 2016Project Member
, Dec 8 2016
In today's Canary. Requesting M56 merge; would be a buildspec-only merge (to get #9 into M56 ffmpeg DEPS) , Dec 8 2016
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review. , Dec 13 2016
This change meets the bar and is approved for merge in M56 , Dec 13 2016Project MemberThe following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/343d2bb3357d276a1d23134d89c43417bed02a2c commit 343d2bb3357d276a1d23134d89c43417bed02a2c Author: Matt Wolenetz <wolenetz@chromium.org> Date: Tue Dec 13 21:04:36 2016 , Dec 13 2016
M56 branch 2924 buildspec containing fix's merge to M56 landed in #16. Manually updating labels... , Jan 24 2017
, Feb 10 2017Updated version of the downstream patch is out for review upstream at https://patchwork.ffmpeg.org/patch/2459/ and https://patchwork.ffmpeg.org/patch/2471/ , Feb 11 2017
, Feb 13 2017Project MemberThe 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 , Feb 13 2017Project MemberThe 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 , Feb 14 2017Project MemberThe 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 , Mar 16 2017Project Member
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 |
|||||||||||||||
Comment 1 by vakh@chromium.org, Sep 4 2016
Components: Internals>Media>FFmpeg
Labels: Security_Severity-Low Security_Impact-Stable
Owner: dalecur...@chromium.org