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

Issue 134101 link

Starred by 3 users

Issue metadata

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

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Security: webRequest API allows extensions to XSS chrome.google.com and gain access to webstorePrivate API

Reported by t...@adblockplus.org, Jun 22 2012

Issue description

This template is ONLY for reporting security bugs. Please use a different
template for other types of bug reports.

Please see the following link for instructions on filing security bugs:
http://www.chromium.org/Home/chromium-security/reporting-security-bugs


VULNERABILITY DETAILS
I noticed a while ago that https://chrome.google.com/webstore/ has access to a special chrome.webstorePrivate API. That's probably the reason why this site is protected against extensions - injecting content scripts won't work and webNavigation API won't receive requests for the chrome.google.com host.

This protection isn't complete however. https://chrome.google.com/webstore/ loads scripts from other hosts, e.g. https://www.google.com/jsapi. When it requests these scripts the requests can be intercepted by the webNavigation API and replaced by extension's own scripts. These scripts will execute in the context of the web store page, in particular they will have access to chrome.webstorePrivate API.

VERSION
Chrome Version: 20.0.1132.27 beta (all other versions affected as well)
Operating System: Windows 7 x64

REPRODUCTION CASE
The attached extension (unzip and load as unpacked extension in Chrome to test) demonstrates this approach. It replaces https://www.google.com/jsapi by its own xss.js script. It then opens web store in a new window, window contents are replaced by an unsuspicious message to prevent the user from suspecting anything (loading web store into a frame inside background.html is prevented by X-Frame-Options header). The xss.js script then goes on to install "Google +1 Button" extension silently.

Granted, only a few whitelisted extensions can be installed silently so the extension also demonstrates a possible approach for other extensions. It nags the user into installing "RSS Subscription Extension" (chosen randomly, any other extension in Web Store will do as well), screenshot of the nag screen is attached. If the user declines the installation then the same dialog will simply come up again, no way to break out of the loop. This is different from chrome.webstore.install() - you aren't bound to installation on user gesture which allows you to nag the user until he finally accepts. Also, you can determine the displayed information - by manipulating the extension name and icon I made this dialog looks like a recommendation coming from Chrome itself.

The web store also has access to the chrome.management API. I didn't implement this in my proof of concept but an attacker who got two extensions installed in user's browser can effectively prevent them from ever being uninstalled: both extensions use chrome.management API to detect when the other is uninstalled. If that happens they use the nagging approach described above to make sure that the extension is reinstalled. If an extension is disabled then they can simply re-enable it directly, without asking the user.

If the goal is really to protect https://chrome.google.com/webstore/ then the webRequest API should not receive *any* requests made by https://chrome.google.com/webstore/, no matter what host these requests point to. In other words, the exclusion criterion here should be the document URL, not the request URL. This will also fix the issue that prompted me to investigate this in the first place (Adblock Plus mistakenly blocked some requests on https://chrome.google.com/webstore/ because it saw these requests without receiving the main_frame request first - missing context information caused the wrong action).

Note: I've sent this to security@google.com before filing a bug here.
 
nagscreen.png
20.1 KB View Download
storexss.zip
8.8 KB Download

Comment 1 by jsc...@chromium.org, Jun 22 2012

Cc: mpcomplete@chromium.org aa@chromium.org jhurwich@chromium.org
Labels: -Pri-0 -Area-Undefined Pri-2 Area-Internals Feature-Extensions SecSeverity-Medium Feature-Webstore SecImpacts-Stable SecImpacts-Beta OS-All Mstone-21
Status: Available
This bug itself isn't that bad, since it requires a malicious extension. However, the bigger worry is that it looks like a stepping stone to a sandbox escape.
Well, the initial malicious extension can be something supposedly harmless like a search results enhancer - it only needs to declare access to google.com. But it can then force the user into installing an extension with an NPAPI plugin (who will be surprised that "Google Security Essentials" want to access all data on your computer?) and gain full access to the system. If the second extension then installs a scareware application on the computer and immediately uninstalls itself then there will be no trace left for the user to figure out what really happened.

Comment 3 by jsc...@chromium.org, Jun 22 2012

Sorry, I didn't mean to sound dismissive. I was just providing context for the severity rating. In terms of larger scale attacks, the big fear is in using this as a component of a sandbox escape (e.g. http://blog.chromium.org/2012/06/tale-of-two-pwnies-part-2.html ).

Comment 4 by jsc...@chromium.org, Jun 22 2012

Cc: jsc...@chromium.org
Makes sense. The webRequest API uses the URL being fetched, not the URL of the page doing the fetching, to determine permissions. It sounds like #0's suggestion is right - we should restrict any requests made by the webstore.

Comment 6 by aa@chromium.org, Jun 22 2012

Owner: battre@chromium.org
Status: Assigned
Summary: Security: webRequest API allows extensions to XSS chrome.google.com and gain access to webstorePrivate API
Yeah, I think the issue here is that we shouldn't be exposing resource requests through the webRequest API that are embedded from hosts you don't have host permissions for.
Maybe CanExtensionAccessURL should take into account the first_party_for_cookies URL? (I'm not sure which field on URLRequest contains the host that issued the request, if any.)

Comment 8 by aa@chromium.org, Jun 22 2012

I looked into this for the downloads API. first_party_for_cookies usually works, but there are some cases where it isn't included (I think due to privacy settings or something?). I think my conclusion was that there's currently no reliable way to get this information out of URLRequest.

Comment 9 by battre@chromium.org, Jun 25 2012

Cc: creis@chromium.org bauerb@chromium.org
I can look into the first_party_for_cookies approach. Maybe we want to use that as a first step to raise the barrier.

Unfortunately, though, this won't be a perfect fix even if we get it right due to the possibility of cache poisoning (kudos for Bernhard for pointing this out). We wonder whether isolated apps could help us.
Here is a patch for the proposed change for comment #7 (not sure whether I am supposed to upload it into codereview).

I could verify this only in debug builds after applying this patch:

diff --git a/content/browser/web_contents/render_view_host_manager.cc b/content/browser/web_contents/render_view_host_manager.cc
index a440682..30774c4 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -236,7 +236,7 @@ void RenderViewHostManager::DidNavigateMainFrame(
 }
 
 void RenderViewHostManager::SetWebUIPostCommit(WebUIImpl* web_ui) {
-  DCHECK(!web_ui_.get());
+  //DCHECK(!web_ui_.get());
   web_ui_.reset(web_ui);
 }

Otherwise the DCHECK would fire:
[10339:10339:2413158038712:FATAL:render_view_host_manager.cc(239)] Check failed: !web_ui_.get(). 
Backtrace:
        base::debug::StackTrace::StackTrace() [0x7ff374096bae]
        logging::LogMessage::~LogMessage() [0x7ff3740c7911]
        RenderViewHostManager::SetWebUIPostCommit() [0x7ff376e29113]
        WebContentsImpl::DidNavigateMainFramePostCommit() [0x7ff376e37e7c]
        WebContentsImpl::DidNavigate() [0x7ff376e39057]
        content::RenderViewHostImpl::OnMsgNavigate() [0x7ff376d974e9]
        content::RenderViewHostImpl::OnMessageReceived() [0x7ff376d94fc1]
        content::RenderProcessHostImpl::OnMessageReceived() [0x7ff376d83502]
        IPC::ChannelProxy::Context::OnDispatchMessage() [0x7ff36e00a2f5]
        base::internal::RunnableAdapter<>::Run() [0x7ff36e00d706]
        base::internal::InvokeHelper<>::MakeItSo() [0x7ff36e00d1d6]
        base::internal::Invoker<>::Run() [0x7ff36e00cab3]
        base::Callback<>::Run() [0x7ff37408f56d]
        MessageLoop::RunTask() [0x7ff3740cc29e]
        MessageLoop::DeferOrRunPendingTask() [0x7ff3740cc3b5]
        MessageLoop::DoWork() [0x7ff3740ccb9b]
        base::MessagePumpGlib::RunWithDispatcher() [0x7ff374070dc2]
        base::MessagePumpGlib::Run() [0x7ff3740711a2]
        MessageLoop::RunInternal() [0x7ff3740cbf73]
        MessageLoop::RunHandler() [0x7ff3740cbe2a]
        MessageLoopForUI::RunWithDispatcher() [0x7ff3740cd02c]
        ChromeBrowserMainParts::MainMessageLoopRun() [0x7ff379be3a83]
        content::BrowserMainLoop::RunMainMessageLoopParts() [0x7ff376b765c9]
        (anonymous namespace)::BrowserMainRunnerImpl::Run() [0x7ff376b7829a]
        BrowserMain() [0x7ff376b74d90]
        content::RunNamedProcessTypeMain() [0x7ff376b518b4]
        content::ContentMainRunnerImpl::Run() [0x7ff376b5264c]
        content::ContentMain() [0x7ff376b50f27]
        ChromeMain [0x7ff3792735dd]
        main [0x7ff37927359c]

bug-134101.patch
6.1 KB View Download

Comment 12 by creis@chromium.org, Jun 25 2012

@comment 9: The downside with isolated storage is that the user would have to sign into the Chrome Web Store separately from their normal Google sign-in.  Seems like we should make it possible to know what page is making a request, regardless of which sub-resource URL it's requesting.

@comment 10: You can check the private checkbox on a code review if you're concerned about visibility.  Only the folks CC'd will see it.
Regarding the private checkbox: I suppose that it would show up in the RSS feeds. Justin: please let me know if you want me to upload the patch.
You can just upload the patch as normal. We'd like better handling of private patches (and we've asked the codesite team before) but the truth is that the fixes sit much longer after being checked into the tree before shipping to stable anyway.

Comment 15 by aa@chromium.org, Jun 26 2012

Cc: sdoyon@chromium.org cnygaard@chromium.org keiger@chromium.org

Comment 16 by aa@chromium.org, Jun 26 2012

An easy hack might be to check the process ID of the originating request. We isolate the store in its own process, so it should be easy to identify requests originating from it.

Can you point me to the location where this happens? I wonder whether there is a simple way to check whether a specific process ID is associated with the CWS.
@creis: I think you are the master of render process switches. If I remove the chrome.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls: ["https://www.google.com/jsapi"]}, ["blocking"]); from background.js from the extension of the original post, the extension does not use the web request API anymore. Still it produces the DCHECK of comment 10. Could you look into this?

Comment 19 by aa@chromium.org, Jun 26 2012

Labels: ReleaseBlock-Stable
It would be really good to get this into M21 if we can find a nice surgical fix.
I agree. You have two CLs for review.

Comment 21 by creis@chromium.org, Jun 26 2012

@comment 18: Dominic, can you CC me on the code review so I can try out the latest patch?  I'm not clear why it's affecting whether a web_ui_ object exists on the tab's RenderViewHostManager.
Done. You don't need to apply the changelists, though. The easiest way to observe this behavior is: Download and unzip storexss.zip from the original post. Delete the first line of background.js and install the extension in a Chrome with DCHECKs enabled.

Comment 23 by creis@chromium.org, Jun 26 2012

Cc: est...@chromium.org
This is odd.  CC'ing estade@, who recently re-did the WebUI stuff in RenderViewHostManager.

Evan, the storexss.zip extension here is triggering a DCHECK in RVHM::SetWebUIPostCommit (on tip-of-tree), if you comment out all but the last line of background.js.  We're getting there because the extension is opening a popup to the Chrome Web Store.  WebContentsImpl::DidNavigateMainFramePostCommit is trying to assign a WebUI to the opened window, but it already has a WebUI.

I don't understand why we're assigning a WebUI to the opened window in that case if it already has one.  Can you take a look?
this falls outside the realm of my refactor. I don't know why this code exists at all:

  if (opener_web_ui_type_ != WebUI::kNoWebUI) {
    // If this is a window.open navigation, use the same WebUI as the renderer
    // that opened the window, as long as both renderers have the same
    // privileges.
    if (delegate_ && opener_web_ui_type_ == GetWebUITypeForCurrentState()) {
      WebUIImpl* web_ui = static_cast<WebUIImpl*>(CreateWebUI(GetURL()));
      // web_ui might be NULL if the URL refers to a non-existent extension.
      if (web_ui) {
        render_manager_.SetWebUIPostCommit(web_ui);
        web_ui->RenderViewCreated(GetRenderViewHost());
      }
    }
    opener_web_ui_type_ = WebUI::kNoWebUI;
  }

I did not originate or ever remember reading this code. I don't know what the valid use case is. I don't know why opener_web_ui_type_ isn't kNoWebUI for the extension's bg page. If it does have some valid use case, then I would agree that we probably don't need to create the new webui if one already exists (like if the website is the CWS or a chrome:// page).
Matt landed this code way back in http://codereview.chromium.org/172120 (chrome/browser/tab_contents/tab_contents.cc)
Labels: Merge-Requested
I have verified that r144529 works as intended on 22.0.1189.0 canary. The popup shows only the CWS.
Cc: kareng@google.com
CC'ing Karen to be sure that she can see the bug.

Comment 29 by kareng@google.com, Jun 29 2012

thsis is marked as security so i am assuming security team will approve it. :)
Labels: -Merge-Requested Merge-Approved
Yeah, if it merges clean fire away.
Labels: -Restrict-View-SecurityTeam -Merge-Approved Restrict-View-SecurityNotify Merge-Merged
Status: FixUnreleased
I had to revert because the unittest does not compile on the beta branch. I'll try again once I can solve the dependency hell of gclient.
Labels: CVE-2012-2853
Cc: michaeln@chromium.org
Cc: vabr@chromium.org
Project Member

Comment 40 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Status: Fixed
Project Member

Comment 42 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-Internals -Feature-Extensions -SecSeverity-Medium -Feature-Webstore -SecImpacts-Stable -SecImpacts-Beta -Mstone-21 Cr-Platform-Extensions Cr-Webstore Security-Impact-Beta Security-Severity-Medium M-21 Cr-Internals Security-Impact-Stable Type-Bug-Security
Project Member

Comment 43 by bugdroid1@chromium.org, Mar 13 2013

Labels: Restrict-View-EditIssue
Project Member

Comment 44 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member

Comment 46 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 47 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member

Comment 48 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 49 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Labels: reward-topanel
Project Member

Comment 51 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 52 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
Labels: -reward-topanel reward-unpaid reward-2000
Congratulations, the panel has decided to reward $2,000 for this bug.  Cheers!
Labels: -reward-unpaid reward-inprocess

Comment 57 by vabr@chromium.org, Nov 21 2016

Cc: -vabr@chromium.org
Labels: CVE_description-submitted

Sign in to add a comment