New issue
Advanced search Search tips

Issue 702041 link

Starred by 2 users

Issue metadata

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

Blocked on:
issue 714751



Sign in to add a comment

Crash in bilinear_interpol

Project Member Reported by ClusterFuzz, Mar 16 2017

Issue description

Comment 1 by kcc@chromium.org, Mar 16 2017

Cc: dsinclair@chromium.org
Components: Internals>Plugins>PDF
Owner: npm@chromium.org
(I've uploaded this test case manually last night. It's derived from the new large PDF corpus I've also uploaded last night)
Project Member

Comment 2 by sheriffbot@chromium.org, Mar 16 2017

Labels: M-58
Project Member

Comment 3 by sheriffbot@chromium.org, Mar 16 2017

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

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

Comment 4 by sheriffbot@chromium.org, Mar 16 2017

Labels: Pri-1
Project Member

Comment 5 by sheriffbot@chromium.org, Mar 16 2017

Status: Assigned (was: Untriaged)

Comment 6 by npm@chromium.org, Mar 16 2017

Status: Started (was: Assigned)
I'm unable to reproduce asan badness. But running under ubsan causes trouble. So maybe the overflows are being handled differently in the bots, causing more trouble. I'll fix the ubsan and see if clusterfuzz is happy afterwards
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 16 2017

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

commit fbd9ea1db2f1bb7fa006e7304a1202afc683c142
Author: Nicolas Pena <npm@chromium.org>
Date: Thu Mar 16 17:25:51 2017

Compare to epsilon instead of 0 in CFX_Matrix::SetReverse

Since we are going to divide i by 0, it is better to compare it to epsilon
and avoid wonkiness from division by something too close to 0.

BUG= chromium:702041 

Change-Id: I8136d6063f8debd41cef37eaab7e4097b3f32f4b
Reviewed-on: https://pdfium-review.googlesource.com/3090
Commit-Queue: Nicolás Peña <npm@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/fbd9ea1db2f1bb7fa006e7304a1202afc683c142/core/fxcrt/fx_basic_coords.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 16 2017

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

commit b3270a7bf6bce74d1c5e79dfac32b6c3373d1474
Author: pdfium-deps-roller <pdfium-deps-roller@chromium.org>
Date: Thu Mar 16 19:06:10 2017

Roll src/third_party/pdfium/ cfdb5fdd1..c9819a972 (2 commits)

https://pdfium.googlesource.com/pdfium.git/+log/cfdb5fdd12d4..c9819a972830

$ git log cfdb5fdd1..c9819a972 --date=short --no-merges --format='%ad %ae %s'
2017-03-16 npm Use EXPECT_FLOAT_EQ in cpdf_devicecs_unittest
2017-03-16 npm Compare to epsilon instead of 0 in CFX_Matrix::SetReverse

Created with:
  roll-dep src/third_party/pdfium
BUG= 702041 

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

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

TBR=dsinclair@chromium.org

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

[modify] https://crrev.com/b3270a7bf6bce74d1c5e79dfac32b6c3373d1474/DEPS

Project Member

Comment 9 by sheriffbot@chromium.org, Mar 17 2017

Labels: -Security_Impact-Head Security_Impact-Beta
Project Member

Comment 10 by sheriffbot@chromium.org, Mar 17 2017

Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Project Member

Comment 11 by ClusterFuzz, Mar 17 2017

ClusterFuzz has detected this issue as fixed in range 457505:457532.

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

Fuzzer: afl_pdfium_fuzzer
Job Type: afl_chrome_asan
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x611ff58010d5
Crash State:
  bilinear_interpol
  CFX_ImageTransformer::Continue
  CFX_ImageRenderer::Continue
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=afl_chrome_asan&range=447007:452906
Fixed: https://clusterfuzz.com/revisions?job=afl_chrome_asan&range=457505:457532

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv94VYzh_WTVndKtPL5S9th-vh88UGde_4-f-d7nN3WTjbueIcUUEElsDQ92mhjXMPY9lDqjMhYCSAHbvkllGZTJ17mcYy3uYSUqlDquX8F-_e-Uo7lAGhg8W-KTPlnCHtj7OgQiCPq5UbOHslm1EL1iIrfaAjLzkIMMWLLiBHrFDXOevT78HJltbCcRzyoTYSjYUJWytKOIGg2hF5KlluWVtMmSi1dT8RVFSakJ7QaCCYXYWc48MyhlpwN0-ucarMKIzsZXFRoW3qTDmFJESLIqyVcfSTBNB_iZympB0yhAXDVDU-qexh9UBM6Vkg0HVjVwfXeEZD94zqMo_AxN9oZJbBA7mA2wPlPCYdNuOAlwj0kP_egE?testcase_id=6362243785818112


See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md 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, Mar 18 2017

ClusterFuzz has detected this issue as fixed in range 457505:457836.

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

Fuzzer: afl_pdfium_fuzzer
Job Type: afl_chrome_asan
Platform Id: linux

Crash Type: UNKNOWN READ
Crash Address: 0x611ff58010d5
Crash State:
  bilinear_interpol
  CFX_ImageTransformer::Continue
  CFX_ImageRenderer::Continue
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=afl_chrome_asan&range=447007:452906
Fixed: https://clusterfuzz.com/revisions?job=afl_chrome_asan&range=457505:457836

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv94VYzh_WTVndKtPL5S9th-vh88UGde_4-f-d7nN3WTjbueIcUUEElsDQ92mhjXMPY9lDqjMhYCSAHbvkllGZTJ17mcYy3uYSUqlDquX8F-_e-Uo7lAGhg8W-KTPlnCHtj7OgQiCPq5UbOHslm1EL1iIrfaAjLzkIMMWLLiBHrFDXOevT78HJltbCcRzyoTYSjYUJWytKOIGg2hF5KlluWVtMmSi1dT8RVFSakJ7QaCCYXYWc48MyhlpwN0-ucarMKIzsZXFRoW3qTDmFJESLIqyVcfSTBNB_iZympB0yhAXDVDU-qexh9UBM6Vkg0HVjVwfXeEZD94zqMo_AxN9oZJbBA7mA2wPlPCYdNuOAlwj0kP_egE?testcase_id=6362243785818112


See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md 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 13 by ClusterFuzz, Mar 18 2017

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

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

Comment 14 by sheriffbot@chromium.org, Mar 18 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 15 by npm@chromium.org, Mar 20 2017

Labels: Merge-Request-58
Project Member

Comment 16 by sheriffbot@chromium.org, Mar 20 2017

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Looks good for merge approval.
Labels: -Merge-Review-58 Merge-Approved-58
Approving merge to M58 branch 3029 based on comment #17. Please merge ASAP. Thank you.
Project Member

Comment 19 by bugdroid1@chromium.org, Mar 27 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/35e70fb57f9e4093e759d0d35b6d9b8e7800a1a3

commit 35e70fb57f9e4093e759d0d35b6d9b8e7800a1a3
Author: Nicolas Pena <npm@chromium.org>
Date: Mon Mar 27 20:21:23 2017

M58: Compare to epsilon instead of 0 in CFX_Matrix::SetReverse

Since we are going to divide i by 0, it is better to compare it to epsilon
and avoid wonkiness from division by something too close to 0.

BUG= chromium:702041 

Change-Id: I8136d6063f8debd41cef37eaab7e4097b3f32f4b
Reviewed-on: https://pdfium-review.googlesource.com/3090
Commit-Queue: Nicolás Peña <npm@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
(cherry picked from commit fbd9ea1db2f1bb7fa006e7304a1202afc683c142)

Change-Id: I9b50a3940599faa91780c7a9209c3c2fb0ea152a
Reviewed-on: https://pdfium-review.googlesource.com/3235
Reviewed-by: Nicolás Peña <npm@chromium.org>

[modify] https://crrev.com/35e70fb57f9e4093e759d0d35b6d9b8e7800a1a3/core/fxcrt/fx_basic_coords.cpp

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

Comment 21 by npm@chromium.org, Apr 21 2017

Cc: -dsinclair@chromium.org npm@chromium.org
Owner: dsinclair@chromium.org
Status: Available (was: Verified)
The fix needs to be reverted so reopening. I'm OOO next week so reassigning for now.

Comment 22 by npm@chromium.org, Apr 21 2017

Status: Assigned (was: Available)
Cc: thestig@chromium.org
Project Member

Comment 24 by bugdroid1@chromium.org, Apr 21 2017

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

commit f9f26b588e0b30fb581097e2a430f5c80f33b692
Author: Nicolas Pena <npm@chromium.org>
Date: Fri Apr 21 20:08:10 2017

Revert "Compare to epsilon instead of 0 in CFX_Matrix::SetReverse"

This reverts commit fbd9ea1db2f1bb7fa006e7304a1202afc683c142.

Reason for revert: This check seems too strict
BUG= chromium:714187 

Original change's description:
> Compare to epsilon instead of 0 in CFX_Matrix::SetReverse
> 
> Since we are going to divide i by 0, it is better to compare it to epsilon
> and avoid wonkiness from division by something too close to 0.
> 
> BUG= chromium:702041 
> 
> Change-Id: I8136d6063f8debd41cef37eaab7e4097b3f32f4b
> Reviewed-on: https://pdfium-review.googlesource.com/3090
> Commit-Queue: Nicolás Peña <npm@chromium.org>
> Reviewed-by: dsinclair <dsinclair@chromium.org>
> 

TBR=tsepez@chromium.org,dsinclair@chromium.org,npm@chromium.org,pdfium-reviews@googlegroups.com
# Not skipping CQ checks because original CL landed > 1 day ago.
BUG= chromium:702041 

Change-Id: Ia7d933fd3d8ce4957788341463866a04679d7f2b
Reviewed-on: https://pdfium-review.googlesource.com/4432
Reviewed-by: Nicolás Peña <npm@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Nicolás Peña <npm@chromium.org>

[modify] https://crrev.com/f9f26b588e0b30fb581097e2a430f5c80f33b692/core/fxcrt/fx_basic_coords.cpp

Project Member

Comment 25 by sheriffbot@chromium.org, Apr 22 2017

Labels: -Security_Impact-Beta Security_Impact-Stable
dsinclair: Uh oh! This issue still open and hasn't been updated in the last 37 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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
Project Member

Comment 26 by sheriffbot@chromium.org, Apr 22 2017

Status: Fixed (was: Assigned)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Assigned (was: Fixed)
I don't know why sheriffbot marked this as fixed, it was re-opened because the change was reverted.
Project Member

Comment 28 by bugdroid1@chromium.org, Apr 24 2017

Labels: merge-merged-3071
The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/9612873a07b631d29a47abf4f8e31eadf85d7798

commit 9612873a07b631d29a47abf4f8e31eadf85d7798
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Mon Apr 24 16:09:05 2017

[Merge M59] Revert "Compare to epsilon instead of 0 in CFX_Matrix::SetReverse"

This reverts commit fbd9ea1db2f1bb7fa006e7304a1202afc683c142.

Reason for revert: This check seems too strict
BUG= chromium:714187 

Original change's description:
> Compare to epsilon instead of 0 in CFX_Matrix::SetReverse
>
> Since we are going to divide i by 0, it is better to compare it to epsilon
> and avoid wonkiness from division by something too close to 0.
>
> BUG= chromium:702041 
>
> Change-Id: I8136d6063f8debd41cef37eaab7e4097b3f32f4b
> Reviewed-on: https://pdfium-review.googlesource.com/3090
> Commit-Queue: Nicolás Peña <npm@chromium.org>
> Reviewed-by: dsinclair <dsinclair@chromium.org>
>

TBR=tsepez@chromium.org,dsinclair@chromium.org,npm@chromium.org,pdfium-reviews@googlegroups.com
BUG= chromium:702041 

Change-Id: Ia7d933fd3d8ce4957788341463866a04679d7f2b
Reviewed-on: https://pdfium-review.googlesource.com/4432
Reviewed-by: Nicolás Peña <npm@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Nicolás Peña <npm@chromium.org>
(cherry picked from commit f9f26b588e0b30fb581097e2a430f5c80f33b692)

Change-Id: Ieb241da85af89e01f2c3c3a73d21bd4fc04fca6c
Reviewed-on: https://pdfium-review.googlesource.com/4452
Reviewed-by: dsinclair <dsinclair@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/9612873a07b631d29a47abf4f8e31eadf85d7798/core/fxcrt/fx_basic_coords.cpp

Project Member

Comment 29 by bugdroid1@chromium.org, Apr 24 2017

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/84213b529908d2b9095ad4c33ecc9fdf5d881df5

commit 84213b529908d2b9095ad4c33ecc9fdf5d881df5
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Mon Apr 24 19:23:05 2017

[Merge M58] Revert "Compare to epsilon instead of 0 in CFX_Matrix::SetReverse"

This reverts commit fbd9ea1db2f1bb7fa006e7304a1202afc683c142.

Reason for revert: This check seems too strict
BUG= chromium:714187 

Original change's description:
> Compare to epsilon instead of 0 in CFX_Matrix::SetReverse
>
> Since we are going to divide i by 0, it is better to compare it to
epsilon
> and avoid wonkiness from division by something too close to 0.
>
> BUG= chromium:702041 
>
> Change-Id: I8136d6063f8debd41cef37eaab7e4097b3f32f4b
> Reviewed-on: https://pdfium-review.googlesource.com/3090
> Commit-Queue: Nicolás Peña <npm@chromium.org>
> Reviewed-by: dsinclair <dsinclair@chromium.org>
>

TBR=tsepez@chromium.org,dsinclair@chromium.org,npm@chromium.org,pdfium-reviews@googlegroups.com
BUG= chromium:702041 

Change-Id: Ia7d933fd3d8ce4957788341463866a04679d7f2b
Reviewed-on: https://pdfium-review.googlesource.com/4432
Reviewed-by: Nicolás Peña <npm@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Nicolás Peña <npm@chromium.org>
Reviewed-on: https://pdfium-review.googlesource.com/4451
Reviewed-by: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/84213b529908d2b9095ad4c33ecc9fdf5d881df5/core/fxcrt/fx_basic_coords.cpp

Blockedon: 714751
Project Member

Comment 31 by bugdroid1@chromium.org, Apr 25 2017

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/233360e3ea0d6d4acea76a7d9e7ce7700ea80b1a

commit 233360e3ea0d6d4acea76a7d9e7ce7700ea80b1a
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Tue Apr 25 19:33:11 2017

Add unittest for matrix reverse

This Cl adds some unit tests for the CFX_Matrix Reverse method.

Bug:  chromium:702041 
Change-Id: I9598d3eb48d6b86a3e192d313fe1091bd29a4701
Reviewed-on: https://pdfium-review.googlesource.com/4492
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/233360e3ea0d6d4acea76a7d9e7ce7700ea80b1a/BUILD.gn
[add] https://crrev.com/233360e3ea0d6d4acea76a7d9e7ce7700ea80b1a/core/fxcrt/fx_coordinates_unittest.cpp

Project Member

Comment 32 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/2e2a4fcd43677c5882dcf00cb4b99635cb2cfcd3

commit 2e2a4fcd43677c5882dcf00cb4b99635cb2cfcd3
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Wed Apr 26 17:06:44 2017

Guard against overflow in CFX_BilinearMatrix code.

If any of the values in the matrix used to initialize the
CFX_BilinearMatrix are close to INT_MAX then the numbers can overflow
when multipled causing undefined behaviour.

This Cl uses a pdfium::CheckedNumeric to handle the multiplications and
then assigns back to the int value if valid.

Bug:  chromium:702041 
Change-Id: Ia1895e2e39c0ac2bf099d45f97e33209cb50d134
Reviewed-on: https://pdfium-review.googlesource.com/4495
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/2e2a4fcd43677c5882dcf00cb4b99635cb2cfcd3/core/fxge/dib/cfx_imagetransformer.cpp

Project Member

Comment 33 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/2e2a4fcd43677c5882dcf00cb4b99635cb2cfcd3

commit 2e2a4fcd43677c5882dcf00cb4b99635cb2cfcd3
Author: Dan Sinclair <dsinclair@chromium.org>
Date: Wed Apr 26 17:06:44 2017

Guard against overflow in CFX_BilinearMatrix code.

If any of the values in the matrix used to initialize the
CFX_BilinearMatrix are close to INT_MAX then the numbers can overflow
when multipled causing undefined behaviour.

This Cl uses a pdfium::CheckedNumeric to handle the multiplications and
then assigns back to the int value if valid.

Bug:  chromium:702041 
Change-Id: Ia1895e2e39c0ac2bf099d45f97e33209cb50d134
Reviewed-on: https://pdfium-review.googlesource.com/4495
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

[modify] https://crrev.com/2e2a4fcd43677c5882dcf00cb4b99635cb2cfcd3/core/fxge/dib/cfx_imagetransformer.cpp

Status: Fixed (was: Assigned)
This is, hopefully, fixed again. I don't think we should merge to M58 given the issue it caused last time. We could consider merging to M59 but I'm not sure if it's worth it, or safer to let it ride through the regular channels.
 Issue 712083  has been merged into this issue.
Labels: -M-58 -merge-merged-3029 -merge-merged-3071 M-60
Labels: Release-0-M60
Project Member

Comment 38 by sheriffbot@chromium.org, Aug 3 2017

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