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

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment

Investigate lifetime of the NativeWindow parent in ExtensionUninstallDialog

Project Member Reported by sashab@chromium.org, Jul 25 2014 Back to list

Issue description

Since the ExtensionUninstallDialog is not constructed until its image has loaded, there could be a lifetime issue where the parent window is closed in the time between when the ExtensionUninstallDialog is created and the app icon has been loaded. If the parent window is freed in this time, but ExtensionUninstallDialog is not, OnImageLoaded() will run with an already-freed parent_ variable and crash.

To fix this, we could add a WidgetObserver to the parent window to watch for when it closes, and then prevent the dialog from launching if it closes in this time. Alternatively, the dialog could be constructed in the constructor, OnClosed() can be overridden to call ExtensionUninstallCancelled(), and OnImageLoaded can simply call Show() on the widget to display it (however, this may need a refresh call to re-layout the dialog controls now that the icon has been changed).

Another solution again would be to make uninstall a sub-class of the install dialog, which just doesn't display the permissions and has slightly different text. However, it should first be investigated whether ExtensionInstallPrompt is also vulnerable to this problem.
 

Comment 1 by kalman@chromium.org, Jul 25 2014

OnImageLoaded is base::Bound to a WeakPtr of ExtensionUninstallDialog... it sounds like a good solution would be the WidgetObserver approach, and the Dialog deleting itself when the parent closes?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/extension_uninstall_dialog.cc&sq=package:chromium&rcl=1406209044&l=97
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 4 2014

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

commit ea87b86d5bd1abe26636abfc19af28e755da7585
Author: pkotwicz <pkotwicz@chromium.org>
Date: Tue Nov 04 22:42:33 2014

Cancel uninstall if the uninstall dialog's parent window is destroyed prior to
the uninstall dialog showing.

BUG= 424999 ,  397396 
TEST=ExtensionUninstallDialogViewBrowserTest.TrackParentWindowDestruction

Review URL: https://codereview.chromium.org/697023002

Cr-Commit-Position: refs/heads/master@{#302691}

[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/extensions/extension_uninstall_dialog.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/extensions/extension_uninstall_dialog.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
[add] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/ea87b86d5bd1abe26636abfc19af28e755da7585/chrome/chrome_tests.gypi

Labels: -Type-Bug Type-Bug-Security Security_Severity-High Restrict-View-SecurityTeam
Status: Fixed (was: NULL)
Project Member

Comment 4 by ClusterFuzz, Nov 5 2014

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-NA
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 11 2014

Labels: merge-merged-2171
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9

commit 6919459fb7f674aaf05277ebe2fe12e746d4a3f9
Author: Peter Kotwicz <pkotwicz@google.com>
Date: Tue Nov 11 16:46:12 2014

[Merge] Cancel uninstall if the uninstall dialog's parent window is destroyed prior to the uninstall dialog showing.

BUG= 424999 ,  397396 
TEST=ExtensionUninstallDialogViewBrowserTest.TrackParentWindowDestruction
TBR=pkotwicz

Review URL: https://codereview.chromium.org/697023002

Review URL: https://codereview.chromium.org/711343003

Cr-Commit-Position: refs/branch-heads/2171@{#402}
Cr-Branched-From: 267aeeb8d85c8503a7fd12bd14654b8ea78d3974-refs/heads/master@{#297060}

[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/extensions/extension_uninstall_dialog.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/extensions/extension_uninstall_dialog.h
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/ui/cocoa/extensions/extension_uninstall_dialog_cocoa.mm
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
[add] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
[modify] https://chromium.googlesource.com/chromium/src.git/+/6919459fb7f674aaf05277ebe2fe12e746d4a3f9/chrome/chrome_tests.gypi

Labels: -Merge-NA Release-0-M39
Project Member

Comment 7 by ClusterFuzz, Nov 12 2014

Labels: -Release-0-M39
This bug is a regression and does not impact stable. Removing incorrectly added Release-0-M39 label.

- Your friendly ClusterFuzz
Labels: Security_Impact-Stable Release-0-M39
Project Member

Comment 9 by ClusterFuzz, Feb 11 2015

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 10 by sheriffbot@chromium.org, Oct 1 2016

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

Comment 11 by sheriffbot@chromium.org, Oct 2 2016

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
Labels: allpublic

Sign in to add a comment