Issue metadata
Sign in to add a comment
|
Use-after-free in ChromeExtensionsBrowserClient::GetOriginalContext upon opening menu after switching from incognito mode |
||||||||||||||||||||||
Issue descriptionChrome version: 54.0.2840.90 (stable, and latest 56.0.2892.0) Steps to reproduce: 1. Start Chrome: ./chrome --user-data-dir=/tmp/whatever 2. Install an extension with an icon (so not an extension with an auto-generated letter icon) - e.g. https://chrome.google.com/webstore/detail/display-anchors/poahndpaaanbpbeafbkploiobpiiieko 3. Visit chrome://extensions and enable the extension in incognito mode. 4. Hide the extension button (right-click on the icon, "Hide in Chromium menu". 5. Close Chrome. 6. Start Chrome in incognito mode: ./chrome --user-data-dir=/tmp/whatever --incognito 7. Open a non-incognito window (e.g. Ctrl+N) 8. Close the incognito window. 9. Click on the menu in the window from step 7. (the above steps describe a user who uses incognito mode by default, and then switches to a non-incognito window) Expected: - The menu shows up Actual: - Crash, heap-use-after-free. ASan stack trace in 54.0.2840.90 ==19978==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c00007b640 at pc 0x5583343cd99a bp 0x7ffc35953940 sp 0x7ffc35953938 READ of size 8 at 0x60c00007b640 thread T0 (chrome) #0 0x5583343cd999 in extensions::ChromeExtensionsBrowserClient::GetOriginalContext(content::BrowserContext*) chrome/browser/extensions/chrome_extensions_browser_client.cc:119:42 #1 0x5583321658ab in KeyedServiceFactory::GetServiceForContext(base::SupportsUserData*, bool) components/keyed_service/core/keyed_service_factory.cc:65:13 #2 0x55832f2cafdf in extensions::IconImage::LoadImageForScaleFactor(ui::ScaleFactor) extensions/browser/extension_icon_image.cc:206:7 #3 0x55832f2ca822 in extensions::IconImage::Source::GetImageForScale(float) extensions/browser/extension_icon_image.cc:116:16 #4 0x558331e772d1 in gfx::internal::ImageSkiaStorage::FindRepresentation(float, bool) const ui/gfx/image/image_skia.cc:240:24 #5 0x558331e79b05 in gfx::ImageSkia::GetRepresentation(float) const ui/gfx/image/image_skia.cc:414:42 #6 0x5583359c1ca1 in IconWithBadgeImageSource::Draw(gfx::Canvas*) chrome/browser/ui/extensions/icon_with_badge_image_source.cc:136:32 #7 0x558331ea2142 in gfx::CanvasImageSource::GetImageForScale(float) ui/gfx/image/canvas_image_source.cc:22:3 #8 0x558331e772d1 in gfx::internal::ImageSkiaStorage::FindRepresentation(float, bool) const ui/gfx/image/image_skia.cc:240:24 #9 0x558331e79b05 in gfx::ImageSkia::GetRepresentation(float) const ui/gfx/image/image_skia.cc:414:42 ASan stack trace in 56.0.2892.0 (debug build) ==20697==ERROR: AddressSanitizer: heap-use-after-free on address 0x6100000a8740 at pc 0x55b3a6b7fb5c bp 0x7fff7467b190 sp 0x7fff7467b188 READ of size 8 at 0x6100000a8740 thread T0 (chrome) #0 0x55b3a6b7fb5b in extensions::ChromeExtensionsBrowserClient::GetOriginalContext(content::BrowserContext*) chrome/browser/extensions/chrome_extensions_browser_client.cc:120:42 #1 0x55b39f192049 in extensions::ImageLoaderFactory::GetBrowserContextToUse(content::BrowserContext*) const extensions/browser/image_loader_factory.cc:44:42 #2 0x7f22e46ebf1b in BrowserContextKeyedServiceFactory::GetContextToUse(base::SupportsUserData*) const components/keyed_service/content/browser_context_keyed_service_factory.cc:105:10 #3 0x7f22e9fcfa56 in KeyedServiceFactory::GetServiceForContext(base::SupportsUserData*, bool) components/keyed_service/core/keyed_service_factory.cc:65:13 #4 0x7f22e46eb996 in BrowserContextKeyedServiceFactory::GetServiceForBrowserContext(content::BrowserContext*, bool) components/keyed_service/content/browser_context_keyed_service_factory.cc:45:31 #5 0x55b39f191e91 in extensions::ImageLoaderFactory::GetForBrowserContext(content::BrowserContext*) extensions/browser/image_loader_factory.cc:17:22 #6 0x55b39f17aab4 in extensions::ImageLoader::Get(content::BrowserContext*) extensions/browser/image_loader.cc:226:10 #7 0x55b39f0602f4 in extensions::IconImage::LoadImageForScaleFactor(ui::ScaleFactor) extensions/browser/extension_icon_image.cc:206:7 #8 0x55b39f05f598 in extensions::IconImage::Source::GetImageForScale(float) extensions/browser/extension_icon_image.cc:116:16 #9 0x7f22fbf7fe34 in gfx::internal::ImageSkiaStorage::FindRepresentation(float, bool) const ui/gfx/image/image_skia.cc:240:24 #10 0x7f22fbf868bd in gfx::ImageSkia::GetRepresentation(float) const ui/gfx/image/image_skia.cc:414:42 #11 0x55b3aa2a8b65 in IconWithBadgeImageSource::Draw(gfx::Canvas*) chrome/browser/ui/extensions/icon_with_badge_image_source.cc:74:32 #12 0x7f22fc0179a0 in gfx::CanvasImageSource::GetImageForScale(float) ui/gfx/image/canvas_image_source.cc:23:3 #13 0x7f22fbf7fe34 in gfx::internal::ImageSkiaStorage::FindRepresentation(float, bool) const ui/gfx/image/image_skia.cc:240:24 #14 0x7f22fbf868bd in gfx::ImageSkia::GetRepresentation(float) const ui/gfx/image/image_skia.cc:414:42
,
Nov 9 2016
Yes I do. I've just created full ASAN reports following the steps from the report. But the relevant parts of the stack trace are already in the report. The UAF is an attempt to dereference a Profile* (BrowserContext*) (=the incognito profile). So the creation shows that it is created on startup, and deleted when the last window in the incognito profile is closed). One of the objects near the top of the stack should have been destroyed when the incognito profile went away, since they (indirectly) store a Profile/BrowserContext pointer.
,
Nov 9 2016
Ah, thanks for the context. It looks like we keep a raw pointer to a profile at https://cs.chromium.org/chromium/src/extensions/browser/extension_icon_image.h?l=103, and it goes all the way back to perhaps here: https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc?l=139 (I'm not 100% sure, there were several IconImage constructors, so I guessed). skuhne, I see your name in a TODO about multi profile awareness for icon loading, mind taking a look at this (or assigning someone who might be more appropriate?)
,
Nov 10 2016
,
Nov 10 2016
This might be the same as crbug.com/651834, which Antony's been investigating.
,
Nov 10 2016
I'll have a look.
,
Nov 15 2016
Can you cc me on bug 651834?
,
Nov 17 2016
asargent@'s been kept busy with other things; I'll have a look at this.
,
Nov 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d09e1c4d5c955d73cc38f507ede1119f6c540320 commit d09e1c4d5c955d73cc38f507ede1119f6c540320 Author: rdevlin.cronin <rdevlin.cronin@chromium.org> Date: Sat Nov 19 01:03:20 2016 [Extensions] Fix lifetime bug in ExtensionAction/IconImage ExtensionActions can lazily load the default icon image, which is implemented as an IconImage. The IconImage, in turn, is constructed with a BrowserContext because it needs access to the ImageLoader (a BrowserContextKeyedService). ExtensionActions are owned by the ExtensionActionManager (another BrowserContextKeyedService with one instance shared across incognito and normal profiles), but took a BrowserContext as an argument when loading the default icon image. This all caused a problem when an incognito profile was passed in as the browser context to load the default icon for the extension action. The extension action would then create an IconImage with the incognito profile as a context, but the ExtensionAction (and thus the IconImage) are owned by the ExtensionActionManager, and are therefore not deleted upon the incognito profile's destruction. This means that if you have a flow where the ExtensionAction's default icon is loaded via an incognito profile, the incognito profile is deleted, and then the icon is later used in the normal profile, there's a crash. Fix this by having the ExtensionActionManager assign the IconImage to the ExtensionAction using its own profile. Since the ExtensionActions are owned by the ExtensionActionManager, and the ExtensionActionManager is owned by the profile by being a BrowserContextKeyedService, this is guaranteed to be safe. This also abstracts out the context of a profile or BrowserContext from the ExtensionAction class, keeping it more in line with its data-structure-like concept. Add a regression test. BUG= 663726 (and possibly others) Review-Url: https://codereview.chromium.org/2506273002 Cr-Commit-Position: refs/heads/master@{#433361} [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/extensions/extension_action.cc [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/extensions/extension_action.h [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/extensions/extension_action_icon_factory.cc [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/extensions/extension_action_icon_factory_unittest.cc [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/extensions/extension_action_manager.cc [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/extensions/extension_browsertest.cc [modify] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc [add] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/test/data/extensions/api_test/browser_action_with_icon/icon.png [add] https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320/chrome/test/data/extensions/api_test/browser_action_with_icon/manifest.json
,
Nov 19 2016
Verified fixed in 57.0.2926.0, following the STR from the report.
,
Nov 20 2016
,
Nov 29 2016
Issue 651834 has been merged into this issue.
,
Nov 30 2016
,
Nov 30 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Nov 30 2016
Issue 667887 has been merged into this issue.
,
Nov 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf76b5aaca31f715c74bb3274edf748901c1a6d7 commit bf76b5aaca31f715c74bb3274edf748901c1a6d7 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Wed Nov 30 01:56:05 2016 [Extensions] Fix lifetime bug in ExtensionAction/IconImage ExtensionActions can lazily load the default icon image, which is implemented as an IconImage. The IconImage, in turn, is constructed with a BrowserContext because it needs access to the ImageLoader (a BrowserContextKeyedService). ExtensionActions are owned by the ExtensionActionManager (another BrowserContextKeyedService with one instance shared across incognito and normal profiles), but took a BrowserContext as an argument when loading the default icon image. This all caused a problem when an incognito profile was passed in as the browser context to load the default icon for the extension action. The extension action would then create an IconImage with the incognito profile as a context, but the ExtensionAction (and thus the IconImage) are owned by the ExtensionActionManager, and are therefore not deleted upon the incognito profile's destruction. This means that if you have a flow where the ExtensionAction's default icon is loaded via an incognito profile, the incognito profile is deleted, and then the icon is later used in the normal profile, there's a crash. Fix this by having the ExtensionActionManager assign the IconImage to the ExtensionAction using its own profile. Since the ExtensionActions are owned by the ExtensionActionManager, and the ExtensionActionManager is owned by the profile by being a BrowserContextKeyedService, this is guaranteed to be safe. This also abstracts out the context of a profile or BrowserContext from the ExtensionAction class, keeping it more in line with its data-structure-like concept. Add a regression test. BUG= 663726 (and possibly others) Review-Url: https://codereview.chromium.org/2506273002 Cr-Commit-Position: refs/heads/master@{#433361} (cherry picked from commit d09e1c4d5c955d73cc38f507ede1119f6c540320) Review URL: https://codereview.chromium.org/2537263003 . Cr-Commit-Position: refs/branch-heads/2924@{#181} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/extensions/extension_action.cc [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/extensions/extension_action.h [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/extensions/extension_action_icon_factory.cc [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/extensions/extension_action_icon_factory_unittest.cc [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/extensions/extension_action_manager.cc [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/extensions/extension_browsertest.cc [modify] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc [add] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/test/data/extensions/api_test/browser_action_with_icon/icon.png [add] https://crrev.com/bf76b5aaca31f715c74bb3274edf748901c1a6d7/chrome/test/data/extensions/api_test/browser_action_with_icon/manifest.json
,
Jan 24 2017
,
Jan 25 2017
,
Feb 26 2017
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
,
Apr 25 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rickyz@chromium.org
, Nov 9 2016