New issue
Advanced search Search tips

Issue 804155 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

stack-buffer-overflow in CPDF_IndexedCS::GetRGB

Reported by attek...@gmail.com, Jan 21 2018

Issue description


Tested on:

Chromium: asan-linux-release-530783, reproduces with chrome and pdfium_test.
OS: Ubuntu 16.04

I did some digging in to the crash. I'll do more digging if this is not a dupe. 

In CPDF_DIBSource::LoadPalette() third_party/pdfium/core/fpdfapi/render/cpdf_dibsource.cpp:767:20

    763     float color_values[3];
    764     color_values[0] = m_pCompData[0].m_DecodeMin;
    765     color_values[1] = color_values[2] = color_values[0];
    766     float R = 0.0f, G = 0.0f, B = 0.0f;
    767     m_pColorSpace->GetRGB(color_values, &R, &G, &B);

CPDF_PatternCS::GetRGB(float* pBuf, float* R, float* G, float* B) is called with float array as the first argument. 

That array pointer is then reintrepret_cast to PatternValue*.

struct PatternValue {
  CPDF_Pattern* m_pPattern;
  CPDF_CountedPattern* m_pCountedPattern;
  int m_nComps;
  float m_Comps[kMaxPatternColorComps];
};

In CPDF_IndexedCS::GetRGB(float*, float*, float*, float*) const third_party/pdfium/core/fpdfapi/page/cpdf_colorspace.cpp:1065:40

     56     PatternValue* pvalue = reinterpret_cast<PatternValue*>(pBuf);
     57     if (m_pBaseCS->GetRGB(pvalue->m_Comps, R, G, B))

After the reinterpret_cast m_pBaseCS->GetRGB is called with pvalue->m_Comps, which points outside of color_values. 

ASAN catches the issue in CPDF_IndexedCS::GetRGB.
   
   1064 bool CPDF_IndexedCS::GetRGB(float* pBuf, float* R, float* G, float* B) const {
   1065   int32_t index = static_cast<int32_t>(*pBuf);

Without ASAN the repro-file ends up returning false from:

   1073     if (!length.IsValid() || length.ValueOrDie() > m_Table.GetLength()) {
   1074       *R = 0;
   1075       *G = 0;
   1076       *B = 0;
   1077       return false;
   1078     }

When check length.ValueOrDie() > m_Table.GetLength() passes.

ASAN-trace:

:/asan-linux-release-530783/pdfium_test$ ./stack-buffer-overflow-color_profile.pdf 
Rendering PDF file ./stack-buffer-overflow-color_profile.pdf.
=================================================================
==22493==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7faf96e01834 at pc 0x000002ce4513 bp 0x7fff8c3e29f0 sp 0x7fff8c3e29e8
READ of size 4 at 0x7faf96e01834 thread T0
    #0 0x2ce4512 in (anonymous namespace)::CPDF_IndexedCS::GetRGB(float*, float*, float*, float*) const third_party/pdfium/core/fpdfapi/page/cpdf_colorspace.cpp:1065:40
    #1 0x2d24449 in CPDF_PatternCS::GetRGB(float*, float*, float*, float*) const third_party/pdfium/core/fpdfapi/page/cpdf_patterncs.cpp:57:20
    #2 0x2e17ba6 in CPDF_DIBSource::LoadPalette() third_party/pdfium/core/fpdfapi/render/cpdf_dibsource.cpp:767:20
    #3 0x2e191e6 in CPDF_DIBSource::ContinueToLoadMask() third_party/pdfium/core/fpdfapi/render/cpdf_dibsource.cpp:240:3
    #4 0x2e19dd5 in CPDF_DIBSource::StartLoadDIBSource(CPDF_Document*, CPDF_Stream const*, bool, CPDF_Dictionary*, CPDF_Dictionary*, bool, unsigned int, bool) third_party/pdfium/core/fpdfapi/render/cpdf_dibsource.cpp:298:8
    #5 0x2e3082c in CPDF_ImageCacheEntry::StartGetCachedBitmap(CPDF_Dictionary*, CPDF_Dictionary*, bool, unsigned int, bool, CPDF_RenderStatus*) third_party/pdfium/core/fpdfapi/render/cpdf_imagecacheentry.cpp:71:48
    #6 0x2e2b8dd in CPDF_PageRenderCache::StartGetCachedBitmap(fxcrt::RetainPtr<CPDF_Image> const&, bool, unsigned int, bool, CPDF_RenderStatus*) third_party/pdfium/core/fpdfapi/render/cpdf_pagerendercache.cpp:97:36
    #7 0x2e7080c in CPDF_ImageLoader::Start(CPDF_ImageObject const*, CPDF_PageRenderCache*, bool, unsigned int, bool, CPDF_RenderStatus*) third_party/pdfium/core/fpdfapi/render/cpdf_imageloader.cpp:34:19
    #8 0x2e65d42 in CPDF_ImageRenderer::StartLoadDIBSource() third_party/pdfium/core/fpdfapi/render/cpdf_imagerenderer.cpp:62:16
    #9 0x2e6de5b in CPDF_ImageRenderer::Start(CPDF_RenderStatus*, CPDF_ImageObject*, CFX_Matrix const*, bool, int) third_party/pdfium/core/fpdfapi/render/cpdf_imagerenderer.cpp:186:7
    #10 0x2e3b7ff in CPDF_RenderStatus::ContinueSingleObject(CPDF_PageObject*, CFX_Matrix const*, IFX_PauseIndicator*) third_party/pdfium/core/fpdfapi/render/cpdf_renderstatus.cpp:1153:26
    #11 0x2e3339d in CPDF_ProgressiveRenderer::Continue(IFX_PauseIndicator*) third_party/pdfium/core/fpdfapi/render/cpdf_progressiverenderer.cpp:93:30
    #12 0x2a70496 in (anonymous namespace)::RenderPageImpl(CPDF_PageRenderContext*, CPDF_Page*, CFX_Matrix const&, FX_RECT const&, int, bool, IFSDK_PAUSE_Adapter*) third_party/pdfium/fpdfsdk/fpdfview.cpp:129:26
    #13 0x2a6fd6e in FPDF_RenderPage_Retail(CPDF_PageRenderContext*, void*, int, int, int, int, int, int, bool, IFSDK_PAUSE_Adapter*) third_party/pdfium/fpdfsdk/fpdfview.cpp:1280:3
    #14 0x2a2e2ee in FPDF_RenderPageBitmap_Start third_party/pdfium/fpdfsdk/fpdf_progressive.cpp:60:3
    #15 0xaa040a in RenderPage third_party/pdfium/samples/pdfium_test.cc:1273:16
    #16 0xaa040a in (anonymous namespace)::RenderPdf(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, char const*, unsigned long, (anonymous namespace)::Options const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) third_party/pdfium/samples/pdfium_test.cc:1469
    #17 0xa9a36c in main third_party/pdfium/samples/pdfium_test.cc:1630:5
    #18 0x7faf9a1d582f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291

Address 0x7faf96e01834 is located in stack of thread T0 at offset 52 in frame
    #0 0x2e1773f in CPDF_DIBSource::LoadPalette() third_party/pdfium/core/fpdfapi/render/cpdf_dibsource.cpp:745

  This frame has 9 object(s):
    [32, 44) 'color_values' (line 763) <== Memory access at offset 52 overflows this variable
    [64, 68) 'R' (line 766) <== Memory access at offset 52 underflows this variable
    [80, 84) 'G' (line 766)
    [96, 100) 'B' (line 766)
    [112, 184) 'color_values80' (line 786)
    [224, 228) 'R104' (line 796)
    [240, 244) 'G105' (line 796)
    [256, 260) 'B106' (line 796)
    [272, 296) 'temp_buf' (line 800)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow third_party/pdfium/core/fpdfapi/page/cpdf_colorspace.cpp:1065:40 in (anonymous namespace)::CPDF_IndexedCS::GetRGB(float*, float*, float*, float*) const
Shadow bytes around the buggy address:
  0x0ff672db82b0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0ff672db82c0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0ff672db82d0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0ff672db82e0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x0ff672db82f0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
=>0x0ff672db8300: f1 f1 f1 f1 00 04[f2]f2 04 f2 04 f2 04 f2 f8 f8
  0x0ff672db8310: f8 f8 f8 f8 f8 f8 f8 f2 f2 f2 f2 f2 f8 f2 f8 f2
  0x0ff672db8320: f8 f2 f8 f8 f8 f3 f3 f3 f3 f3 f3 f3 00 00 00 00
  0x0ff672db8330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff672db8340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff672db8350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==22493==ABORTING


 
stack-buffer-overflow-color_profile.pdf
384 bytes Download
Components: Internals>Plugins>PDF
Owner: hnakashima@chromium.org
Labels: Pri-1
Status: Started (was: Unconfirmed)

Comment 4 by mea...@chromium.org, Jan 24 2018

Labels: Security_Severity-High
Project Member

Comment 5 by ClusterFuzz, Jan 25 2018

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5728559868674048.
Project Member

Comment 6 by ClusterFuzz, Jan 25 2018

Labels: Security_Impact-Stable
Detailed report: https://clusterfuzz.com/testcase?key=5728559868674048

Job Type: linux_asan_pdfium
Crash Type: Stack-buffer-overflow READ 4
Crash Address: 0x7f31897df034
Crash State:
  CPDF_IndexedCS::GetRGB
  CPDF_PatternCS::GetRGB
  CPDF_DIBSource::LoadPalette
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_pdfium&range=350971:350997

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5728559868674048

See https://github.com/google/clusterfuzz-tools for more information.

The recommended severity is different from what was assigned to the bug. Please double check the accuracy of the assigned severity.

Project Member

Comment 7 by sheriffbot@chromium.org, Jan 26 2018

Labels: M-64
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 26 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/49363202ce06ca9ff418b4df384cffadf924303c

commit 49363202ce06ca9ff418b4df384cffadf924303c
Author: Henrique Nakashima <hnakashima@chromium.org>
Date: Fri Jan 26 15:42:59 2018

Fix crash in palette loading with Pattern colorspace.

Bug:  chromium:804155 
Change-Id: Ie70e93116696e3c4db987a10b8fc31b1af8aea70
Reviewed-on: https://pdfium-review.googlesource.com/23990
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>

[modify] https://crrev.com/49363202ce06ca9ff418b4df384cffadf924303c/core/fpdfapi/render/cpdf_dibsource.cpp

Reported bug is fixed, will leave this open for now while I work on a larger CL that changes the signature of GetRGB so this doesn't happen again.

Will request a cherry-pick for my fix in a few days after baking in Canary.
Project Member

Comment 10 by ClusterFuzz, Jan 28 2018

ClusterFuzz has detected this issue as fixed in range 532192:532203.

Detailed report: https://clusterfuzz.com/testcase?key=5728559868674048

Job Type: linux_asan_pdfium
Crash Type: Stack-buffer-overflow READ 4
Crash Address: 0x7f31897df034
Crash State:
  CPDF_IndexedCS::GetRGB
  CPDF_PatternCS::GetRGB
  CPDF_DIBSource::LoadPalette
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_pdfium&range=350971:350997
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_pdfium&range=532192:532203

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5728559868674048

See https://github.com/google/clusterfuzz-tools 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 11 by ClusterFuzz, Jan 28 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 5728559868674048 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Labels: reward-topanel
Labels: -reward-topanel reward-0
The VRP panel dug into this bug a bit, but found that with the DCHECK removed this didn't lead to memory corruption or other exploitable condition.
Labels: -Type-Bug-Security -Security_Impact-Stable -Security_Severity-High Type-Bug
Labels: Merge-Request-65
Do we want to merge this into M65?

It's not exploitable, and even though it fixes a crash, the crash only happens with very specific PDFs created to demonstrate the issue.
Please add affected OSs.
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Cc: awhalley@chromium.org
+awhalley@ to comment whether we need this merge to M65 or not. Please see comment #15. If it is not critical, then it is best to wait for M66.
Labels: -M-64 -Merge-Request-65 M-66
Fine to leave to 66.
Project Member

Comment 20 by sheriffbot@chromium.org, Feb 8 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 21 by sheriffbot@chromium.org, May 6 2018

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