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 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-buffer-overflow in CPDF_TextObject::CalcPositionData

Project Member Reported by ClusterFuzz, Nov 10 2015

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6470504964161536

Uploader: mjurczyk@google.com
Job Type: linux_asan_pdfium
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x60d0000029ec
Crash State:
  CPDF_TextObject::CalcPositionData
  CPDF_StreamContentParser::AddTextObject
  CPDF_StreamContentParser::Handle_ShowText
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv958IRUw0B--Ss6MOQl-OSrx37JBFwe-TGIuQqN-t3dlisFq0eGSWWtSPap_cD2zR_EOC50r7My62rOATKypDZfYIIOjRCDTVRNVaVjaq2yKB3vXLKKTDqIL_pRN06hcxQrhqwgZXUwIWZeEWTKnxI5aflDWXg


Filer: mjurczyk

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 

Comment 1 by mjurczyk@google.com, Nov 10 2015

The issue is tracked at https://code.google.com/p/google-security-research/issues/detail?id=623 on the Project Zero side.

This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.
Cc: tsepez@chromium.org och...@chromium.org

Comment 3 by och...@chromium.org, Nov 10 2015

Cc: -och...@chromium.org
Owner: och...@chromium.org
Yay another bug.

Comment 4 by tsepez@chromium.org, Nov 10 2015

could be found by static analysis even, 

  for (int i = 0; i < m_nChars; ++i) {
    FX_DWORD charcode =
        m_nChars == 1 ? (FX_DWORD)(uintptr_t)m_pCharCodes : m_pCharCodes[i];
    if (charcode == (FX_DWORD)-1) {
      curpos -= FXSYS_Mul(m_pCharPos[i - 1], fontsize) / 1000;
      continue;
    }
    if (i) {
      m_pCharPos[i - 1] = curpos;
    }
 

Comment 5 by tsepez@chromium.org, Nov 10 2015

Cc: jun_f...@foxitsoftware.com
Project Member

Comment 6 by ClusterFuzz, Nov 10 2015

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6470504964161536

Uploader: mjurczyk@google.com
Job Type: linux_asan_pdfium
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x60d000002abc
Crash State:
  CPDF_TextObject::CalcPositionData
  CPDF_StreamContentParser::AddTextObject
  CPDF_StreamContentParser::Handle_ShowText
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_pdfium&range=350971:350997

Minimized Testcase (1.77 Kb): https://cluster-fuzz.appspot.com/download/AMIfv9599o3Ckxl_nrHvHWrRnV7SsyBLfWTiP999dKxX6rCVJfINV0Ngj5Ns_IshPWqi3M_EdoaGLrCh4zEH2ZPXk_oK83qQtWZgRvTNZH-8mUyamz54KV28KnuDWP4zmebKBKetFJbmNpgqrhMiN00asnGkgAuJKg

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

Comment 7 by tsepez@chromium.org, Nov 10 2015

Nah, this has been wrong for a long long time, there's some other change that allowed it to be hit that's unrelated.

I suspect the -1 stuff is because we know that the first char is always at 0, so we can save one element by introducing a special case.  Hardly seems worth it. Also, the optimization on the third line above to not allocate an array for runs of a single character seems dubious, too.  And parallel arrays seem dubious, too.  I'd much rather see a single array of { FX_DWORD code, FX_FLOAT position }.  Something to think about if you want to re-write this section of code.

Labels: Security_Impact-Stable M-47
Project Member

Comment 9 by ClusterFuzz, Nov 10 2015

Labels: Pri-1
Status: Assigned
Status: Started
Hmm, it looks like (DWORD)-1 is used as a sentinel value here for indicating the boundaries between each segment (and the kerning values), but judging by this testcase it can be a valid charcode too, so we'll probably also need to rip that out too. 

That said, I'll submit a simple patch for this first that prevents the underflow, and probably rewrite this thing later to fix up everything.
Status: Fixed
Fixed in https://pdfium.googlesource.com/pdfium/+/46d2e278f62454ed2392630b6d18d33d380a20eb. 

Will add a personal TODO to clean this part of the code another day.
Project Member

Comment 12 by ClusterFuzz, Nov 10 2015

Labels: -Restrict-View-SecurityTeam Merge-Triage M-48 M-46 Restrict-View-SecurityNotify
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 fix is very close to the branch point. After the branch happens, please make sure to check if your fix is in.

- Your friendly ClusterFuzz
Labels: Merge-Request-47
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 11 2015

Comment 15 by tin...@google.com, Nov 11 2015

Labels: -Merge-Request-47 Merge-Review-47 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-47 -Hotlist-Merge-Review Merge-Approved-47 Hotlist-Merge-Approved
Approved for M47 (branch 2526)
Project Member

Comment 17 by ClusterFuzz, Nov 11 2015

ClusterFuzz has detected this issue as fixed in range 358928:359050.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6470504964161536

Uploader: mjurczyk@google.com
Job Type: linux_asan_pdfium
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x60d000002abc
Crash State:
  CPDF_TextObject::CalcPositionData
  CPDF_StreamContentParser::AddTextObject
  CPDF_StreamContentParser::Handle_ShowText
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_pdfium&range=350971:350997
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_pdfium&range=358928:359050

Minimized Testcase (1.77 Kb): https://cluster-fuzz.appspot.com/download/AMIfv9599o3Ckxl_nrHvHWrRnV7SsyBLfWTiP999dKxX6rCVJfINV0Ngj5Ns_IshPWqi3M_EdoaGLrCh4zEH2ZPXk_oK83qQtWZgRvTNZH-8mUyamz54KV28KnuDWP4zmebKBKetFJbmNpgqrhMiN00asnGkgAuJKg

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect,try re-doing that job on the test case report page.
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 11 2015

Labels: -Merge-Approved-47 merge-merged-2526
The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=80518

------------------------------------------------------------------
r80518 | ochang@google.com | 2015-11-11T18:27:45.612216Z

-----------------------------------------------------------------
Cc: timwillis@chromium.org
Labels: -Merge-Triage -M-48 -M-46 Release-0-M47
Project Member

Comment 20 by ClusterFuzz, Feb 17 2016

Labels: -Restrict-View-SecurityNotify
This security bug has been closed for more than 14 weeks. Removing view restrictions.

- Your friendly ClusterFuzz
Project Member

Comment 21 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 22 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