Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 663726 Use-after-free in ChromeExtensionsBrowserClient::GetOriginalContext upon opening menu after switching from incognito mode
Starred by 5 users Project Member Reported by rob@robwu.nl, Nov 9 2016 Back to list
Status: Verified
Owner:
OOO through July 24
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug-Security



Sign in to add a comment
Chrome 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
 
 
Labels: Security_Severity-Low Security_Impact-Stable
Thanks for the detailed report - would you happen to have the full ASAN report (including the stack traces of where it was freed and allocated)?
Comment 2 by rob@robwu.nl, 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.
asan-54.0.2840.90.log
14.4 KB View Download
asan-56.0.2892.0.log
19.0 KB View Download
Components: UI>Browser>Profiles
Owner: skuhne@chromium.org
Status: Assigned
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?)
Project Member Comment 4 by sheriffbot@chromium.org, Nov 10 2016
Labels: Pri-2
Cc: asargent@chromium.org
This might be the same as crbug.com/651834, which Antony's been investigating.
Owner: asargent@chromium.org
Status: Started
I'll have a look. 

Comment 7 by rob@robwu.nl, Nov 15 2016
Can you cc me on bug 651834?
Cc: -rdevlin....@chromium.org
Owner: rdevlin....@chromium.org
asargent@'s been kept busy with other things; I'll have a look at this.
Project Member Comment 9 by bugdroid1@chromium.org, 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

Comment 10 by rob@robwu.nl, Nov 19 2016
Status: Verified
Verified fixed in 57.0.2926.0, following the STR from the report.
Project Member Comment 11 by sheriffbot@chromium.org, Nov 20 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Issue 651834 has been merged into this issue.
Labels: Merge-Request-56
Comment 14 by dimu@chromium.org, Nov 30 2016
Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Issue 667887 has been merged into this issue.
Project Member Comment 16 by bugdroid1@chromium.org, Nov 30 2016
Labels: -merge-approved-56 merge-merged-2924
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

Labels: Release-0-M56
Labels: CVE-2017-5021
Project Member Comment 19 by sheriffbot@chromium.org, Feb 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