Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users
Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: OOM situation can result in heap buffer overflow in CFX_BinaryBuf (pdfium)
Reported by cloudfuz...@gmail.com, Jan 4 2015 Back to list
VULNERABILITY DETAILS

CFX_BinaryBuf::ExpandBuf is implemented as follows:

void CFX_BinaryBuf::ExpandBuf(FX_STRSIZE add_size)
{
    FX_STRSIZE new_size = add_size + m_DataSize;
    if (m_AllocSize >= new_size) {
        return;
    }
    int alloc_step;
    if (m_AllocStep == 0) {
        alloc_step = m_AllocSize / 4;
        if (alloc_step < 128 ) {
            alloc_step = 128;
        }
    } else {
        alloc_step = m_AllocStep;
    }
    new_size = (new_size + alloc_step - 1) / alloc_step * alloc_step;
    FX_LPBYTE pNewBuffer = m_pBuffer;
    if (pNewBuffer) {
        pNewBuffer = FX_Realloc(FX_BYTE, m_pBuffer, new_size);
    } else {
        pNewBuffer = FX_Alloc(FX_BYTE, new_size);
    }
    if (pNewBuffer) {
        m_pBuffer = pNewBuffer;
        m_AllocSize = new_size;
    }
}

The function doesn't return an error code, so a calling function can't tell whether a reallocation failed. If the call to FX_Realloc fails pNewBuffer will be zero and m_pBuffer will not be reassigned and the code will continue to run with the insufficient buffer size. This can for example be exploited through calls to CFX_WideTextBuf::AppendChar:

void CFX_WideTextBuf::AppendChar(FX_WCHAR ch)
{
    if (m_AllocSize < m_DataSize + (FX_STRSIZE)sizeof(FX_WCHAR)) {
        ExpandBuf(sizeof(FX_WCHAR));
    }
    ASSERT(m_pBuffer != NULL);
    *(FX_WCHAR*)(m_pBuffer + m_DataSize) = ch;
    m_DataSize += sizeof(FX_WCHAR);
}

VERSION
Chrome Version: tested against latest pdfium_test 32 and 64-bit.

REPRODUCTION CASE
Reproduction of OOM issues is a little tricky. I have written a custom ASAN patch which allows me to limit the overall malloc'ed size, which makes issues like this easier to find and reliable to reproduce. The patch simulates the behaviour of a real world allocator (return null on failed allocation). The patch is attached as asan_alloc_limit.patch

Using an ASAN build with this patch allows us to reproduce the issue reliable using the attached test.pdf:

ASAN_OPTIONS=asan_alloc_limit_mb=256,allocator_may_return_null=1 ./pdfium_test ./test.pdf

==20607==WARNING: Hit total allocation size limit of 256MB while trying to allocate 2450560 bytes
=================================================================
==20607==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f741cdc0202 at pc 0x00000073dd58 bp 0x7fff890f75d0 sp 0x7fff890f75c8
WRITE of size 1 at 0x7f741cdc0202 thread T0
    #0 0x73dd57 in AppendChar /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/../../../include/fpdfapi/../fxcrt/fx_basic.h:63
    #1 0x724817 in GetObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2115
    #2 0x725efa in GetObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2191
    #3 0x7272d2 in ParseIndirectObjectAt /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1399
    #4 0x7283c5 in ParseIndirectObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1202
    #5 0x6ea71a in GetIndirectObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1218
    #6 0x6f8375 in GetDirect /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:231
    #7 0x6cb036 in Start /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:958
    #8 0x642070 in StartParse /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page.cpp:900
    #9 0x642232 in ParseContent /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page.cpp:905
    #10 0x4ef64d in FPDF_LoadPage /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/fpdfsdk/src/fpdfview.cpp:310
    #11 0x4e9a6d in RenderPdf /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/samples/pdfium_test.cc:412
    #12 0x4eafc8 in main /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/samples/pdfium_test.cc:512
    #13 0x7f742f1e8ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287

0x7f741cdc0202 is located 0 bytes to the right of 1960450-byte region [0x7f741cbe1800,0x7f741cdc0202)
allocated by thread T0 here:
    #0 0x4c8095 in __interceptor_realloc _asan_rtl_ (discriminator 2)
    #1 0xc0a511 in CFX_BinaryBuf::ExpandBuf(int) /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fxcrt/fx_basic_buffer.cpp:87
    #2 0x73c8f8 in AppendByte /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/../../../include/fpdfapi/../fxcrt/fx_basic.h:61
    #3 0x724817 in GetObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2115
    #4 0x725efa in GetObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2191
    #5 0x7272d2 in ParseIndirectObjectAt /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1399
    #6 0x7283c5 in ParseIndirectObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1202
    #7 0x6ea71a in GetIndirectObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1218
    #8 0x6f8375 in GetDirect /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:231
    #9 0x6cb036 in Start /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:958
    #10 0x642070 in StartParse /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page.cpp:900
    #11 0x642232 in ParseContent /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page.cpp:905
    #12 0x4ef64d in FPDF_LoadPage /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/fpdfsdk/src/fpdfview.cpp:310
    #13 0x4e9a6d in RenderPdf /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/samples/pdfium_test.cc:412
    #14 0x4eafc8 in main /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/samples/pdfium_test.cc:512
    #15 0x7f742f1e8ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287


By setting allocator_may_return_null=0 we can also confirm that the reallocation is failing:

ASAN_OPTIONS=asan_alloc_limit_mb=256,allocator_may_return_null=0 ./pdfium_test ./test.pdf

==2203==WARNING: Hit total allocation size limit of 256MB while trying to allocate 2450560 bytes
==2203==AddressSanitizer's allocator is terminating the process instead of returning 0
==2203==If you don't like this behavior set allocator_may_return_null=1
==2203==AddressSanitizer CHECK failed: /home/nils/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:146 "((0)) != (0)" (0x0, 0x0)
    #0 0x4cf944 in AsanCheckFailed _asan_rtl_
    #1 0x4d6571 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /home/nils/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_common.cc:125
    #2 0x4d4f53 in __sanitizer::ReportAllocatorCannotReturnNull() /home/nils/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:146 (discriminator 2)
    #3 0x44224e in ReturnNullOrDie /home/nils/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_allocator.h:1310
    #4 0x442cc2 in Reallocate _asan_rtl_
    #5 0x4c815e in __interceptor_realloc _asan_rtl_
    #6 0xc0a511 in CFX_BinaryBuf::ExpandBuf(int) /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fxcrt/fx_basic_buffer.cpp:87
    #7 0x73c8f8 in AppendByte /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/../../../include/fpdfapi/../fxcrt/fx_basic.h:61
    #8 0x724817 in GetObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2115
    #9 0x725efa in GetObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2191
    #10 0x7272d2 in ParseIndirectObjectAt /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1399
    #11 0x7283c5 in ParseIndirectObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1202
    #12 0x6ea71a in GetIndirectObject /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1218
    #13 0x6f8375 in GetDirect /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:231
    #14 0x6cb036 in Start /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:958
    #15 0x642070 in StartParse /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page.cpp:900
    #16 0x642232 in ParseContent /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/core/src/fpdfapi/fpdf_page/fpdf_page.cpp:905
    #17 0x4ef64d in FPDF_LoadPage /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/fpdfsdk/src/fpdfview.cpp:310
    #18 0x4e9a6d in RenderPdf /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/samples/pdfium_test.cc:412
    #19 0x4eafc8 in main /home/nils/build/targets/chrome/src/out/Release/../../third_party/pdfium/samples/pdfium_test.cc:512
    #20 0x7ff7fc2e7ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #21 0x440a3e in _start ??:?
 
asan_alloc_limit.patch
3.2 KB Download
test.pdf.gz
35.2 KB Download
Comment 1 by wfh@chromium.org, Jan 4 2015
Cc: cevans@chromium.org tsepez@chromium.org
Labels: Cr-Internals-Plugins-PDF
Thanks for this.  Another issue where malloc() returning NULL instead of terminating the process is potentially bad.  We need to make sure that all dependent libaries have the allocator shim in place that should enforce this behavior - see  issue 434397 .

Are you finding that without allocator_may_return_null=1 set, the allocator can return NULL in release/production builds of Chrome?
Comment 2 by wfh@chromium.org, Jan 4 2015
Cc: wfh@chromium.org
wfh - allocator_may_return_null=1 is an ASAN only option so it won't have an effect on prod builds. Unless chrome does anything special the default behaviour for malloc/realloc is to return null on failure (in accordance to the man page). Is there a shim in place for pdfium?
Comment 4 by wfh@chromium.org, Jan 4 2015
There is a shim but I think it's only in place for the main chrome dlls and not pdfium or ffmpeg. It's on my list for next week to investigate this more and see if we can put the same shim in place for ffmpeg and pdfium.

We can also set the heap options to raise exception on allocation failure.  See the cl in the bug I linked if you have any comments.
Project Member Comment 5 by clusterf...@chromium.org, Jan 4 2015
ClusterFuzz is analyzing your testcase. Chromium developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=6611165059743744
Comment 6 by f...@chromium.org, Jan 4 2015
Cc: infe...@chromium.org
inferno@, I don't know how to run clusterfuzz with this patch added to asan. can you take a look?
Cc: -infe...@chromium.org jun_f...@foxitsoftware.com
Labels: Security_Severity-High Security_Impact-Stable OS-All Pri-1
Owner: bo...@foxitsoftware.com
Status: Assigned
Project Member Comment 8 by clusterf...@chromium.org, Jan 7 2015
Labels: M-39
Labels: -M-39 M-40
No more M39 patches, moving to M40.
Owner: jun_f...@foxitsoftware.com
Project Member Comment 11 by clusterf...@chromium.org, Jan 27 2015
Labels: Nag
jun_fang@: Uh oh! This issue is still open and hasn't been updated in the last 14 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Cc: -jun_f...@foxitsoftware.com kai_j...@foxitsoftware.com
Status: Started
Project Member Comment 13 by clusterf...@chromium.org, Feb 11 2015
jun_fang@: Uh oh! This issue is still open and hasn't been updated in the last 14 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Comment 14 by wfh@chromium.org, Feb 11 2015
re: #3 pdfium is now contained inside chrome_child.dll so should gain the same memory protections as the rest of chrome - i.e. 2gb limit and also raising an exception on OOM.  I think this means we can close this.
thanks for the update. could you please go ahead to close it or Foxit needs to assign it back to you?  -- Kai


Comment 16 by wfh@chromium.org, Feb 11 2015
Even though pdf will now exception on OOM - the underlying bug is that CFX_BinaryBuf::ExpandBuf does not return a value so there is no way for the caller to know whether it succeeded or not.  I think this should be fixed, and return code handling happen appropriately.
got it. we will take care of it.
xref  issue 401995 
Project Member Comment 19 by clusterf...@chromium.org, Feb 20 2015
Labels: -M-40 M-41
Project Member Comment 20 by clusterf...@chromium.org, Feb 26 2015
jun_fang@: Uh oh! This issue is still open and hasn't been updated in the last 14 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 21 by clusterf...@chromium.org, Mar 10 2015
Labels: Deadline-Exceeded
You have far exceeded the 60-day deadline for fixing this high severity security vulnerability.

We commit ourselves to this deadline and appreciate your utmost priority on this issue.

If you are unable to look into this soon, please find someone else to own this.

- Your friendly ClusterFuzz
Blockedon: chromium:401995
Cc: timwillis@chromium.org
Marking as blocking unless proven otherwise.
My quickly looking at this issue, I think the problem is fairly clear and can be fixed as suggested in the original comment.
The  issue 401995  can possibly be unrelated and in this case it still requires separate investigation.

That being said, I'd vote for reversing the blocks/blockedon dependency or removing it.
Blockedon: -chromium:401995
Owner: kai_j...@foxitsoftware.com
@timurrr: ack, unblocked.

@kai_jing: Can you please take care of this?
@kai_jing - ping on this one as well please! :)
ok, I will take care of the issue now. Thanks for reminder.
Owner: jun_f...@foxitsoftware.com
Project Member Comment 28 by clusterf...@chromium.org, Apr 3 2015
Labels: -M-41 M-42
Cc: steven...@foxitsoftware.com
cloudfuzzer, I have prepared a patch to fix this issue. However, I haven't reproduced the issue that you raised. Seem that the ASAN patch you provided doesn't work in the last version of ASAN (some files are changed). Can you give me the updated patch to reproduce this issue? Also, please tell me how to apply customized ASAN so that it can replace the default one. Thanks!  
Newer versions of clang have adopted a similar implementation to my patch. The binary builds use this version already. You can easily reproduce this issue using the following command line:

ASAN_OPTIONS=soft_rss_limit_mb=256,allocator_may_return_null=1 ./asan-symbolized-linux-release-324711/pdfium_test test.pdf

This page has information on how to build chromium with a custom clang version: https://www.chromium.org/developers/testing/addresssanitizer
jun_fang: I've been able to reproduce this locally, and this doesn't seem to be fixed. Could you try with the ASAN_OPTIONS from c#31?

Hi mbarbella, I reproduced this issue after I built pdfium_test using the custom Clang. Using the default clang in chrome, it can't produce this issue. Thanks for the information. 
jun_fang@ - based on #33, are you suggesting that a fix isn't necessary?
Project Member Comment 36 by clusterf...@chromium.org, May 15 2015
Labels: -M-42 M-43
 Issue 465435  has been merged into this issue.
 Issue 465740  has been merged into this issue.
 Issue 459654  has been merged into this issue.
Project Member Comment 41 by clusterf...@chromium.org, May 16 2015
Labels: -Restrict-View-SecurityTeam Merge-Triage Restrict-View-SecurityNotify M-44
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Labels: -Nag -M-43 -Merge-Triage Merge-Request-44
Merge-Request to M44 (2403) PDFium branch
Labels: -Merge-Request-44 Merge-Review-44 Hotlist-Merge-Review
[Automated comment] No bugdroid (commit) comments found, couldn't auto-approve, needs manual review.
Labels: -Merge-Review-44 -Hotlist-Merge-Review Merge-Approved-44
Merge approved for M44 (2403) pdfium branch.  Please get it merged before end of business PST Monday.
Cc: penny...@chromium.org
**And let me know the new branch hash after you commit.
Merged to M44 in https://pdfium.googlesource.com/pdfium/+/ce95f50e0ed551f6280f163a05b58031a3d011a9

I imagine we'll just do one DEPS roll once all the M44 pdfium fixes have been merged?
Yup.  Once we sort out the other three, I'll just take the top of the m44 branch.  Thanks stig.
Labels: -Merge-Approved-44 merge-merged-2403
Labels: Release-0-M44 reward-topanel
I think this bug should be marked as no particular severity due to https://code.google.com/p/chromium/issues/detail?id=446032#c14, right? Any OOM will just crash the pdfium process.
Labels: CVE-2015-1271
Labels: -reward-topanel reward-3000 reward-unpaid
Congrats - $3,000 for this report.
Project Member Comment 53 by clusterf...@chromium.org, Aug 22 2015
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Labels: -reward-unpaid reward-inprocess
Labels: -reward-inprocess
Processing via our e-payment system takes ~7 days, but the reward should be on its way to you. Thanks again for your help!
Project Member Comment 56 by sheriffbot@chromium.org, Oct 1 2016
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
Project Member Comment 57 by sheriffbot@chromium.org, Oct 2 2016
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
Labels: allpublic
Sign in to add a comment