Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 643950 Security: FFMPEG MP4 Decoder chrome_child!mov_read_hdlr heap allocation wrap
Starred by 1 user Reported by p...@paulmehta.com, Sep 3 2016 Back to list
Status: Fixed
Owner:
Closed: Dec 8
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_hdlr

Chrome_child.dll version: 53.0.2785.89


int64_t truncation results in av_malloc(0), and subsequent memory corruption may be semi-useless heap corruption bugs (requires >4GB file, and is likely just a DOS)


This is an interesting case... an incorrect size check between a size_t and int64_t, followed by a truncation. 

If a non-negative title_size > 4GB will title_size will pass the check "if (title_size > 0)", and then truncate to zero because the allocator is 32-bit.

When atom.size == 0x1'000000017 (I've tested this with a video file > 4GB).





static int mov_read_hdlr(MOVContext *c, AVIOContext *pb, MOVAtom atom)
{
    AVStream *st;
    uint32_t type;
    uint32_t av_unused ctype;
    int64_t title_size;
    char *title_str;
    int ret;

    avio_r8(pb); /* version */
    avio_rb24(pb); /* flags */

    /* component type */
    ctype = avio_rl32(pb);
    type = avio_rl32(pb); /* component subtype */

    av_log(c->fc, AV_LOG_TRACE, "ctype= %.4s (0x%08x)\n", (char*)&ctype, ctype);
    av_log(c->fc, AV_LOG_TRACE, "stype= %.4s\n", (char*)&type);

    if (c->trak_index < 0) {  // meta not inside a trak
        if (type == MKTAG('m','d','t','a')) {
            c->found_hdlr_mdta = 1;
        }
        return 0;
    }

    st = c->fc->streams[c->fc->nb_streams-1];

    if     (type == MKTAG('v','i','d','e'))
        st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
    else if (type == MKTAG('s','o','u','n'))
        st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
    else if (type == MKTAG('m','1','a',' '))
        st->codec->codec_id = AV_CODEC_ID_MP2;
    else if ((type == MKTAG('s','u','b','p')) || (type == MKTAG('c','l','c','p')))
        st->codec->codec_type = AVMEDIA_TYPE_SUBTITLE;

    avio_rb32(pb); /* component  manufacture */
    avio_rb32(pb); /* component flags */
    avio_rb32(pb); /* component flags mask */

    title_size = atom.size - 24; // <--------------------------------------------------------- When atom.size ==  0xFFFFFFFF + 0x18 (as an int64_t), 
    if (title_size > 0) {
        title_str = av_malloc(title_size + 1); /* Add null terminator */  // <------------------------------ title_size will truncate to zero here (because the allocator is 32-bit)
        if (!title_str)
            return AVERROR(ENOMEM);

        ret = ffio_read_size(pb, title_str, title_size);
        if (ret < 0) {
            av_freep(&title_str);
            return ret;
        }
        title_str[title_size] = 0;
        if (title_str[0]) {
            int off = (!c->isom && title_str[0] == title_size - 1);
            av_dict_set(&st->metadata, "handler_name", title_str + off, 0);
        }
        av_freep(&title_str);
    }

    return 0;
}


-------------------------------------------------- Disassembly ----------------------------------------------------


.text:1159656A                 mov     edi, dword ptr [ebp+atom.size]    <-------------------- when atom.size == 0x1'000000017 (requires a video file > 4GB, but I've proven that this doable)
.text:1159656D                 add     esp, 0Ch
.text:11596570                 mov     eax, dword ptr [ebp+atom.size+4]  
.text:11596573                 sub     edi, 18h							<----------- Positive Int64 causes a zero-allocation size when intrinsically cast to UINT32 (void *__cdecl av_malloc(unsigned int size))
.text:11596576                 sbb     eax, 0
.text:11596579                 mov     [ebp+c], eax
.text:1159657C                 js      loc_11596616
.text:11596582                 jg      short loc_1159658C
.text:11596584                 test    edi, edi
.text:11596586                 jz      loc_11596616
.text:1159658C
.text:1159658C loc_1159658C:                           ; CODE XREF: mov_read_hdlr+101j
.text:1159658C                 lea     eax, [edi+1]				<------------------------ UINT32 wrap
.text:1159658F                 push    eax             ; size
.text:11596590                 call    _av_malloc				<------------------------ allocate zero			void *__cdecl av_malloc(unsigned int size)
.text:11596595                 mov     [ebp+title_str], eax
.text:11596598                 pop     ecx
.text:11596599                 test    eax, eax
.text:1159659B                 jnz     short loc_115965A2
.text:1159659D                 push    0FFFFFFF4h
.text:1159659F                 pop     eax
.text:115965A0                 jmp     short loc_11596618
.text:115965A2 ; ---------------------------------------------------------------------------
.text:115965A2
.text:115965A2 loc_115965A2:                           ; CODE XREF: mov_read_hdlr+11Aj
.text:115965A2                 push    edi             ; size
.text:115965A3                 push    eax             ; buf
.text:115965A4                 push    esi             ; s
.text:115965A5                 call    _ffio_read_size






Breakpoint 0 hit
eax=00000000 ebx=0f99ade0 ecx=03af1100 edx=05d5ea94 esi=03af1100 edi=ffffffff
eip=6e4a6590 esp=05d5eab4 ebp=05d5ead4 iopl=0         nv up ei ng nz na pe nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000286
chrome_child!mov_read_hdlr+0x10f:
6e4a6590 e8c4c0fdff      call    chrome_child!av_malloc (6e482659)


eax=0535c1e0 ebx=0f99ade0 ecx=00000020 edx=0535c1d8 esi=03af1100 edi=ffffffff
eip=6e4a6595 esp=05d5eab4 ebp=05d5ead4 iopl=0         nv up ei pl nz na po nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000202
chrome_child!mov_read_hdlr+0x114:
6e4a6595 8945fc          mov     dword ptr [ebp-4],eax ss:002b:05d5ead0=726c6468
0:011> dd 0535c1e0 
0535c1e0  6eda8a51 00000000 00001000 00000900
0535c1f0  00000002 05fb3af8 006a99b0 00000000
0535c200  4393b0de 88000100 00000000 00000000
0535c210  00000000 00000000 00000000 00000000
0535c220  00000000 00000000 00000000 00000000
0535c230  4393b0d8 8800cc90 00000001 6d28910e
0535c240  6cfec83c e20e4602 00000001 00000004
0535c250  838acb34 00000003 00609ea8 0062b6c8



chrome_child!mov_read_hdlr+0x114 [c:\b\build\slave\win\build\src\third_party\ffmpeg\libavformat\mov.c @ 679]
chrome_child!mov_read_default+0x340 [c:\b\build\slave\win\build\src\third_party\ffmpeg\libavformat\mov.c @ 4221]
chrome_child!mov_read_trak+0x70 [c:\b\build\slave\win\build\src\third_party\ffmpeg\libavformat\mov.c @ 3026]
chrome_child!mov_read_default+0x340 [c:\b\build\slave\win\build\src\third_party\ffmpeg\libavformat\mov.c @ 4221]
chrome_child!mov_read_header+0xa6 [c:\b\build\slave\win\build\src\third_party\ffmpeg\libavformat\mov.c @ 4688]
chrome_child!avformat_open_input+0x22f [c:\b\build\slave\win\build\src\third_party\ffmpeg\libavformat\utils.c @ 498]
chrome_child!media::FFmpegGlue::OpenContext+0xec [c:\b\build\slave\win\build\src\media\filters\ffmpeg_glue.cc @ 181]
chrome_child!base::Callback<bool __cdecl(void),1>::Run+0xa [c:\b\build\slave\win\build\src\base\callback.h @ 389]
chrome_child!base::internal::ReturnAsParamAdapter<bool>+0xd [c:\b\build\slave\win\build\src\base\task_runner_util.h @ 22]
chrome_child!base::internal::RunnableAdapter<void (__cdecl*)(base::Callback<bool __cdecl(void),1> const &,bool *)>::Run+0xc [c:\b\build\slave\win\build\src\base\bind_internal.h @ 144]
chrome_child!base::internal::InvokeHelper<0,void>::MakeItSo+0xc [c:\b\build\slave\win\build\src\base\bind_internal.h @ 296]
chrome_child!base::internal::Invoker<base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl*)(base::Callback<bool __cdecl(void),1> const &,bool *)>,base::Callback<bool __cdecl(void),1> const &,bool * &>,void __cdecl(void)>::RunImpl+0xc [c:\b\build\slave\win\build\src\base\bind_internal.h @ 363]
chrome_child!base::internal::Invoker<base::internal::BindState<base::internal::RunnableAdapter<void (__cdecl*)(base::Callback<bool __cdecl(void),1> const &,bool *)>,base::Callback<bool __cdecl(void),1> const &,bool * &>,void __cdecl(void)>::Run+0x12 [c:\b\build\slave\win\build\src\base\bind_internal.h @ 342]
chrome_child!base::Callback<void __cdecl(void),1>::Run+0x8 [c:\b\build\slave\win\build\src\base\callback.h @ 389]
chrome_child!base::`anonymous namespace'::PostTaskAndReplyRelay::Run+0x1c [c:\b\build\slave\win\build\src\base\threading\post_task_and_reply_impl.cc @ 43]
chrome_child!base::Callback<void __cdecl(void),1>::Run+0xb [c:\b\build\slave\win\build\src\base\callback.h @ 389]
chrome_child!base::debug::TaskAnnotator::RunTask+0x16f [c:\b\build\slave\win\build\src\base\debug\task_annotator.cc @ 51]
chrome_child!base::MessageLoop::RunTask+0x2cd [c:\b\build\slave\win\build\src\base\message_loop\message_loop.cc @ 494]
chrome_child!base::MessageLoop::DeferOrRunPendingTask+0x63 [c:\b\build\slave\win\build\src\base\message_loop\message_loop.cc @ 502]
chrome_child!base::MessageLoop::DoWork+0x42e [c:\b\build\slave\win\build\src\base\message_loop\message_loop.cc @ 625]




 
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
Status: Untriaged
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
Project Member Comment 3 by sheriffbot@chromium.org, Sep 4 2016
Labels: Pri-2
Project Member Comment 4 by sheriffbot@chromium.org, Sep 4 2016
Status: Assigned
Cc: -wolenetz@chromium.org dalecur...@chromium.org
Owner: wolenetz@chromium.org
Blocking: 591845
Comment 7 by p...@paulmehta.com, Sep 9 2016
Patch:
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavformat/mov.c?rcl=0&l=678

677    title_size = atom.size - 24;
678    if (title_size > 0) {
+           if (title_size >= UINT_MAX)
+               return AVERROR_INVALIDDATA;
679        title_str = av_malloc(title_size + 1); /* Add null terminator */
680        if (!title_str)
681            return AVERROR(ENOMEM)

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.
Downstream patch is out for review @ https://chromium-review.googlesource.com/#/c/416271/ (will need DEPS roll and subsequent merging/cherry-picking and pushing upstream).
Project Member Comment 10 by bugdroid1@chromium.org, Dec 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/fd878457cd55690d4a27d74411b68a30c9fb2313

commit fd878457cd55690d4a27d74411b68a30c9fb2313
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Sat Dec 03 02:10:39 2016

lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr

Core of patch is from paul@paulmehta.com

BUG= 643950 
R=dalecurtis@chromium.org

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

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

Labels: M-57 Merge-Request-56
Status: Fixed
In today's Canary. Requesting M56 merge; would be a buildspec-only merge (to get #10 into M56 ffmpeg DEPS).
Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Cc: bustamante@chromium.org
bustamante@ -- FYI - The merge *will* be a buildspec update (Since #11 did a DEPS roll to pull this fix into trunk, but that DEPS roll didn't reference this bug, the merge-review process wasn't done for this following my request in #12. Please expedite any necessary review; unless I hear otherwise, I'll assume this remains approved.)
Project Member Comment 15 by sheriffbot@chromium.org, Dec 9
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 16 by sheriffbot@chromium.org, Dec 12
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 17 by bugdroid1@chromium.org, Dec 13
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 #17. Manually updating labels...
Labels: -Hotlist-Merge-Approved
Labels: Release-0-M56
Labels: CVE-2017-5025
Updated upstream patch is in review at https://patchwork.ffmpeg.org/patch/2446/
(This supersedes the previous upstream patch at https://patchwork.ffmpeg.org/patch/1785/)
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/+/be66b658991cdcb8ca87efade290ed99ca04a76c

commit be66b658991cdcb8ca87efade290ed99ca04a76c
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Thu Feb 09 23:23:09 2017

lavf/mov.c: Avoid heap allocation wrap in mov_read_hdlr

Core of patch is from paul@paulmehta.com
Reference https://crbug.com/643950

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
Check value reduced as the code does not support larger lengths
(cherry picked from commit fd30e4d57fe5841385f845440688505b88c0f4a9)

This is upstream's slightly modified version of the previous downstream
patch.
BUG= 643950 

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

[modify] https://crrev.com/be66b658991cdcb8ca87efade290ed99ca04a76c/libavformat/mov.c
[modify] https://crrev.com/be66b658991cdcb8ca87efade290ed99ca04a76c/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 17
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