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

Issue 117715 link

Starred by 4 users

Issue metadata

Status: Verified
Last visit > 30 days ago
Closed: Mar 2012
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

  • Only users with EditIssue permission may comment.

Sign in to add a comment

LoadExtension binding in chrome://extensions/ is too permissive

Project Member Reported by, Mar 11 2012

Issue description

The binding that allows chrome://extensions/ is too permissive. It allows the web ui to specify the path to load, which gives an easy way for XSS's in chrome://extensions/ to upgrade to full user privileges.

The binding should just be load() (no path specified) and C++ should control putting up the dialog and loading the extension.

Comment 1 by, Mar 11 2012

Labels: -OS-Mac OS-All

Comment 2 by, Mar 11 2012

Labels: Restrict-View-SecurityTeam

Comment 3 by, Mar 11 2012

csilv, jhawkins: do you all want to handle this, or should we.
Labels: -Feature-Options Feature-Extensions
aa: Are you proposing using a native dialog for extension loading?  I have to admit I'm not familiar with chrome://extensions.

Comment 5 by, Mar 11 2012

This is a folder selection dialog, so it has to be "native", and already is. It's just a question of whether JavaScript gets to tell C++ the path to load or not. Right now it can, but it shouldn't be able to. JavaScript should only tell the C++ to ask the user to load an extension - not which extension to load.

Comment 6 by, Mar 11 2012

Labels: -Pri-2 Pri-1

Comment 8 by, Mar 12 2012

My heart skipped a beat or three when I saw me being cc-ed on this bug as I immediately feared I'd been the cause of some recent ruckus. Digging through svn, however, showed this problem already existed when I converted the page. However, that still leaves me with the question as to the intent of the cc'ing. I'd be happy to put other things aside and work on this as top priority if that is what you intended (I'm asking because I don't want to needlessly duplicate effort if someone is already working on this)?
I cc'd you because I figured that it was possible that this was code that you had changed, and if not that you were probably still the person who knew it best.  Aaron, is there someone else who can take this?

Comment 10 by, Mar 12 2012

Labels: Feature-Options
Erik: See comment 3. I asked the options guys if they wanted to take this because they have increasingly been doing more work on chrome://extensions/ and it is a natural part of their domain. I also think it is important that they understand the issues here.

If they couldn't do it for some reason, my next suggestion would have been finnur.

Comment 11 by, Mar 13 2012

Labels: SecImpacts-Stable SecImpacts-Beta Mstone-18 SecSeverity-High
csilv, jhawkins - are either of you able to handle this one?  We have a window where we could get a patch into M18, and security is asking if we can get this wrapped up quickly.
Status: Started
I thought we assumed that WebUI couldn't be XSS'd? It seems there are probably a ton of holes in webui if we assume an attacker can take control of the page. (At least one hole that allowed this is currently being patched by creis.)

If we assume an attacker can take control of a page, and therefore harden all of webui, then we may as well let extensions run on chrome:// pages, no?
This is a defensive measure to prevent trivial system-level code execution from WebUI pages.

We're also adding defensive measures to further separate web origins from webui origins, etc.

Comment 17 by, Mar 14 2012

I'm not sure what the traditional take is on whether chrome:// pages are xss'able, but I think the consensus is currently leaning toward "yes".

There have been a few recent attacks here, so I think we should write these bindings as narrow as possible.

Comment 18 by, Mar 14 2012

Also, the power exposed by some of these bindings is so great (run any native code), that we should be extra extra careful, even if we really believe it is impossible to xss chrome://extensions/.
The CL above says that it's going to be checked in, but isn't marked closed.  What's the status?

Comment 20 by, Mar 20 2012

I went ahead and flippped the commit checkbox.  Hopefully it can go in without a merge conflict.

Comment 21 by, Mar 20 2012

CL is out of date, it'll have to be rebased.

Comment 22 by, Mar 20 2012

Is it going to make it for Chrome 19? Kinda important.

Comment 23 by, Mar 20 2012

Project Member

Comment 24 by, Mar 25 2012

The following revision refers to this bug:

r128801 | | Sat Mar 24 18:49:01 PDT 2012

Changed paths:

Extensions: Don't pass the selected unpacked extension path from JS to C++

This is an unecessary hoop to jump through since the C++ is the original
callback of the file dialog.

BUG= 117715 

Review URL:
So this needs to be merged to M18 branch?
Labels: -Restrict-View-SecurityTeam -Mstone-18 -SecSeverity-High Restrict-View-SecurityNotify Mstone-19 SecSeverity-Low
Status: FixUnreleased
I think M18 has another change (relative to M17) that should be a good mitigation: all extension installs go through an "accept?" native dialog. This was missing in M17 for "install unpacked extension".
Assuming the above is true, we can IMHO just let this roll into M19 ?
That works for me; the merge would be a pain.
@jorjelo: can you confirm that M18 is ok so we'll all done?
The exact mitigation in 18 is that we are prompting for unpacked extensions that include an NPAPI plugin - we weren't doing that in 17. This is the relevant snippet:

  const ExtensionSet* disabled_extensions =
  if (service_weak_->show_extensions_prompts() &&
      prompt_for_plugins_ &&
      !extension->plugins().empty() &&
      !disabled_extensions->Contains(extension->id())) {
    SimpleExtensionLoadPrompt* prompt = new SimpleExtensionLoadPrompt(
    return;  // continues in SimpleExtensionLoadPrompt::InstallUI*

|show_extensions_prompt()| is always true, as is |prompt_for_plugins_|. Therefore, we will always prompt if the extension has plugins and is not disabled. This is not ideal, since we won't prompt for extensions without plugins. Is it enough together with the rest of the things that went into 18?
Labels: -Restrict-View-SecurityNotify
Status: Verified
Project Member

Comment 31 by, 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.
Labels: -Feature-Options Feature-Settings
Project Member

Comment 33 by, Mar 10 2013

Labels: -Area-Internals -Type-Security -Feature-Extensions -SecImpacts-Stable -SecImpacts-Beta -Mstone-19 -SecSeverity-Low -Feature-Settings Security-Severity-Low M-19 Cr-Platform-Extensions Cr-UI-Settings Security-Impact-Stable Security-Impact-Beta Cr-Internals Type-Bug-Security
Project Member

Comment 34 by, Mar 14 2013

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

Comment 35 by, Mar 21 2013

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

Comment 36 by, Mar 21 2013

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

Comment 37 by, Mar 21 2013

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

Comment 38 by, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 39 by, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member

Comment 40 by, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment