New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 847346 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Use-of-uninitialized-value in CFX_DIBitmap::Clear

Project Member Reported by ClusterFuzz, May 29 2018

Issue description

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

Fuzzer: tokenfuzz_pdf_curated
Job Type: linux_msan_pdfium
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  CFX_DIBitmap::Clear
  CPDF_RenderStatus::LoadSMask
  CPDF_RenderStatus::ProcessTransparency
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_pdfium&range=552473:552476

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, May 29 2018

Components: Internals>Plugins>PDF
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, May 29 2018

Labels: Test-Predator-Auto-Owner
Owner: thestig@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://pdfium.googlesource.com/pdfium/+/dd2a629f9ede484e0e570ce09d1e9d8906aa11be (Add CPDF_PatternCS::GetPatternRGB(const PatternValue& value).).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Project Member

Comment 3 by sheriffbot@chromium.org, May 29 2018

Labels: M-68
Project Member

Comment 4 by sheriffbot@chromium.org, May 29 2018

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by sheriffbot@chromium.org, May 29 2018

Labels: Pri-1
Status: Started (was: Assigned)
PDFium has some bool GetRGB() methods with a bad contract, where:
1) The caller never checks the return value before reading the RGB values.
2) The GetRGB() implementations will return false, but set RGB to some default value.

So GetRGB() can fail, but the RGB values are initialized and everything happily continues. My CL changed one GetRGB() method to not do (2), with a NOTREACHED(). Now one caller is hitting that NOTREACHED(). To avoid that, I uploaded: https://pdfium-review.googlesource.com/33210
Project Member

Comment 7 by sheriffbot@chromium.org, May 30 2018

Labels: -Security_Impact-Head Security_Impact-Beta
Project Member

Comment 8 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/1c0de38c90947694b5d75349802a0b737418afe3

commit 1c0de38c90947694b5d75349802a0b737418afe3
Author: Lei Zhang <thestig@chromium.org>
Date: Wed May 30 18:35:31 2018

Exclude certain colorspace types for calculating transparency backdrop color.

Per discussion for the "CS" entry in the PDF 1.7 spec table 7.13, several
types of colorspaces do not meet the requirements of this particular
colorspace entry. In terms of implementation, this avoids hitting a
NOTREACHED() in CPDF_PatternCS::GetRGB().

BUG= chromium:847346 

Change-Id: If994a91cdcd84b8977196256ee6926e20c4b74aa
Reviewed-on: https://pdfium-review.googlesource.com/33210
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

[modify] https://crrev.com/1c0de38c90947694b5d75349802a0b737418afe3/core/fpdfapi/render/cpdf_renderstatus.cpp
[modify] https://crrev.com/1c0de38c90947694b5d75349802a0b737418afe3/constants/transparency.h

Status: Fixed (was: Started)
Project Member

Comment 10 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db0edeaad8667dbf579755f9f67b5e442b0cf2d4

commit db0edeaad8667dbf579755f9f67b5e442b0cf2d4
Author: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Wed May 30 23:51:44 2018

Roll src/third_party/pdfium 0789714..a7b65b8 (7 commits)

https://pdfium.googlesource.com/pdfium.git/+log/0789714..a7b65b8


git log 0789714..a7b65b8 --date=short --no-merges --format='%ad %ae %s'
2018-05-30 rharrison@chromium.org Migrate coverage_report.py to use upstream Chromium scripts
2018-05-30 npm@chromium.org Roll third_party/freetype/src/ 9e345c911..d45d4b97e (27 commits)
2018-05-30 thestig@chromium.org Remove dead code in various write function.
2018-05-30 thestig@chromium.org Add pixel tests for PDFs that use generation numbers.
2018-05-30 thestig@chromium.org Exclude certain colorspace types for calculating transparency backdrop color.
2018-05-30 thomasanderson@chromium.org Remove manual references to exe_and_shlib_deps
2018-05-30 tsepez@chromium.org Make common page base class for XFA and non-XFA.

Created with:
  gclient setdep -r src/third_party/pdfium@a7b65b8

The AutoRoll server is located here: https://pdfium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:847346 , chromium:845700 

TBR=dsinclair@chromium.org

Change-Id: I040d66e3a3a9344b7d3858c743a524f40ccd5a76
Reviewed-on: https://chromium-review.googlesource.com/1079847
Reviewed-by: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#563070}
[modify] https://crrev.com/db0edeaad8667dbf579755f9f67b5e442b0cf2d4/DEPS

Project Member

Comment 11 by ClusterFuzz, May 31 2018

ClusterFuzz has detected this issue as fixed in range 563069:563073.

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

Fuzzer: tokenfuzz_pdf_curated
Job Type: linux_msan_pdfium
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  CFX_DIBitmap::Clear
  CPDF_RenderStatus::LoadSMask
  CPDF_RenderStatus::ProcessTransparency
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_pdfium&range=552473:552476
Fixed: https://clusterfuzz.com/revisions?job=linux_msan_pdfium&range=563069:563073

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

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 12 by ClusterFuzz, May 31 2018

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

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 13 by sheriffbot@chromium.org, May 31 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Approved-68
Labels: -Merge-Approved-68 Merge-Request-68
Project Member

Comment 16 by sheriffbot@chromium.org, Jun 5 2018

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-68 Merge-Approved-68
Approved for 68. Branch:3440
Project Member

Comment 18 by sheriffbot@chromium.org, Jun 11 2018

Cc: awhalley@chromium.org abdulsyed@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: hnakashima@chromium.org
I noticed this PDF [1] renders ever slightly differently. Reading more of the PDF spec in section 7.2.3, it looks like some ICCBased colorspaces are allowed. I need to figure out that part, so we don't fix this bug but cause a rendering regression. I'm traveling this week, so I won't get to this until next week. I'll definitely get to this before the M-68 Stable release.

[1] http://maestro2025.edu.co/uploads/user/files/Instructivo_ActualizacionDatos.pdf
Project Member

Comment 20 by sheriffbot@chromium.org, Jun 15 2018

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: abdulsyed@chromium.org
Pls merge you change to M68 branch 3440 ASAP so we can pick it up for this week Beta release. Merge has to happen latest by 1:00 PM PT tomorrow, Tuesday (06/19), so we can pick it up for Wednesday Beta release.




Cc: thestig@chromium.org
Owner: dsinclair@chromium.org
According to #19, merging this may cause a rendering regression. Dan, I'd rather wait for him to come back, but since it's a security issue we may want to just merge the fix. WDYT?
Owner: thestig@chromium.org
My preference is to leave this for thestig@ as I don't know how many PDFs will have rendering issues. As mentioned in #19 he'll take a look next week when back.
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/2895880fa8354b273b4e2b72e61a5b78d1985fa8

commit 2895880fa8354b273b4e2b72e61a5b78d1985fa8
Author: Lei Zhang <thestig@chromium.org>
Date: Fri Jun 29 18:39:50 2018

Better determine if ICC colorspaces can be used for blending.

Implement CPDF_ColorSpace::IsNormal() and check it when rendering. While
IsNormal() is trivial for most colorspaces, it needs to be implemented
separately for ICC colorspaces.

This fixes a rendering regression from commit 1c0de38c.

BUG= chromium:847346 

Change-Id: Iaafed3f8ee40b26ac2cbfbdf2251407f7935311b
Reviewed-on: https://pdfium-review.googlesource.com/36571
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

[modify] https://crrev.com/2895880fa8354b273b4e2b72e61a5b78d1985fa8/core/fpdfapi/page/cpdf_colorspace.cpp
[modify] https://crrev.com/2895880fa8354b273b4e2b72e61a5b78d1985fa8/core/fxcodec/codec/ccodec_iccmodule.h
[modify] https://crrev.com/2895880fa8354b273b4e2b72e61a5b78d1985fa8/core/fpdfapi/page/cpdf_colorspace.h
[modify] https://crrev.com/2895880fa8354b273b4e2b72e61a5b78d1985fa8/core/fpdfapi/render/cpdf_renderstatus.cpp
[modify] https://crrev.com/2895880fa8354b273b4e2b72e61a5b78d1985fa8/core/fxcodec/codec/fx_codec_icc.cpp

Let's let the latest CL bake over the weekend. We can merge next week.
Project Member

Comment 26 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/032a11bedb3aab908e0b4ab9ed742b4d3af5152c

commit 032a11bedb3aab908e0b4ab9ed742b4d3af5152c
Author: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jun 29 21:42:39 2018

Roll src/third_party/pdfium ae82b696f236..2895880fa835 (1 commits)

https://pdfium.googlesource.com/pdfium.git/+log/ae82b696f236..2895880fa835


git log ae82b696f236..2895880fa835 --date=short --no-merges --format='%ad %ae %s'
2018-06-29 thestig@chromium.org Better determine if ICC colorspaces can be used for blending.


Created with:
  gclient setdep -r src/third_party/pdfium@2895880fa835

The AutoRoll server is located here: https://pdfium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:847346 
TBR=dsinclair@chromium.org

Change-Id: I449f49dbc325fb0f7ed9fed5dd977bbf4c96b3dc
Reviewed-on: https://chromium-review.googlesource.com/1120879
Reviewed-by: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#571647}
[modify] https://crrev.com/032a11bedb3aab908e0b4ab9ed742b4d3af5152c/DEPS

Has this been merged yet to M68?
No, I will do so now.
Project Member

Comment 29 by bugdroid1@chromium.org, Jul 3

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/6ef10bb80315d50c99adc0dd0212437ea4916873

commit 6ef10bb80315d50c99adc0dd0212437ea4916873
Author: Lei Zhang <thestig@chromium.org>
Date: Tue Jul 03 20:40:06 2018

M68: Exclude certain colorspace types for calculating transparency backdrop color.

Per discussion for the "CS" entry in the PDF 1.7 spec table 7.13, several
types of colorspaces do not meet the requirements of this particular
colorspace entry. In terms of implementation, this avoids hitting a
NOTREACHED() in CPDF_PatternCS::GetRGB().

BUG= chromium:847346 
TBR=hnakashima@chromium.org

Change-Id: If994a91cdcd84b8977196256ee6926e20c4b74aa
Reviewed-on: https://pdfium-review.googlesource.com/33210
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
(cherry picked from commit 1c0de38c90947694b5d75349802a0b737418afe3)
Reviewed-on: https://pdfium-review.googlesource.com/37011
Reviewed-by: Lei Zhang <thestig@chromium.org>

[modify] https://crrev.com/6ef10bb80315d50c99adc0dd0212437ea4916873/core/fpdfapi/render/cpdf_renderstatus.cpp
[modify] https://crrev.com/6ef10bb80315d50c99adc0dd0212437ea4916873/constants/transparency.h

Project Member

Comment 30 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/98a004ce39b944ca3341c8bdd06d06432b39cdb3

commit 98a004ce39b944ca3341c8bdd06d06432b39cdb3
Author: Lei Zhang <thestig@chromium.org>
Date: Tue Jul 03 21:10:46 2018

M68: Better determine if ICC colorspaces can be used for blending.

Implement CPDF_ColorSpace::IsNormal() and check it when rendering. While
IsNormal() is trivial for most colorspaces, it needs to be implemented
separately for ICC colorspaces.

This fixes a rendering regression from commit 1c0de38c.

BUG= chromium:847346 
TBR=hnakashima@chromium.org

Change-Id: Iaafed3f8ee40b26ac2cbfbdf2251407f7935311b
Reviewed-on: https://pdfium-review.googlesource.com/36571
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
(cherry picked from commit 2895880fa8354b273b4e2b72e61a5b78d1985fa8)
Reviewed-on: https://pdfium-review.googlesource.com/37013
Reviewed-by: Lei Zhang <thestig@chromium.org>

[modify] https://crrev.com/98a004ce39b944ca3341c8bdd06d06432b39cdb3/core/fpdfapi/page/cpdf_colorspace.cpp
[modify] https://crrev.com/98a004ce39b944ca3341c8bdd06d06432b39cdb3/core/fxcodec/codec/ccodec_iccmodule.h
[modify] https://crrev.com/98a004ce39b944ca3341c8bdd06d06432b39cdb3/core/fpdfapi/page/cpdf_colorspace.h
[modify] https://crrev.com/98a004ce39b944ca3341c8bdd06d06432b39cdb3/core/fpdfapi/render/cpdf_renderstatus.cpp
[modify] https://crrev.com/98a004ce39b944ca3341c8bdd06d06432b39cdb3/core/fxcodec/codec/fx_codec_icc.cpp

Labels: -Hotlist-Merge-Review
Labels: -ReleaseBlock-Stable
Project Member

Comment 33 by sheriffbot@chromium.org, Sep 6

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