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 117736 No permission prompt when loading unpacked extension with NPAPI plugin
Starred by 4 users Project Member Reported by erikkay@chromium.org, Mar 11 2012 Back to list
Status: Verified
Owner:
User never visited
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
As part of the recent PinkiePie Pwnium exploit, it appears that the attacker was able to gain access to the extensions management page and get it to load an unpacked extension with an NPAPI plugin (see also  bug 117715 ) without generating a prompt.  Looking at the code in UnpackedInstaller::OnLoaded, it looks like it should generate a prompt in all cases unless the extension is disabled.  It's unclear to me from reading the code if re-enabling the extension would still trigger the prompt, but my guess is that it doesn't.

 
Labels: Restrict-View-SecurityTeam
Owner: yoz@chromium.org
Yoyo, since you were looking at this recently, could you verify?
Comment 3 by aa@chromium.org, Mar 11 2012
Are you sure that this is still needed? With the current design, there is no way to install an extension that doesn't put up a native dialog. We have some bugs, but I don't think that means that we should have two dialogs, just in case one breaks.
Comment 4 by yoz@chromium.org, Mar 12 2012
I'm not sure if this is what you were getting at, but I found what appear to be some holes. Say we have version 1 of the extension that has no plugin, and version 2 has a plugin (modifying the manifest in the same location).

This triggers a permission warning:
1. Load unpacked (version 1) from directory
2. Edit the manifest to be version 2 and include plugin section
3. Load unpacked (version 2) from directory

This doesn't trigger a permission warning:
1. Load unpacked (version 1) from directory
2. Disable
3. Edit the manifest to be version 2 and include plugin section
4. Load unpacked (version 2) from directory (it's still disabled)
5. Re-enable

Strangely, this also doesn't trigger a permission warning:
1. Load unpacked (version 1) from directory
2. Edit the manifest to be version 2 and include plugin section
3. 'Reload' the extension from chrome://extensions

It seems to me that since the prompting behavior is in UnpackedInstaller, there's no way it can happen at enable time.
Comment 5 by aa@chromium.org, Mar 12 2012
> It seems to me that since the prompting behavior is in UnpackedInstaller, there's no way it can happen at enable time.

Yup. Really good point. Needs to be at load time. But then reload would be affected, and would need a special case.

This is the kind of complexity that worries me about this feature.
Comment 6 by cdn@chromium.org, Mar 13 2012
Cc: jorgelo@chromium.org
Labels: OS-All SecImpacts-Stable SecImpacts-Beta SecSeverity-High
Jorge is busily working on understanding the exploit and should be able to give you the exact details of how this is possible.
So, from my current understanding of the shellcode, the message used is "extensionSettingsLoad", which is handled by ExtensionSettingsHandler::HandleLoadMessage according to:

chromium/src/chrome/browser/ui/webui/options/extension_settings_handler.cc

The relevant code:

void ExtensionSettingsHandler::HandleLoadMessage(const ListValue* args) {
  FilePath::StringType string_path;
  CHECK_EQ(1U, args->GetSize()) << args->GetSize();
  CHECK(args->GetString(0, &string_path));
  extensions::UnpackedInstaller::Create(extension_service_)->
      Load(FilePath(string_path));
}

Which eventually should end up here:

void UnpackedInstaller::OnLoaded(
    const scoped_refptr<const Extension>& extension) {
  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
  if (!service_weak_.get())
    return;
  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*
  }

  PermissionsUpdater perms_updater(service_weak_->profile());
  perms_updater.GrantActivePermissions(extension);
  service_weak_->OnExtensionInstalled(extension,
                                      false,  // Not from web store.
                                      StringOrdinal());
}

The question now is why the code is not going into the if statement that presents the prompt. Erik mentions that service_weak_->show_extensions_prompts() and prompt_for_plugins_ should both be true, so the suspects are the other two clauses of the if guard.
An interesting (if maybe just semantic) issue that came up while discussing this with Erik is that the OnLoaded function seems to be called before things are *actually* loaded (why would it try to show a prompt if not), but the name clearly does not reflect that. The worry is that the name somehow is correct and there's stuff being loaded before the prompt is even shown.
So, this is what the code looks like in 17:

http://src.chromium.org/viewvc/chrome/branches/963/src/chrome/browser/extensions/unpacked_installer.cc?revision=113174&view=markup

void UnpackedInstaller::OnLoaded(
    const scoped_refptr<const Extension>& extension) {
  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
  if (!service_weak_.get())
    return;
  const ExtensionList* disabled_extensions =
      service_weak_->disabled_extensions();
  if (service_weak_->show_extensions_prompts() &&
      prompt_for_plugins_ &&
      !extension->plugins().empty() &&
      std::find(disabled_extensions->begin(),
                disabled_extensions->end(),
                extension) !=
      disabled_extensions->end()) {
    SimpleExtensionLoadPrompt* prompt = new SimpleExtensionLoadPrompt(
        service_weak_->profile(),
        service_weak_,
        extension);
    prompt->ShowPrompt();
    return;  // continues in SimpleExtensionLoadPrompt::InstallUI*
  }
  service_weak_->OnExtensionInstalled(extension,
                                      false,  // Not from web store.
                                      -1);
}

It's easy to see that the last check has the logic wrong: the prompt is being shown only if the extension is disabled.
BTW, this has been fixed in ToT.
Is it possible to merge a fix for this to 17?
Let's wait and see if M18 is canceled or not. Once we know that, we can merge to M17 or M18 as appropriate.
M18 is fixed, so the only vulnerable scenario is if we need to keep 17 stable for longer =).
@jorgelo: do you have reference to a CL that fixed this?

Also, this bug should probably be FixUnreleased, Mstone-18 if it is really fixed in 18 ?
First it was refactored from List to Set, but the bug remained:
http://src.chromium.org/viewvc/chrome?view=rev&revision=113233

(That's a revert of a revert though =( )

Then the check in the if statement was fixed:
http://src.chromium.org/viewvc/chrome?view=rev&revision=119135

Do we use FixUnreleased even if this is not fixed in M17 (963)?
Labels: -SecSeverity-High SecSeverity-Medium Merge-Approved
Status: FixUnreleased
Yeah, FixUnreleased and Merge-Approved to indicate we might want to merge it somewhere. I still have hope that M18 might be saved, though :) I'll set Merge-Approved just in case it isn't.

Did a unit test get added? The missing dialog has bitten us too many times :(

Jim and I discussed the need for a test to avoid regressing this. From my conversation with Erik I don't think there's one, but I'll find out. At least he mentioned how one would go about adding it.
A separate functional bug to add a test might make sense.
Comment 19 by aa@chromium.org, Mar 19 2012
In a separate thread, I think we agreed to remove this prompt completely, right? I think the changes to the chrome API have landed to make it more restrictive.
Remove what prompt? I've lost track a bit. Are we talking about avoiding double-prompting?
The invariant that must never be breached is: all extension installs, no matter how initiated, and whether they contain NPAPI or not, must be mediated by a browser dialog.
Aaron's right: the conclusion in that other thread (with Justin's ack) was:

"""
1) Once we have the reproduction, figure out what broke and how/why
2) Fix the webui API to be more strict (117715)
3) Remove NPAPI prompting code (We can repurpose 117736 for this)
"""

1) We now know what broke, and all bugs are on track to being fixed.
2) Aaron: Did the CL for 117715 land? The fix would remove the ability to specify the path from where to load an unpacked extension.
3) Maybe we do not want to remove prompting =P
Comment 22 by aa@chromium.org, Mar 19 2012
The CL is here: http://codereview.chromium.org/9703039/. It's looking close to landing.
Project Member Comment 23 by bugdroid1@chromium.org, Mar 21 2012
Labels: merge-merged-963
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=127868

------------------------------------------------------------------------
r127868 | cevans@chromium.org | Tue Mar 20 18:31:25 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/963/src/chrome/browser/extensions/unpacked_installer.cc?r1=127868&r2=127867&pathrev=127868

Merge a 1-liner fix to M17.

BUG= 117736 
Review URL: https://chromiumcodereview.appspot.com/9794013
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam -Merge-Approved Restrict-View-SecurityNotify
Labels: CVE-2011-3055
Labels: pwnium
Comment 27 by cdn@chromium.org, May 15 2012
Status: Fixed
Marking old security bugs Fixed..
Labels: -Restrict-View-SecurityNotify
Status: Verified
Project Member Comment 29 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.
Project Member Comment 30 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Type-Security -Area-Internals -Feature-Extensions -Mstone-17 -SecImpacts-Stable -SecImpacts-Beta -SecSeverity-Medium Cr-Platform-Extensions Security-Impact-Stable Security-Severity-Medium Cr-Internals Security-Impact-Beta Type-Bug-Security M-17
Project Member Comment 31 by bugdroid1@chromium.org, Mar 13 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member Comment 32 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 33 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member Comment 34 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member Comment 35 by sheriffbot@chromium.org, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 36 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 37 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