Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 653090 Security: Heap-use-after-free in Field::UpdateFormField
Starred by 1 user Reported by chamal.d...@gmail.com, Oct 5 2016 Back to list
Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
VULNERABILITY DETAILS

This is a variation of  bug 632709 . This bug happens because of below mentioned code in Field::UpdateFormField method.

std::vector<CPDFSDK_Widget*> widgets;
    pInterForm->GetWidgets(pFormField, &widgets);
    int nFieldType = pFormField->GetFieldType();
    if (nFieldType == FIELDTYPE_COMBOBOX || nFieldType == FIELDTYPE_TEXTFIELD) {
      for (CPDFSDK_Annot* pAnnot : widgets) {
        FX_BOOL bFormatted = FALSE;
        CPDFSDK_Annot::ObservedPtr pObserved(pAnnot);
        CFX_WideString sValue =
            static_cast<CPDFSDK_Widget*>(pObserved.Get())->OnFormat(bFormatted);
        if (pObserved) {
          static_cast<CPDFSDK_Widget*>(pObserved.Get())
              ->ResetAppearance(bFormatted ? &sValue : nullptr, FALSE);
        }
      }
.......

Fix for  bug 632709  introduced ObservedPtr to prevent a deleted single object being used again. But it is not sufficient when there are multiple CPDFSDK_Annot objects with same name. All CPDFSDK_Annot objects can be deleted through call to 
static_cast<CPDFSDK_Widget*>(pObserved.Get())->OnFormat(bFormatted);.
Then next iteration of for loop will use deleted CPDFSDK_Annot object.

VERSION
Chrome Version: [53.0.2785.143] + [stable]
                [55.0.2882.0] + [TOT]
 

Operating System: [Ubuntu 16.04, Windows 10]

REPRODUCTION CASE
1. Open testcase.pdf with chrome.
2. Wait 6 seconds
3. PDF plugin process will crash.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: [PDF plugin process]
Crash State: Address Sanitizer Output

==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x607000018210 at pc 0x561332df3073 bp 0x7fff0decc190 sp 0x7fff0decc188
READ of size 8 at 0x607000018210 thread T0 (chrome)
    #0 0x561332df3072 in __root ./out/asan/../../buildtools/third_party/libc++/trunk/include/__tree:877:65
    #1 0x561332df3072 in __find_equal<CFX_Observable<CPDFSDK_Annot>::ObservedPtr *> ./out/asan/../../buildtools/third_party/libc++/trunk/include/__tree:1597:0
    #2 0x561332df3072 in __insert_unique ./out/asan/../../buildtools/third_party/libc++/trunk/include/__tree:1856:0
    #3 0x561333285fab in insert ./out/asan/../../buildtools/third_party/libc++/trunk/include/set:602:25
    #4 0x561333285fab in AddObservedPtr ./out/asan/../../third_party/pdfium/core/fxcrt/cfx_observable.h:58:0
    #5 0x561333285fab in ObservedPtr ./out/asan/../../third_party/pdfium/core/fxcrt/cfx_observable.h:21:0
    #6 0x561333285fab in UpdateFormField ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:276:0
    #7 0x561333287846 in SetBorderStyle ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:520:9
    #8 0x5613332870ea in borderStyle ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:443:7
    #9 0x56133329f7d2 in JSPropSetter<Field, &Field::borderStyle> ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/JS_Define.h:114:8
    #10 0x5613262a677f in Call ./out/asan/../../v8/src/api-arguments-inl.h:131:3
    #11 0x56132643b4cb in SetPropertyWithAccessor ./out/asan/../../v8/src/objects.cc:1435:10
    #12 0x561326471c58 in SetPropertyInternal ./out/asan/../../v8/src/objects.cc:4679:16
    #13 0x561326470d70 in SetProperty ./out/asan/../../v8/src/objects.cc:4711:9
    #14 0x56132627a3f9 in Store ./out/asan/../../v8/src/ic/ic.cc:1573:3
    #15 0x56132628f583 in __RT_impl_Runtime_StoreIC_Miss ./out/asan/../../v8/src/ic/ic.cc:2423:5
    #16 0x56132628f583 in Runtime_StoreIC_Miss ./out/asan/../../v8/src/ic/ic.cc:2408:0
    #11 0x7fa6d5f043a6  (<unknown module>)
    #12 0x7fa6d6005233  (<unknown module>)
    #13 0x7fa6d6005053  (<unknown module>)
    #14 0x7fa6d5f52be2  (<unknown module>)
    #15 0x7fa6d5f2c320  (<unknown module>)
    #17 0x56132605c40a in Invoke ./out/asan/../../v8/src/execution.cc:139:13
    #18 0x56132605bbb2 in Call ./out/asan/../../v8/src/execution.cc:176:10
    #19 0x56132560cc6b in Run ./out/asan/../../v8/src/api.cc:1865:7
    #20 0x56133330141a in Execute ./out/asan/../../third_party/pdfium/fxjs/fxjs_v8.cpp:477:25
    #21 0x5613332302bc in ?? ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/cjs_runtime.cpp:241:14
    #22 0x5613332fbeea in RunScript ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/cjs_context.cpp:52:24
    #23 0x5613332e8516 in RunJsScript ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/app.cpp:615:15
    #24 0x5613332e8516 in TimerProc ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/app.cpp:603:0
.............

0x607000018210 is located 16 bytes inside of 80-byte region [0x607000018200,0x607000018250)
freed by thread T0 (chrome) here:
    #0 0x561324be335b in operator delete(void*) ??:?
    #1 0x561332dfcc4f in ~CPDFSDK_PageView ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_pageview.cpp:71:23
    #2 0x561332df1d2e in RemovePageView ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_document.cpp:157:3
    #3 0x561328dc4bf2 in Unload ./out/asan/../../pdf/pdfium/pdfium_page.cc:112:7
    #4 0x561328d98cba in CalculateVisiblePages ./out/asan/../../pdf/pdfium/pdfium_engine.cc:2650:20
    #5 0x561328db2a1d in GetMostVisiblePage ./out/asan/../../pdf/pdfium/pdfium_engine.cc:2290:3
    #6 0x561328d95594 in Form_GetCurrentPage ./out/asan/../../pdf/pdfium/pdfium_engine.cc:3502:21
    #7 0x561332df126f in ?? ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_document.cpp:69:38
    #8 0x561333259bec in pageNum ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Document.cpp:220:52
    #9 0x56133326d8ee in JSPropGetter<Document, &Document::pageNum> ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/JS_Define.h:89:8
    #10 0x5613262a7467 in Call ./out/asan/../../v8/src/api-arguments-inl.h:32:1
    #11 0x561326433e95 in GetPropertyWithAccessor ./out/asan/../../v8/src/objects.cc:1352:34
    #12 0x5613264318fa in GetProperty ./out/asan/../../v8/src/objects.cc:998:16
    #13 0x5613268c0e6c in GetObjectProperty ./out/asan/../../v8/src/runtime/runtime-object.cc:34:32
    #14 0x5613268ca1ea in __RT_impl_Runtime_GetProperty ./out/asan/../../v8/src/runtime/runtime-object.cc:345:3
    #15 0x5613268ca1ea in Runtime_GetProperty ./out/asan/../../v8/src/runtime/runtime-object.cc:338:0
    #15 0x7fa6d5f043a6  (<unknown module>)
    #16 0x7fa6d600470e  (<unknown module>)
    #17 0x7fa6d5f52be2  (<unknown module>)
    #18 0x7fa6d5f2c320  (<unknown module>)
    #16 0x56132605c40a in Invoke ./out/asan/../../v8/src/execution.cc:139:13
    #17 0x56132605bbb2 in Call ./out/asan/../../v8/src/execution.cc:176:10
    #18 0x56132560cc6b in Run ./out/asan/../../v8/src/api.cc:1865:7
    #19 0x56133330141a in Execute ./out/asan/../../third_party/pdfium/fxjs/fxjs_v8.cpp:477:25
    #20 0x5613332302bc in ?? ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/cjs_runtime.cpp:241:14
    #21 0x5613332fbeea in RunScript ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/cjs_context.cpp:52:24
    #22 0x561332df74c9 in OnFormat ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_interform.cpp:314:34
    #23 0x561332e132e9 in OnFormat ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_widget.cpp:802:24
    #24 0x561333285fc9 in UpdateFormField ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:278:60
    #25 0x561333287846 in SetBorderStyle ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:520:9
    #26 0x5613332870ea in borderStyle ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:443:7

previously allocated by thread T0 (chrome) here:
    #0 0x561324be271b in operator new(unsigned long) ??:?
    #1 0x561332e2881a in NewAnnot ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_widgethandler.cpp:64:29
    #2 0x561332dff0a9 in LoadFXAnnots ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_pageview.cpp:506:47
    #3 0x561332df0cbf in GetPageView ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_document.cpp:63:14
    #4 0x561332de3d43 in FormHandleToPageView ./out/asan/../../third_party/pdfium/fpdfsdk/fpdfformfill.cpp:57:29
    #5 0x561332de3d43 in FORM_OnAfterLoadPage ./out/asan/../../third_party/pdfium/fpdfsdk/fpdfformfill.cpp:650:0
    #6 0x561328dc4e42 in GetPage ./out/asan/../../pdf/pdfium/pdfium_page.cc:127:7
    #7 0x561332df12fb in GetPageView ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_document.cpp:75:38
    #8 0x561332df59ff in GetWidget ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_interform.cpp:113:28
    #9 0x561332df5f12 in GetWidgets ./out/asan/../../third_party/pdfium/fpdfsdk/cpdfsdk_interform.cpp:138:31
    #10 0x5613332861ad in UpdateFormField ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:296:17
    #11 0x56133329a7b9 in SetValue ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:2778:11
    #12 0x561333299bb1 in value ./out/asan/../../third_party/pdfium/fpdfsdk/javascript/Field.cpp:2674:7
......................
 
testcase.pdf
3.4 KB Download
* Fix for  bug 632709  is not yet merged to stable chrome version 53.0.2785.143. So this test case crash stable version due to  bug 632709 . That crash happens before next iteration of for loop in Field::UpdateFormField which is the reason mentioned in this bug.
Project Member Comment 2 by clusterf...@chromium.org, Oct 5 2016
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=5551157751840768
Labels: Security_Severity-Medium M-54 OS-All Pri-2
Owner: dsinclair@chromium.org
Status: Assigned
Components: Internals>Plugins>PDF
Labels: Security_Impact-Stable
Project Member Comment 5 by sheriffbot@chromium.org, Oct 6 2016
Labels: -Pri-2 Pri-1
Status: Started
The relevant bit of javascript:

  function movePage() {
    this.pageNum = 2;
  }

  function test() {
    this.getField('cmb1').value = 'two';
    this.getField('cmb1').borderStyle = 'dashed';
  }

  app.setTimeOut('movePage()',3000);
  app.setTimeOut('test()',6000);



When we execute the OnFormat call we'll end up executing javascript attached to the node. That javascript does n = this.pageNum. This will force a call out to the embedder to calculate the current page number.

In Chromium, this call will |CalculateVisiblePages|. Part of calculate visible pages is to cleanup pages which aren't currently onscreen. In this case, the page we're working with has been moved off screen, so Chromium cleans up the page, but we're currently working with the page, so when we get back to the calling code the page object and all its annotations have been deleted by the page unload.
Cc: thestig@chromium.org raymes@chromium.org
CL up for review: https://codereview.chromium.org/2418533002/
Status: Fixed
Project Member Comment 10 by bugdroid1@chromium.org, Oct 12 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf6a6765d44b09c64b8c75d749efb84742a250e7

commit bf6a6765d44b09c64b8c75d749efb84742a250e7
Author: dsinclair <dsinclair@chromium.org>
Date: Wed Oct 12 17:36:50 2016

[pdf] Defer page unloading in JS callback.

One of the callbacks from PDFium JavaScript into the embedder is to get the
current page number. In Chromium, this will trigger a call to
CalculateMostVisiblePage that method will determine the visible pages and unload
any non-visible pages. But, if the originating JS is on a non-visible page
we'll delete the page and annotations associated with that page. This will
cause issues as we are currently working with those objects when the JavaScript
returns.

This Cl defers the page unloading triggered by getting the most visible page
until the next event is handled by the Chromium embedder.

BUG= chromium:653090 

Review-Url: https://codereview.chromium.org/2418533002
Cr-Commit-Position: refs/heads/master@{#424781}

[modify] https://crrev.com/bf6a6765d44b09c64b8c75d749efb84742a250e7/pdf/pdfium/pdfium_engine.cc

Project Member Comment 11 by sheriffbot@chromium.org, Oct 13 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Project Member Comment 13 by sheriffbot@chromium.org, Oct 21 2016
Labels: Merge-Request-55
Comment 14 by dimu@chromium.org, Oct 21 2016
Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Labels: -reward-topanel reward-unpaid reward-3000
Congratulations, the panel has awarded $3,000 for this report.  Cheers!
Labels: -reward-unpaid reward-inprocess
Will merge.
Project Member Comment 19 by bugdroid1@chromium.org, Oct 24 2016
Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff05b41fda6ab437bd48d7eaaaf538bf7a60479a

commit ff05b41fda6ab437bd48d7eaaaf538bf7a60479a
Author: Lei Zhang <thestig@chromium.org>
Date: Mon Oct 24 20:33:44 2016

M55: [pdf] Defer page unloading in JS callback.

One of the callbacks from PDFium JavaScript into the embedder is to get the
current page number. In Chromium, this will trigger a call to
CalculateMostVisiblePage that method will determine the visible pages and unload
any non-visible pages. But, if the originating JS is on a non-visible page
we'll delete the page and annotations associated with that page. This will
cause issues as we are currently working with those objects when the JavaScript
returns.

This Cl defers the page unloading triggered by getting the most visible page
until the next event is handled by the Chromium embedder.

BUG= chromium:653090 

Review-Url: https://codereview.chromium.org/2418533002
Cr-Commit-Position: refs/heads/master@{#424781}
(cherry picked from commit bf6a6765d44b09c64b8c75d749efb84742a250e7)

Review URL: https://codereview.chromium.org/2446613003 .

Cr-Commit-Position: refs/branch-heads/2883@{#256}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/ff05b41fda6ab437bd48d7eaaaf538bf7a60479a/pdf/pdfium/pdfium_engine.cc

Labels: -M-54 M-55
Project Member Comment 21 by bugdroid1@chromium.org, Oct 27 2016
Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff05b41fda6ab437bd48d7eaaaf538bf7a60479a

commit ff05b41fda6ab437bd48d7eaaaf538bf7a60479a
Author: Lei Zhang <thestig@chromium.org>
Date: Mon Oct 24 20:33:44 2016

M55: [pdf] Defer page unloading in JS callback.

One of the callbacks from PDFium JavaScript into the embedder is to get the
current page number. In Chromium, this will trigger a call to
CalculateMostVisiblePage that method will determine the visible pages and unload
any non-visible pages. But, if the originating JS is on a non-visible page
we'll delete the page and annotations associated with that page. This will
cause issues as we are currently working with those objects when the JavaScript
returns.

This Cl defers the page unloading triggered by getting the most visible page
until the next event is handled by the Chromium embedder.

BUG= chromium:653090 

Review-Url: https://codereview.chromium.org/2418533002
Cr-Commit-Position: refs/heads/master@{#424781}
(cherry picked from commit bf6a6765d44b09c64b8c75d749efb84742a250e7)

Review URL: https://codereview.chromium.org/2446613003 .

Cr-Commit-Position: refs/branch-heads/2883@{#256}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/ff05b41fda6ab437bd48d7eaaaf538bf7a60479a/pdf/pdfium/pdfium_engine.cc

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
Labels: -Hotlist-Merge-Approved
Labels: Release-0-M55
Labels: CVE-2016-5216
Project Member Comment 26 by sheriffbot@chromium.org, Jan 19
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