Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 643951 Security: FFMPEG MP4 Decoder chrome_child!mov_read_uuid heap allocation wrap
Starred by 1 user Reported by p...@paulmehta.com, Sep 3 2016 Back to list
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
Source 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
 
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 OS-Linux OS-Mac OS-Windows
Owner: dalecur...@chromium.org
dalecurtis@chromium.org: Assigning three FFmpeg issues to you. Please triage and assign appropriately.
Project Member Comment 2 by sheriffbot@chromium.org, Sep 4 2016
Labels: Pri-2
Project Member Comment 3 by sheriffbot@chromium.org, Sep 4 2016
Status: Assigned
Cc: -wolenetz@chromium.org dalecur...@chromium.org
Owner: wolenetz@chromium.org
Blocking: 591845
Comment 6 by p...@paulmehta.com, 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);
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.
Comment 8 by p...@paulmehta.com, Dec 6 2016
Sounds good
> 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.
Project Member Comment 10 by bugdroid1@chromium.org, Dec 6 2016
The 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

Project Member Comment 11 by bugdroid1@chromium.org, Dec 6 2016
The 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

Project Member Comment 12 by bugdroid1@chromium.org, Dec 7 2016
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

Labels: M-57
Status: Fixed
Merge request to M56 is pending stabilization in next Canary.
Project Member Comment 14 by sheriffbot@chromium.org, Dec 8 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-56
In today's Canary. Requesting M56 merge; would be a buildspec-only merge (to get #11 into M56 ffmpeg DEPS)
Comment 16 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.
Labels: -Merge-Review-56 Merge-Approved-56
This also meets the bar and is approved for merge into M56
Project Member Comment 18 by bugdroid1@chromium.org, Dec 13 2016
The 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

Labels: -Merge-Approved-56 merge-merged-2924 M-56
M56 branch 2924 buildspec containing fix's merge to M56 landed in #18. Manually updating labels...
Labels: Release-0-M56
Labels: CVE-2017-5024
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/)
Cc: hubbe@chromium.org
Project Member Comment 24 by bugdroid1@chromium.org, Feb 9
The 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

Project Member Comment 25 by bugdroid1@chromium.org, Feb 10
The 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

Project Member Comment 26 by sheriffbot@chromium.org, Mar 16
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