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 descriptionI'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 2016, Project Member
Sep 4 2016, Project Member
Sep 6 2016,
Sep 6 2016,
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 } 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. Dec 6 2016, Project 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 2016, Project 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 2016, Project 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 2016, Project 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 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/ Feb 11 2017,
Feb 13 2017, Project 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 2017, Project 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 2017, Project 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 2017, Project 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