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.
Issue 555784 Heap-buffer-overflow in CCodec_RLScanlineDecoder::v_GetNextLine
Starred by 0 users Project Member Reported by clusterf...@chromium.org, Nov 13 2015 Back to list
Status: Fixed
Owner:
Closed: Nov 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5517681135976448

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

Crash Type: Heap-buffer-overflow WRITE {*}
Crash Address: 0x609000009c51
Crash State:
  CCodec_RLScanlineDecoder::v_GetNextLine
  CCodec_ScanlineDecoder::GetScanline
  CPDF_DIBSource::GetScanline
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv94Pq5xglZBUr9L9fKu_WPB0Mp1pznzxsHcFH8PUcOnTqmXko14CHSX_Tq7XFEYDnqhaqvHv592m9ZbIA7brzjSTqp47XBip27WJ5Jdy6VaXFZjMsY2MUWvDA9YJOB9_0xnkOgO26Gz-yT_WpbRf15UyfWFHHCXpaOqDeEroTgmgKAT9Kxw


Filer: ochang

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Labels: M-47 M-48
Owner: och...@chromium.org
Comment 2 by och...@chromium.org, Nov 13 2015
Found by auditing -- Out of bounds write.

The issue here is in CCodec_RLScanlineDecoder::Create:

FX_BOOL CCodec_RLScanlineDecoder::Create(const uint8_t* src_buf,
                                         FX_DWORD src_size,
                                         int width,
                                         int height,
                                         int nComps,
                                         int bpc) {
  m_pSrcBuf = src_buf;
  m_SrcSize = src_size;
  m_OutputWidth = m_OrigWidth = width;
  m_OutputHeight = m_OrigHeight = height;
  m_nComps = nComps;
  m_bpc = bpc;
  m_bColorTransformed = FALSE;
  m_DownScale = 1;
  m_Pitch = (width * nComps * bpc + 31) / 32 * 4; <---- overflow
  m_dwLineBytes = (width * nComps * bpc + 7) / 8;
  m_pScanline = FX_Alloc(uint8_t, m_Pitch); 
  return CheckDestSize();
}

While the result of (width * nComps * bpc + 7) / 8 is checked for unsigned 32-bit overflow in CPDF_DIBSource::StartLoadDIBSource before this is called, it doesn't sure that (width * nComps * bpc + 31) / 32 * 4 doesn't overflow. The result of this is stored in |m_Pitch| and used to allocate memory for the scanlines.

In my testcase, I used an image with a width == 81915, height == 1, BitsPerComponent == 16, with a DeviceN colourspace that has 3277 components.

This leads to an allocation of:

((81915 * 1 * 16 * 3277) + 31) / 32 == 0, which leads to an out of bounds write.



Labels: Security_Impact-Stable
Comment 4 by och...@chromium.org, Nov 13 2015
Cc: tsepez@chromium.org
Another point to add here is that these are signed ints being multiplied, while overflow was checked using unsigned numbers.. sigh

This testcase itself doesn't seem to reproduce under Chrome. Tom, are there any mitigations/different code paths on the Chrome side that might be preventing this from triggering here? I'm not entirely convinced this doesn't affect Chrome, so might be safe to keep the Security_Impact-Stable label.


Project Member Comment 5 by clusterf...@chromium.org, Nov 13 2015
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5517681135976448

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

Crash Type: Heap-buffer-overflow WRITE {*}
Crash Address: 0x609000009c51
Crash State:
  CCodec_RLScanlineDecoder::v_GetNextLine
  CCodec_ScanlineDecoder::GetScanline
  CPDF_DIBSource::GetScanline
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_pdfium&range=307685:308523

Minimized Testcase (8211.80 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95u1jr0WmMKYEgtQkOQe6rDYkqfk_VO7lUNYiqFZZSoEFlaiRmBfVYxLKLtLNyA8yedumbKb2clx0TPE9-SiVx23aQbmHjPfTjA6x6JmPykbKpJ0rasGRoGscwv6_MVbqSHdE5FKyg9eEvctTIkz4tCIM1a-opcd23lQQjQkGiV6QjWMUw

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
Project Member Comment 6 by clusterf...@chromium.org, Nov 14 2015
Labels: Pri-1
Status: Assigned
Comment 7 by och...@chromium.org, Nov 20 2015
I just reproed this on a clean release build of Chrome, not sure why it doesn't repro under ClusterFuzz...
Project Member Comment 9 by clusterf...@chromium.org, Nov 20 2015
Labels: -Restrict-View-SecurityTeam Merge-Triage 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 friendly ClusterFuzz
Labels: Merge-Request-47 Merge-Request-48
Comment 12 by tin...@google.com, Nov 24 2015
Labels: -Merge-Request-47 Merge-Review-47 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Comment 13 by tin...@google.com, Nov 24 2015
Labels: -Merge-Request-48 Merge-Review-48
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-47 Merge-Rejected-47
We already have a stable candidate build, so not accepting merges into M47 at this point. Rejecting the merge for now.
Labels: -Merge-Rejected-47 -Hotlist-Merge-Review Merge-Approved-47 Hotlist-Merge-Approved
Cc: timwillis@chromium.org
Labels: -Merge-Triage -Merge-Review-48
@ochang: Did this end up getting rolled with the first M47? I assume not and is approved for a post-stable patch. If so, please do the needful - you have a merge approval in #15.
Shruthi asked for the merge to land on Monday, so will do that then.
Ack - thanks.
Project Member Comment 19 by bugdroid1@chromium.org, Nov 30 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=81212

------------------------------------------------------------------
r81212 | ochang@google.com | 2015-11-30T16:58:28.196521Z

-----------------------------------------------------------------
Labels: Merge-Request-48
Comment 21 by tin...@google.com, Nov 30 2015
Labels: -Merge-Request-48 Merge-Review-48 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-48 Merge-Approved-48
Merge approved for M48 (branch 2564), pls merge asap as we're cutting M48 beta candidate today.
Project Member Comment 23 by bugdroid1@chromium.org, Nov 30 2015
Labels: -Merge-Approved-48 merge-merged-2564
The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=81216

------------------------------------------------------------------
r81216 | ochang@google.com | 2015-11-30T19:47:18.086561Z

-----------------------------------------------------------------
Labels: Release-1-M47
Project Member Comment 25 by clusterf...@chromium.org, Mar 2 2016
Labels: -Restrict-View-SecurityNotify
This security bug has been closed for more than 14 weeks. Removing view restrictions.

- Your friendly Sheriffbot
Project Member Comment 26 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 27 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