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.
Starred by 4 users
Status: Verified
Owner:
Closed: Mar 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Restricted
  • 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 aa@chromium.org, Mar 11 2012 Back to list
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 aa@chromium.org, Mar 11 2012
Labels: -OS-Mac OS-All
Comment 2 by aa@chromium.org, Mar 11 2012
Cc: -est...@chromium.org
Labels: Restrict-View-SecurityTeam
Comment 3 by aa@chromium.org, Mar 11 2012
csilv, jhawkins: do you all want to handle this, or should we.
Cc: est...@chromium.org
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 aa@chromium.org, 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 aa@chromium.org, Mar 11 2012
Cc: erikkay@chromium.org
Cc: finnur@chromium.org
Labels: -Pri-2 Pri-1
Comment 8 by finnur@chromium.org, 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 aa@chromium.org, Mar 12 2012
Cc: csilv@chromium.org jhawkins@chromium.org
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 cdn@chromium.org, 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.
Owner: jhawkins@chromium.org
Status: Started
Cc: creis@chromium.org
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 aa@chromium.org, 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 aa@chromium.org, 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 csilv@chromium.org, Mar 20 2012
I went ahead and flippped the commit checkbox.  Hopefully it can go in without a merge conflict.
Comment 21 by csilv@chromium.org, Mar 20 2012
CL is out of date, it'll have to be rebased.
Comment 22 by aa@chromium.org, Mar 20 2012
Is it going to make it for Chrome 19? Kinda important.
Comment 23 by csilv@chromium.org, Mar 20 2012
Absolutely.
Project Member Comment 24 by bugdroid1@chromium.org, Mar 25 2012
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=128801

------------------------------------------------------------------------
r128801 | jhawkins@chromium.org | Sat Mar 24 18:49:01 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/extensions/extension_settings_handler.cc?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/options2/options_ui2.cc?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/extensions/pack_extension_overlay.js?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/select_file_dialog.h?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/extensions/extension_settings_handler.h?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/extensions/extensions.js?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/extensions/pack_extension_handler.cc?r1=128801&r2=128800&pathrev=128801
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/extensions/pack_extension_handler.h?r1=128801&r2=128800&pathrev=128801

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

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

BUG= 117715 
TEST=none
TBR=sky
R=csilv,aa


Review URL: http://codereview.chromium.org/9703039
------------------------------------------------------------------------
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.
Cc: jorgelo@chromium.org
@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 =
      service_weak_->disabled_extensions();
  if (service_weak_->show_extensions_prompts() &&
      prompt_for_plugins_ &&
      !extension->plugins().empty() &&
      !disabled_extensions->Contains(extension->id())) {
    SimpleExtensionLoadPrompt* prompt = new SimpleExtensionLoadPrompt(
        service_weak_->profile(),
        service_weak_,
        extension);
    prompt->ShowPrompt();
    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 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.
Labels: -Feature-Options Feature-Settings
Project Member Comment 33 by bugdroid1@chromium.org, 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 bugdroid1@chromium.org, Mar 14 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member Comment 35 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-Low Security_Severity-Low
Project Member Comment 36 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 37 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member Comment 38 by sheriffbot@chromium.org, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 39 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 40 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