New issue
Advanced search Search tips

Issue 681462 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in views::MenuController::SetSelection

Project Member Reported by ClusterFuzz, Jan 16 2017

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6320762245087232

Fuzzer: meacer_chromebot_extensions
Job Type: linux_asan_chrome_v8_arm
Platform Id: linux

Crash Type: Heap-use-after-free READ 4
Crash Address: 0xca748610
Crash State:
  views::MenuController::SetSelection
  views::MenuController::ExitMenuRun
  views::MenuController::Accept
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_v8_arm&range=441524:442831

Minimized Testcase (45.53 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95PbtoimMeb42qmZSdV9we_hXRFBVe4lQyg7XUtj7yeEbUe83_Y3looqFE8oqKYmxo3g-wy9rHoC9PbWDQqKxkmhLDoLY3iE-wHaoQt7z-jTDmi9cLxCQrFCQOmO1MKvDN5nQt9QCNVsTXo23dskoh-ynstUgJITK8Da7wk7yI8bvDhL8_PGkoSjzCMk750gAhV6-zgRwTCus-HqqpnhKit58l0GwLQ4XYIwiVvJrfCErv5pCkihAX0sOCAsJPcU17iwE6PL8IUyQzBBOhYB5VwsFQpnlMbslb3xBDW1Fpoyx5pngDZJbEUGq5VBZ8vL3iKE-Mdnxu_eI8w9okK8jSnZTlnMDWyXF284uRo5MLtTG8HyTQ?testcase_id=6320762245087232

Additional requirements: Requires Gestures

Issue filed automatically.

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Jan 16 2017

Labels: M-57
Project Member

Comment 2 by sheriffbot@chromium.org, Jan 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 3 by sheriffbot@chromium.org, Jan 16 2017

Labels: Pri-1
Components: Internals>Views
Owner: jonr...@chromium.org
Status: Assigned (was: Untriaged)
jonross: not sure what's causing this one. The blamelist doesn't seem to have anything relevant. Can you please take a look?
It looks like the MenuController has been freed, but the key event has still been propagated to the now-freed object. Some sort of at exit destruction ordering?
Status: Started (was: Assigned)
I see the cause, but can't use the attached testcase to repro. I'll setup a unit test that can repro and fix it.

It's a weird destruction ordering issue. It's not a key event propagating, it's actually still the same key that closed the menu. However we have nested menus, and they had been blocking the browser shutdown. So now we have the browser closing mid unraveling of nested menus.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 18 2017

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

commit faaee985121bc612a5a79b03215a490f7f65d0eb
Author: jonross <jonross@chromium.org>
Date: Wed Jan 18 00:30:37 2017

Fix MenuController Heap-use-after-free

MenuController applies a ref to ViewsDelegate, in order to prevent Chrome from
shutting down while a menu is open. This ref is released as the menu is closing.

However it is possible for the release of the ref to lead to Chrome shutting
down immediately. During this MenuController is deleted. However it was possible
that MenuController would access the heap as the stack collapsed.

This change updates the menu closing process to detect the deletion and to
shutdown cleanly.

TEST=MenuControllerTest.DestroyedDuringViewsRelease
BUG= 681462 

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

[modify] https://crrev.com/faaee985121bc612a5a79b03215a490f7f65d0eb/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/faaee985121bc612a5a79b03215a490f7f65d0eb/ui/views/controls/menu/menu_controller_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 18 2017

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

commit cd4f55690437f7c40bfa710586781ec316cc889e
Author: meade <meade@chromium.org>
Date: Wed Jan 18 05:18:53 2017

Revert of Fix MenuController Heap-use-after-free (patchset #2 id:20001 of https://codereview.chromium.org/2636293002/ )

Reason for revert:
Caused a memory leak in BookmarkBarViewTest7.DNDToDifferentMenu and BookmarkBarViewTest8.DNDBackToOriginatingMenu

See  crbug.com/682109 

Build link:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/19037

Original issue's description:
> Fix MenuController Heap-use-after-free
>
> MenuController applies a ref to ViewsDelegate, in order to prevent Chrome from
> shutting down while a menu is open. This ref is released as the menu is closing.
>
> However it is possible for the release of the ref to lead to Chrome shutting
> down immediately. During this MenuController is deleted. However it was possible
> that MenuController would access the heap as the stack collapsed.
>
> This change updates the menu closing process to detect the deletion and to
> shutdown cleanly.
>
> TEST=MenuControllerTest.DestroyedDuringViewsRelease
> BUG= 681462 
>
> Review-Url: https://codereview.chromium.org/2636293002
> Cr-Commit-Position: refs/heads/master@{#444203}
> Committed: https://chromium.googlesource.com/chromium/src/+/faaee985121bc612a5a79b03215a490f7f65d0eb

TBR=sky@chromium.org,jonross@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 681462 

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

[modify] https://crrev.com/cd4f55690437f7c40bfa710586781ec316cc889e/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/cd4f55690437f7c40bfa710586781ec316cc889e/ui/views/controls/menu/menu_controller_unittest.cc

Project Member

Comment 9 by ClusterFuzz, Jan 18 2017

ClusterFuzz has detected this issue as fixed in range 443977:443979.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6320762245087232

Fuzzer: meacer_chromebot_extensions
Job Type: linux_asan_chrome_v8_arm
Platform Id: linux

Crash Type: Heap-use-after-free READ 4
Crash Address: 0xca748610
Crash State:
  views::MenuController::SetSelection
  views::MenuController::ExitMenuRun
  views::MenuController::Accept
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_v8_arm&range=441524:442831
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_v8_arm&range=443977:443979

Minimized Testcase (45.53 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95PbtoimMeb42qmZSdV9we_hXRFBVe4lQyg7XUtj7yeEbUe83_Y3looqFE8oqKYmxo3g-wy9rHoC9PbWDQqKxkmhLDoLY3iE-wHaoQt7z-jTDmi9cLxCQrFCQOmO1MKvDN5nQt9QCNVsTXo23dskoh-ynstUgJITK8Da7wk7yI8bvDhL8_PGkoSjzCMk750gAhV6-zgRwTCus-HqqpnhKit58l0GwLQ4XYIwiVvJrfCErv5pCkihAX0sOCAsJPcU17iwE6PL8IUyQzBBOhYB5VwsFQpnlMbslb3xBDW1Fpoyx5pngDZJbEUGq5VBZ8vL3iKE-Mdnxu_eI8w9okK8jSnZTlnMDWyXF284uRo5MLtTG8HyTQ?testcase_id=6320762245087232

Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs 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 10 by ClusterFuzz, Jan 18 2017

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

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

Comment 11 by sheriffbot@chromium.org, Jan 18 2017

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

Comment 13 by sheriffbot@chromium.org, Apr 26 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