Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: potential UAF in pdfium timer
Reported by jinmot...@gmail.com, Jan 10 2017 Back to list
VULNERABILITY DETAILS
There is js timer implementation in src/third_party/pdfium/fpdfsdk/javascript/app.cpp.

When GlobalTimer is removed, it must delete <timer ID>-><GlobalTimer> mapping, but if m_TimerID is 0, it doesn't remove it.

On Line 81:
GlobalTimer::~GlobalTimer() {
  if (!m_nTimerID)
    return;

  if (GetRuntime())
    m_pFormFillEnv->GetSysHandler()->KillTimer(m_nTimerID);

  GetGlobalTimerMap()->erase(m_nTimerID);
}

Additionally, GetGlobalTimerMap is referenced in GlobalTimer::Trigger, which is called as timer callback. However if m_nTimerID is 0, it doesn't kill the timer, so it seems like UAF.

VERSION
Chrome Version: 55.0.2883.87 stable
Operating System: Windows 10

REPRODUCTION CASE
For now there is memory leak on app.setTimeOut / app.clearTimeOut, so can't reproduce in my laptop.
 
Comment 1 by jinmot...@gmail.com, Jan 10 2017
However after 16 hours of waiting, it can be triggered without memory leak. The step is:

1. Have a textbox field
2. focus on that field and other field, again and again. The caret creates timer and kills the timer when focused/unfocused.
3. Loop.

42 sec on 0x400000 tries..
Comment 2 by jinmot...@gmail.com, Jan 10 2017
Possible mitigations would be adjusting timerid as 64bit, or just remove the check.
Comment 3 by tsepez@chromium.org, Jan 11 2017
Thanks for the report.  Having an ID that can't overflow seems desirable, even if the check is removed.
Comment 4 by tsepez@chromium.org, Jan 11 2017
Cc: dsinclair@chromium.org
Components: Internals>Plugins>PDF
Owner: tsepez@chromium.org
Status: Assigned
Comment 5 by tsepez@chromium.org, Jan 11 2017
Labels: Security_Severity-High Security_Impact-Stable
Comment 6 by tsepez@chromium.org, Jan 11 2017
https://cs.chromium.org/chromium/src/third_party/pdfium/public/fpdf_formfill.h?rcl=1484151649&l=508 means that the timer ID comes from the embedder as an int and can't be enlarged without a public API change.  So much for that idea.
Labels: OS-All
Project Member Comment 9 by sheriffbot@chromium.org, Jan 12 2017
Labels: M-55
Project Member Comment 10 by sheriffbot@chromium.org, Jan 12 2017
Labels: Pri-1
Project Member Comment 11 by bugdroid1@chromium.org, Jan 12 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34ff66f6a7ed64c19c9494b0327a7a4037b7b2ff

commit 34ff66f6a7ed64c19c9494b0327a7a4037b7b2ff
Author: pdfium-deps-roller <pdfium-deps-roller@chromium.org>
Date: Thu Jan 12 22:28:15 2017

Roll src/third_party/pdfium/ db7647083..98d00b230 (4 commits).

https://pdfium.googlesource.com/pdfium.git/+log/db7647083d0a..98d00b230aa1

$ git log db7647083..98d00b230 --date=short --no-merges --format='%ad %ae %s'
2017-01-12 dsinclair Remove used items from the CSS code.
2017-01-12 tsepez Don't put timers with ID == 0 into the global timer map.
2017-01-12 tsepez Custom toString() methods may delete annots.
2017-01-12 npm Fix leak in OJPEGReadHeaderInfoSecTablesAcTable when read fails.

BUG= 679649 , 679643 , 680520 

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/2627073004
Cr-Commit-Position: refs/heads/master@{#443386}

[modify] https://crrev.com/34ff66f6a7ed64c19c9494b0327a7a4037b7b2ff/DEPS

Status: Fixed
Labels: -M-55 reward-topanel M-57
Project Member Comment 14 by sheriffbot@chromium.org, Jan 18 2017
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -reward-topanel reward-unpaid reward-500
The panel decided to award $500 for this bug.  Cheers!
Labels: -reward-unpaid reward-inprocess
Project Member Comment 18 by sheriffbot@chromium.org, Feb 3 2017
Labels: Merge-Request-57
Project Member Comment 19 by sheriffbot@chromium.org, Feb 3 2017
Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+awhalley@ for M57 merge review.
+govind: good to approve for 57.  Note that we only need a merge of "Don't put timers with ID == 0 into the global timer map." not necessarily the full DEPS roll.
Labels: -Merge-Review-57 Merge-Approved-57
Approving merge to M57 branch 2987. Please refer to comment #21 before merging. Thank you.
Comment 23 Deleted
If possible, please merge your change to M57 branch 2987 today (Tuesday) before 5:00 PM PT so we can pick it up for this week beta release. Thank you.
Project Member Comment 25 by sheriffbot@chromium.org, Feb 16 2017
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
Please merge your change to M57 branch 2987 before 5:00 PM PT Friday (02/17), so we can pick it up for next week Beta release. Thank you.
Please merge your change to M57 branch 2987 before 5:00 PM PT Monday (02/20), so we can pick it up for next week Beta release. Thank you.
Project Member Comment 28 by sheriffbot@chromium.org, Feb 20 2017
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
Please merge your change to M57 branch 2987 by 5:00 PM PT Tuesday (02/21) so we can pick it up for this week beta release. Thank you.
Please merge your change to M57 branch 2987 by 5:00 PM PT Monday (02/27) so we can take it in for next week last M57 Desktop Beta release before Stable promotion. Thank you.
Labels: -Hotlist-Merge-Review -Merge-Approved-57
sheriffbot is being a bit silly. The fix made it in before M57 branched. We should have merged to M56, but now it's probably too late for that, so nothing to do here except for removing the merge label.
Labels: Release-0-57
Labels: -Release-0-57 Release-0-M57
Labels: CVE-2017-5039
Project Member Comment 35 by sheriffbot@chromium.org, Apr 26
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