1658214 - (CVE-2020-15664) InstallTrigger can take the principal from the wrong inner window when initialized

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Kaizer Soze]

Attachment: inheritPermissions-testcase-new.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

I created a form, that first loads about:blank into a full new window. I then used the onsubmit handler to load https://fpn.firefox.com/ into this window which has the extension install permission, while holding a reference to the eval function for that window. Upon submitting the form I have scripted an install of ghostery.

Actual results:

The secondary prompt still displays, which asks a user for permissions to be allowed. Currently this stops a full install of a webextension, but allows for one to attempt more exploitation by manipulating input into Extensions.jsm.

Expected results:

Holding a reference to the eval function of a window should not allow one to inherit permissions of the window that is loaded. This allows all permissions to be inherited but proper defense in depth stops things like accessing video/microphone streams.

Storage APIs may still be able to be manipulated, as I have not had time to fully test this. This could allow one to pollute the local storage of extensions, and persistent webworker pages.

Comment 1

[Kaizer Soze]

Hey guys, I haven't had time to fully follow up on the storage API permissions and such yet. I will continue to work on this as a fix is put in place. For any additional info please NI me.

Also, can I get this nominated for a sec-bounty? I know back in the day we would have made this a moderate but things have been changing. The potential for exploiting code in Extensions.jsm is high but my time is low.

I happened to mention a bug to Bobby a couple of days ago, I'll link him here.

Comment 2

[Bobby Holley (:bholley)]

Kris, do you have cycles to have a look here?

Comment 3

[Bobby Holley (:bholley)]

I haven't actually debugged the testcase. But the way this is supposed to work is: Synchronously-created windows (iframes, and the return value of window.open) have an initial about:blank document that is same-origin with the creator. Upon navigation, we reuse/persist the global of that about:blank document (and thus globals like eval) of the initial about:blank window if the navigated-to document is same-origin.

So in general, the holdEval trick used in the testcase should just be acquiring the eval of the initial about:blank window, which should be different from fpn.firefox.com, and thus safe from a same-origin-policy perspective. But it could be the case that the permission handling here is out-of-band somehow, and that the inactive inner window is doing permission checks via the active one (via the outer/docshell). If that were the case, that would be bad.

Comment 4

[Kaizer Soze]

I'll try to save you some time with the debugging. InstallTrigger is still only created upon request. Eval allows access to it as if it is the InstallTrigger Object of the about:blank page. InstallTrigger gets the principal it is working with from the window it is created for as it is accessed (lazy initialization). This makes the extension installation case a primary way to show this off.

As for further debugging, other permissions are inherited but only possibly useful case by case. Most seem to have proper defense in depth to prevent similar abuse. This eval trick was only to show off that permissions can get inherited and not meant to be a major point here. I know with so much attention being on webextensions right now, this was a good place to start.

As for further exploitation, I assume all the code in Extensions.jsm isn't supposed to be web facing. I have done practically no work on the storage issues this could lead to, as there's just no time for me to right now.

If I'm interrupting here and you guys want to just roll with this, that's fine with me. My debugging skills definitely aren't as sharp as most.

Comment 5

[Bobby Holley (:bholley)]

Looks like InstallTrigger is JS-implemented WebIDL [1]. Implementing in JS makes it much easier to succumb to inner/outer window confusion, wonder if that's what's happening here.

(To be clear I'm just speculating here since I don't have cycles to actually dig in. Hopefully Kris does.)

[1] https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/dom/webidl/InstallTrigger.webidl#23

Comment 6

[Kris Maglione [:kmag]]

Yeah, it's a case of inner window confusion. The InstallTrigger's init method takes a window object, and saves the principal from its current document, when it's first initialized. If that happens after the window has navigated, then it winds up taking it from whatever the current document is.

We should probably just forbid initializing any JS-implemented WebIDL globals when their inner window isn't active.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1658214: Only construct JS-implemented objects if inner window is current. r=bholley

Comment 8

[Kaizer Soze]

Hey guys, could we get a sec-rating here? Thanks

Comment 9

[Andrew McCreight [:mccr8]]

This should get a rating before it lands, as that will affect sec-approval. Maybe Dan has some thoughts on a rating.

Comment 10

[Daniel Veditz [:dveditz]]

As-is the testcase is a super bad spoof: wouldn't be too hard to imagine a "click here to get uBlock Origin from addons.mozilla.org" link, that when clicked opens AMO to the legit uBlock Origin page, and then uses this trick to prompt to install something else with a similar name. Or same name even? not sure the automatic scans require names to be unique. That's not quite sec-high because that powerful spoofing scenario is limited to a single site and action -- but it's a super powerful action!

There are two other objects created using the function that's being patched (that I found -- are there more?). One was the sidebar/add search provider function, but that's a deprecated stub and not useful. The other was the AddonManager. Looking at the IsValidHost logic it ought to be available on AMO but I couldn't get it to work. If you can it would be super powerful. AddonManager.getAddonByID() alone is fantastically useful. You could fingerprint people based on what they have installed. It looks like the "SyncGUID" is a per-install generated thing, if the victim has Sync. If the addon has an icon the URL gives me the full file path to the extension in my profile -- so that gets leaked if someone has a "load known local file" exploit. And if you could make this work and you didn't like adblockers you could have fun with this:

1. Install uBlock Origin if you don't have it already
2. open about:preferences (need it privileged since I couldn't get AMO to work)
3. open the DevTools console
4. execute (await AddonManager.getAddonByID("[e-mail]")).disable()

If you want to play and see what other data is available do await AddonManager.getAddonByID("[e-mail]")

.uninstall() also works (more realistic attack) but harder to undo as a test! change disable() to enable() to get your addon back (or manually in about:addons).

Like I said, I couldn't make that actually work on AMO, but maybe I'm missing something. It's certainly a juicy enough target to be worth playing with.

Testcase works on Firefox ESR-68 (as expected) so we'll need a patch for that, too. The "fpn" site wasn't in the whitelist back then so you have to either manually add the site as an ALLOW exception first, or modify the testcase to use AMO which was in the built-in exception list.

Comment 11

[Kris Maglione [:kmag]]

The AddonManager API isn't vulnerable to this issue. The decision on whether or not to make that available happens in C++ via AddonManagerWebAPI::IsAPIEnabled, which isn't susceptible to inner window confusion.

Comment 12

[Julien Cristau [:jcristau]]

We should land this soon to make 80. Kris, can you request sec-approval?

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 9169274 [details]
Bug 1658214: Only construct JS-implemented objects if inner window is current. r=bholley

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Fairly easily, given the small number of APIs constructed this way, but the actual risk of the exploit is relatively low.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should already apply cleanly to all branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It only affects constructing objects for 3 rarely-used APIs when a window has been navigated away from, which no valid code should be relying on.

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9169274 [details]
Bug 1658214: Only construct JS-implemented objects if inner window is current. r=bholley

Land it and let's get it uplifted too.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/1edd9346c110b011ed87e50eb3d417202ea445fb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9169274 [details]
Bug 1658214: Only construct JS-implemented objects if inner window is current. r=bholley

Also approved for 80.0rc1, 78.2esr, and 68.12esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6fa858e9b31d
https://hg.mozilla.org/releases/mozilla-esr78/rev/bbbd6d1ea0f5
https://hg.mozilla.org/releases/mozilla-esr68/rev/4bdb757c65b2

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1edd9346c110

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 20

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Here's an attempt at an advisory, LMK if anyone thinks it can be worded better.