New issue
Advanced search Search tips
Starred by 2 users
Status: Fixed
Owner:
Closed: Jun 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Android, Windows, Chrome, Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: IndexedDB OpenCursor UaF
Reported by nedwilli...@gmail.com, Jun 2 Back to list
VULNERABILITY DETAILS
In content/browser/indexed_db/indexed_db_database.cc, leveldb::Status IndexedDBDatabase::OpenCursorOperation creates a cursor and passes ownership of it to an instance of IndexedDBCallbacks, which itself takes ::indexed_db::mojom::CallbacksAssociatedPtrInfo callbacks_info which is provided by the renderer. If callbacks_info is invalid, the IndexedDBCallbacks leaves callbacks_ uninitialized. Before passing ownership of the cursor, it stores a raw pointer to the cursor in the transaction.

The unique_ptr owning the cursor makes its way to IndexedDBCallbacks::IOThreadHelper::SendSuccessCursor, where it is finally added to the dispatcher. But if the IndexedDBCallbacks doesn't have a callbacks_ or dispatcher_host_, the ownership is not passed and so the cursor gets destructed. A UaF occurs when the raw pointer to the cursor is accessed as accessed as part of the transaction.

I'm not sure if this requires a renderer patch, as there may be another way to destroy the dispatcher host or cause the mojo binding to fail before the callback occurs, but this should sufficient to show the issue.

VERSION
Chrome Version: 58.0 Stable
Operating System: All

REPRODUCTION CASE
Apply the attached renderer patch and open cursor.html. Close the browser or refresh to see the ASAN error.

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: browser
Crash State: see asan.log
 
asan.log
13.7 KB View Download
no_callback.patch
826 bytes Download
cursor.html
274 bytes View Download
I think we can fix this by ensuring that the cursor removes itself from the transaction when IndexedDBCursor is destructed, rather than when CursorImpl (that we return before constructing) is destructed. See the attached patch.
fix.patch
940 bytes Download
Components: Blink>Storage>IndexedDB
Cc: cmumford@chromium.org dmu...@chromium.org
Labels: Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: jsb...@chromium.org
Status: Assigned
jsbell: Would you mind taking a look at this or reassigning?
Cc: -cmumford@chromium.org pwnall@chromium.org
Owner: dmu...@chromium.org
dmurph@ - please take a look
Cc: jsb...@chromium.org
Project Member Comment 6 by sheriffbot@chromium.org, Jun 6
Labels: M-59
Project Member Comment 7 by sheriffbot@chromium.org, Jun 6
Labels: Pri-1
Project Member Comment 8 by bugdroid1@chromium.org, Jun 7
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d007b8b750851fe1b375c463009ea3b24e5c021d

commit d007b8b750851fe1b375c463009ea3b24e5c021d
Author: Daniel Murphy <dmurph@chromium.org>
Date: Wed Jun 07 01:24:31 2017

[IndexedDB] Fix Cursor UAF

If the connection is closed before we return a cursor, it dies in
IndexedDBCallbacks::IOThreadHelper::SendSuccessCursor. It's deleted on
the correct thread, but we also need to makes sure to remove it from its
transaction.

To make things simpler, we have the cursor remove itself from its
transaction on destruction.

R: pwnall@chromium.org
Bug:  728887 
Change-Id: I8c76e6195c2490137a05213e47c635d12f4d3dd2
Reviewed-on: https://chromium-review.googlesource.com/526284
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477504}
[modify] https://crrev.com/d007b8b750851fe1b375c463009ea3b24e5c021d/content/browser/indexed_db/cursor_impl.cc
[modify] https://crrev.com/d007b8b750851fe1b375c463009ea3b24e5c021d/content/browser/indexed_db/indexed_db_cursor.cc
[modify] https://crrev.com/d007b8b750851fe1b375c463009ea3b24e5c021d/content/browser/indexed_db/indexed_db_cursor.h

Labels: Merge-Request-60 Merge-Request-59
Project Member Comment 10 by sheriffbot@chromium.org, Jun 7
Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 11 by sheriffbot@chromium.org, Jun 7
Labels: -Merge-Request-59 Merge-Review-59 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 12 by sheriffbot@chromium.org, Jun 7
Status: Fixed
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
Cc: awhalley@chromium.org
+awhalley@ for security review. 
Thanks for confirming - can you please confirm if both changes are safe merges, and if they are tested/verified in canary?
Project Member Comment 15 by sheriffbot@chromium.org, Jun 8
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
I strongly believe the change is safe - it should be in canary today. I think it is safe to merge.
Labels: -Merge-Review-59 Merge-Rejected-59
Per discussion with awhalley@, since this is not baked in canary, dev, or beta yet - we will not consider this for Monday's M59 respin. Per the usual process, merges should be baked in beta prior to coming to stable.
Labels: reward-topanel
Project Member Comment 19 by sheriffbot@chromium.org, Jun 12
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
Labels: -M-59 M-60
Project Member Comment 21 by bugdroid1@chromium.org, Jun 12
Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/51bbaf611b628c9396f573435bb0a9289434923f

commit 51bbaf611b628c9396f573435bb0a9289434923f
Author: Daniel Murphy <dmurph@chromium.org>
Date: Mon Jun 12 18:40:02 2017

[IndexedDB] Fix Cursor UAF

If the connection is closed before we return a cursor, it dies in
IndexedDBCallbacks::IOThreadHelper::SendSuccessCursor. It's deleted on
the correct thread, but we also need to makes sure to remove it from its
transaction.

To make things simpler, we have the cursor remove itself from its
transaction on destruction.

TBR=dmurph@chromium.org

(cherry picked from commit d007b8b750851fe1b375c463009ea3b24e5c021d)

R: pwnall@chromium.org
Bug:  728887 
Change-Id: I8c76e6195c2490137a05213e47c635d12f4d3dd2
Reviewed-on: https://chromium-review.googlesource.com/526284
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#477504}
Reviewed-on: https://chromium-review.googlesource.com/531774
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#308}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/51bbaf611b628c9396f573435bb0a9289434923f/content/browser/indexed_db/cursor_impl.cc
[modify] https://crrev.com/51bbaf611b628c9396f573435bb0a9289434923f/content/browser/indexed_db/indexed_db_cursor.cc
[modify] https://crrev.com/51bbaf611b628c9396f573435bb0a9289434923f/content/browser/indexed_db/indexed_db_cursor.h

Labels: -reward-topanel reward-unpaid reward-1000
Congratulations nedwilliamson@! The VRP panel decided to award $1,000 for this report :-)
Thanks! Though I thought because this was a use-after-free in the browser process it would be considered a sandbox escape. I was able to reliably crash the browser in my testing but didn't get register control yet (the object is smaller so spraying on the IDB thread isn't as reliable as my other bug, at least using the same spray technique). If there's something I can do to improve the report please let me know!
Labels: -reward-unpaid reward-inprocess
Labels: -Reward-1000 reward-10000
Hi nedwilliamson@ - I'm terribly sorry, we made a mistake and the reward was too low by a decimal order of magnitude!  Thank you for raising the fact you thought it was a bit low :-D  I've managed to get the value changed with our finance folk so it shouldn't delay payment.
awhalley@: Wow, thanks! I just wanted to make sure I wasn't missing any useful information in the report. Thanks again for the generous reward! :-)
Labels: Release-0-M60
Labels: CVE-2017-5091
Project Member Comment 30 by sheriffbot@chromium.org, Sep 14
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