Issue 643951: Security: FFMPEG MP4 Decoder chrome_child!mov_read_uuid heap allocation wrap
Reported by
p...@paulmehta.com,
Sep 3 2016
|
||||||||||||||||
Issue descriptionSource file: mov.c chrome_child!mov_read_uuid chrome_child.dll version: 53.0.2785.89 int64_t truncation results in av_malloc(0), and subsequent memory corruption. Might be semi-useless heap corruption bugs (requires >4GB file, and is likely just a DOS). This bug is nearly identical to the mov_read_hdlr, but while I have observed the mov_read_hdlr heap allocation size wrap. All this info was statically determined; I see no reason why it wouldn't yield the same result, but I have not demo'd this bug. ---- Breakpoints ---- bu chrome_child!mov_read_uuid static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom) { int ret; uint8_t uuid[16]; static const uint8_t uuid_isml_manifest[] = { 0xa5, 0xd4, 0x0b, 0x30, 0xe8, 0x14, 0x11, 0xdd, 0xba, 0x2f, 0x08, 0x00, 0x20, 0x0c, 0x9a, 0x66 }; static const uint8_t uuid_xmp[] = { 0xbe, 0x7a, 0xcf, 0xcb, 0x97, 0xa9, 0x42, 0xe8, 0x9c, 0x71, 0x99, 0x94, 0x91, 0xe3, 0xaf, 0xac }; if (atom.size < sizeof(uuid) || atom.size == INT64_MAX) return AVERROR_INVALIDDATA; ret = avio_read(pb, uuid, sizeof(uuid)); if (ret < 0) { return ret; } else if (ret != sizeof(uuid)) { return AVERROR_INVALIDDATA; } if (!memcmp(uuid, uuid_isml_manifest, sizeof(uuid))) { uint8_t *buffer, *ptr; char *endptr; size_t len = atom.size - sizeof(uuid); if (len < 4) { return AVERROR_INVALIDDATA; } ret = avio_skip(pb, 4); // zeroes len -= 4; buffer = av_mallocz(len + 1); if (!buffer) { return AVERROR(ENOMEM); } ret = avio_read(pb, buffer, len); if (ret < 0) { av_free(buffer); return ret; } else if (ret != len) { av_free(buffer); return AVERROR_INVALIDDATA; } ptr = buffer; while ((ptr = av_stristr(ptr, "systemBitrate=\""))) { ptr += sizeof("systemBitrate=\"") - 1; c->bitrates_count++; c->bitrates = av_realloc_f(c->bitrates, c->bitrates_count, sizeof(*c->bitrates)); if (!c->bitrates) { c->bitrates_count = 0; av_free(buffer); return AVERROR(ENOMEM); } errno = 0; ret = strtol(ptr, &endptr, 10); if (ret < 0 || errno || *endptr != '"') { c->bitrates[c->bitrates_count - 1] = 0; } else { c->bitrates[c->bitrates_count - 1] = ret; } } av_free(buffer); } else if (!memcmp(uuid, uuid_xmp, sizeof(uuid))) { uint8_t *buffer; size_t len = atom.size - sizeof(uuid); // <------------------------------------------------------------------ when atom.size (int64_t) == 0xFFFFFFFF + sizeof(uuid); buffer = av_mallocz(len + 1); // results in a allocation of size == 0 if (!buffer) { return AVERROR(ENOMEM); } ret = avio_read(pb, buffer, len); // and subsequent memory corruption if (ret < 0) { av_free(buffer); return ret; } else if (ret != len) { av_free(buffer); return AVERROR_INVALIDDATA; } if (c->export_xmp) { buffer[len] = '\0'; av_dict_set(&c->fc->metadata, "xmp", buffer, 0); } av_free(buffer); } return 0; } --------------------------------------------------- DISASSEMBLY --------------------------------------------------- .text:11595A61 loc_11595A61: ; CODE XREF: mov_read_uuid+84j .text:11595A61 mov eax, [ebx+ecx*4] .text:11595A64 mov ebx, offset uuid_isml_manifest .text:11595A69 cmp eax, [ebx+ecx*4] .text:11595A6C lea ebx, [ebp+uuid] .text:11595A6F jnz loc_11595B98 .text:11595A75 inc ecx .text:11595A76 cmp ecx, edx .text:11595A78 jnz short loc_11595A61 .text:11595A7A mov ebx, dword ptr [ebp+atom.size] .text:11595A7D add ebx, 0FFFFFFF0h .text:11595A80 cmp ebx, edx .text:11595A82 jb loc_11595C26 .text:11595A88 push 0 .text:11595A8A push edx ; offset .text:11595A8B push edi ; s .text:11595A8C call _avio_skip .text:11595A91 sub ebx, 4 .text:11595A94 lea eax, [ebx+1] <------------------------------ Allocation wrap when atom.size (int64_t) == 0xFFFFFFFF + sizeof(uuid); .text:11595A97 push eax ; size .text:11595A98 call _av_mallocz .text:11595A9D add esp, 10h .text:11595AA0 mov [ebp+block], eax .text:11595AA3 test eax, eax .text:11595AA5 jnz short loc_11595AAF Sep 4 2016, Project Member
Sep 4 2016, Project Member
Sep 6 2016,
Sep 6 2016,
Sep 9 2016,Patch: https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mov.c?rcl=0&l=3850 3850 } else if (!memcmp(uuid, uuid_xmp, sizeof(uuid))) { 3851 uint8_t *buffer; 3852 size_t len = atom.size - sizeof(uuid); + if (len >= UINT_MAX) + return AVERROR_INVALIDDATA; 3853 3854 buffer = av_mallocz(len + 1); 3855 if (!buffer) { 3856 return AVERROR(ENOMEM); Dec 6 2016,Thanks for reporting this, Paul. 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. It also looks like there's a similar wrapping potential in the "then" clause just prior to the "else" clause in the snippet in #6, a fix for which I'll also include in the patch. Dec 6 2016,Sounds good Dec 6 2016,> It also looks like there's a similar wrapping potential in the "then" clause just prior to the "else" clause in the snippet in #6, a fix for which I'll also include in the patch. This is already protected against in that clause. Dec 6 2016, Project MemberThe following revision refers to this bug: https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/9d45f272a682b0ea831c20e36f696e15cc0c55fe commit 9d45f272a682b0ea831c20e36f696e15cc0c55fe Author: Matt Wolenetz <wolenetz@chromium.org> Date: Tue Dec 06 20:33:08 2016 lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid Core of patch is from paul@paulmehta.com BUG= 643951 R=dalecurtis@chromium.org Change-Id: Ib4dd9b30c7d882a37bec89ddd56d6691851ec61c Reviewed-on: https://chromium-review.googlesource.com/417133 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> [modify] https://crrev.com/9d45f272a682b0ea831c20e36f696e15cc0c55fe/libavformat/mov.c [modify] https://crrev.com/9d45f272a682b0ea831c20e36f696e15cc0c55fe/chromium/patches/README Dec 6 2016, Project MemberThe following revision refers to this bug: https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/9d45f272a682b0ea831c20e36f696e15cc0c55fe commit 9d45f272a682b0ea831c20e36f696e15cc0c55fe Author: Matt Wolenetz <wolenetz@chromium.org> Date: Tue Dec 06 20:33:08 2016 lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid Core of patch is from paul@paulmehta.com BUG= 643951 R=dalecurtis@chromium.org Change-Id: Ib4dd9b30c7d882a37bec89ddd56d6691851ec61c Reviewed-on: https://chromium-review.googlesource.com/417133 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> [modify] https://crrev.com/9d45f272a682b0ea831c20e36f696e15cc0c55fe/libavformat/mov.c [modify] https://crrev.com/9d45f272a682b0ea831c20e36f696e15cc0c55fe/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 #11 into M56 ffmpeg DEPS) Dec 8 2016,
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review. Dec 13 2016,
This also meets the bar and is approved for merge into 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 #18. Manually updating labels... Jan 24 2017,
Jan 25 2017,
Feb 8 2017,Updated upstream patch is in review at https://patchwork.ffmpeg.org/patch/2447/ (This supersedes the previous upstream patch at https://patchwork.ffmpeg.org/patch/1786/) Feb 8 2017,
Feb 9 2017, Project MemberThe following revision refers to this bug: https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/a77cdbfeb7b629eb3a5012d7c6c94ef11e0488be commit a77cdbfeb7b629eb3a5012d7c6c94ef11e0488be Author: Matt Wolenetz <wolenetz@chromium.org> Date: Thu Feb 09 23:27:32 2017 lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid Core of patch is from paul@paulmehta.com Reference https://crbug.com/643951 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> Check value reduced as the code does not support values beyond INT_MAX Also the check is moved to a more common place and before integer truncation (cherry picked from commit 2d453188c2303da641dafb048dc1806790526dfd) This is upstream's slightly modified version of the previous downstream patch. BUG= 643951 Change-Id: I7981cd60ccefdd8b8248de21b8b31cf77bc20954 Reviewed-on: https://chromium-review.googlesource.com/439534 Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> [modify] https://crrev.com/a77cdbfeb7b629eb3a5012d7c6c94ef11e0488be/libavformat/mov.c [modify] https://crrev.com/a77cdbfeb7b629eb3a5012d7c6c94ef11e0488be/chromium/patches/README Feb 10 2017, Project MemberThe following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/18995b57d57d54ca64ad66a2ff6d89501ea557a6 commit 18995b57d57d54ca64ad66a2ff6d89501ea557a6 Author: wolenetz <wolenetz@chromium.org> Date: Fri Feb 10 00:48:47 2017 Roll src/third_party/ffmpeg/ 239c9f9e2..a77cdbfeb (2 commits). https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/239c9f9e2754..a77cdbfeb7b6 $ git log 239c9f9e2..a77cdbfeb --date=short --no-merges --format='%ad %ae %s' 2017-02-09 wolenetz lavf/mov.c: Avoid heap allocation wrap in mov_read_uuid 2017-02-08 wolenetz lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr Created with: roll-dep src/third_party/ffmpeg BUG= 643950 , 643951 TBR=hubbe@chromium.org Review-Url: https://codereview.chromium.org/2685013005 Cr-Commit-Position: refs/heads/master@{#449497} [modify] https://crrev.com/18995b57d57d54ca64ad66a2ff6d89501ea557a6/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 Apr 25 2018,
|
||||||||||||||||
►
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 OS-Linux OS-Mac OS-Windows
Owner: dalecur...@chromium.org