1320039 - (CVE-2016-9902) Pocket extension unnecessarily exposes its messaging interface to web pages

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Wladimir Palant]

Attachment: Proof of concept webpage

Note: I am reporting this issue here rather than to Pocket because Pocket is an integral part of Firefox and cannot be disabled or removed by regular means.

The bubble displayed by the Pocket toolbar button is actually an <iframe> loading from either about:pocket-signup or about:pocket-saved. In order to communicate with that frame the Pocket extension registers event listeners on browser.xul's document, accepting untrusted events. The event listeners don't do anything to verify the origin of the event so that events bubbling up from the browser's content area will be processed as well.

Steps to reproduce (I tested with Firefox 53.0a1 nightly 2016-11-23 on macOS 10.12):

1. Make sure that E10S is disabled (with E10S events from content won't bubble up to the browser).
2. Click the Pocket button to make sure that the extension is initialized. It doesn't matter whether anything happens after that, you don't need to sign up with Pocket.
3. Now open the attached webpage in Firefox and click the "Open about:preferences with Sync tab selected".

This button will open about:preferences#sync page, something that web pages normally cannot do. Pop-up blocker doesn't affect this, so websites can open any number of pop-ups at any time this way. Security checks don't apply either so opening arbitrary file:///, chrome:// or about: URLs is possible.

Note that this only abuses "openTabWithUrl" message that Pocket listens to. There is a number of other message listeners which might also have abuse potential - e.g. "deleteItem" message allows deleting items from Pocket. Note also that the website can detect when its attack was successful because the Pocket extension will remove the event target from the page.

Also, this issue is completely avoidable. There is only one Pocket frame in the browser document, the event listeners could be registered directly on that iframe element rather than on the document - this would make sure that no events from other sources will be received.

Comment 1

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #0)
> Created attachment 8814034 [details]
> Proof of concept webpage
> 
> Note: I am reporting this issue here rather than to Pocket because Pocket is
> an integral part of Firefox and cannot be disabled or removed by regular
> means.

Eh, fairly sure that you can just right click the pocket button, click 'remove from toolbar', and that should stop all of this (if not immediately, certainly on the next restart). Also, most of this code was still written by pocket...

Shane, can you make sure we CC whoever needs to be CC'd from the pocket side on this?

> The bubble displayed by the Pocket toolbar button is actually an <iframe>
> loading from either about:pocket-signup or about:pocket-saved. In order to
> communicate with that frame the Pocket extension registers event listeners
> on browser.xul's document, accepting untrusted events. The event listeners
> don't do anything to verify the origin of the event so that events bubbling
> up from the browser's content area will be processed as well.

Ugggh.

Comment 2

[Wladimir Palant]

Well, if you never click the Pocket button it will never initialize and my PoC won't work. But if you click it, and be it out of curiosity - removing it from the toolbar will no longer help you, for the current session you will be vulnerable.

Comment 3

[:Gijs (he/him)]

Attachment: Patch v0.1

This time, actual origin principal checks against about:pocket-signup/saved *and* restricting the message listeners to the iframe. Mike, can you check that this is sufficient and that I haven't broken anything (looking briefly, it seems OK to me, but...).

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> Created attachment 8814048 [details] [diff] [review]
> Patch v0.1
> 
> This time, actual origin principal checks against about:pocket-signup/saved
> *and* restricting the message listeners to the iframe. Mike, can you check
> that this is sufficient and that I haven't broken anything (looking briefly,
> it seems OK to me, but...).

I'm assuming I don't need to touch messages.js because it only runs *inside* the iframes for messages/events going in the other direction. That said, I wonder if you could frame those documents using their chrome URIs and then manipulate them that way :| .

Comment 5

[Wladimir Palant]

(In reply to :Gijs Kruitbosch from comment #4)
> That said, I
> wonder if you could frame those documents using their chrome URIs and then
> manipulate them that way :| .

Given bug 1320057 it's probably a good thing that they are accessed via about: URIs that drop all privileges.

Note that changing removeMessageListener() is pointless - it is dead code not being called anywhere and it wouldn't work anyway (it removes the wrong function, not the closure added in addMessageListener). There is also quite a bit of unnecessary complexity in the surrounding code but that's irrelevant for this security issue.

Comment 6

[:Gijs (he/him)]

Attachment: Patch v0.2

We might as well make it impossible for it to open URLs it couldn't otherwise open itself in its own iframe, for defense in depth purposes.

Comment 7

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > That said, I
> > wonder if you could frame those documents using their chrome URIs and then
> > manipulate them that way :| .
> 
> Given bug 1320057 it's probably a good thing that they are accessed via
> about: URIs that drop all privileges.

They are in the iframe we use for the actual UI. But the pages have chrome:// URLs, e.g. chrome://pocket/content/panels/saved.html . They're contentaccessible too, presumably in order for the about: pages to be able to load all the other gunk. That still doesn't let you link to them, fortunately, and AIUI this vulnerability doesn't let you open URLs into arbitrary subframes - "just" in new tabs, which at least avoids then being able to spoof the messages back to these pages (like, say, the suggested tags) which you could then exploit to go other places in the now-system-privileged page you just opened. :|

Comment 8

[:Gijs (he/him)]

(though also, I think/hope that we don't allow system-privileged frames inside content-privileged documents)

Comment 9

[:Gijs (he/him)]

Attachment: Patch v0.3

If it's unused, let's just take it out, then.

Comment 10

[Wladimir Palant]

(In reply to :Gijs Kruitbosch from comment #8)
> (though also, I think/hope that we don't allow system-privileged frames
> inside content-privileged documents)

I'm pretty sure that this is possible as long as a system-privileged script sets location.href.

Comment 11

[Wladimir Palant]

Comment on attachment 8814076 [details] [diff] [review]
Patch v0.3

Review of attachment 8814076 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pocket/content/main.js
@@ +364,5 @@
>          // Open a new tab with a given url and activate if
>          var _openTabWithUrlMessageId = "openTabWithUrl";
> +        pktUIMessaging.addMessageListener(iframe, _openTabWithUrlMessageId, function(panelId, data, contentPrincipal) {
> +            try {
> +              urlSecurityCheck(data.url, contentPrincipal, Services.scriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);

Given that this is technically an extension and that the code is probably upstreamed to Pocket, is it a good idea to rely on implementation details like the global urlSecurityCheck function? Besides, just calling scriptSecurityManager directly wouldn't be significantly longer:

  var secMan = Services.scriptSecurityManager;
  secMan.checkLoadURIStrWithPrincipal(data.url, contentPrincipal, secMan.DISALLOW_INHERIT_PRINCIPAL);

Comment 12

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > (though also, I think/hope that we don't allow system-privileged frames
> > inside content-privileged documents)
> 
> I'm pretty sure that this is possible as long as a system-privileged script
> sets location.href.

Hrmpf. Will file an orthogonal bug about that.

Comment 13

[:Gijs (he/him)]

(In reply to Wladimir Palant from comment #11)
> Comment on attachment 8814076 [details] [diff] [review]
> Patch v0.3
> 
> Review of attachment 8814076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/pocket/content/main.js
> @@ +364,5 @@
> >          // Open a new tab with a given url and activate if
> >          var _openTabWithUrlMessageId = "openTabWithUrl";
> > +        pktUIMessaging.addMessageListener(iframe, _openTabWithUrlMessageId, function(panelId, data, contentPrincipal) {
> > +            try {
> > +              urlSecurityCheck(data.url, contentPrincipal, Services.scriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> 
> Given that this is technically an extension and that the code is probably
> upstreamed to Pocket, is it a good idea to rely on implementation details
> like the global urlSecurityCheck function?

It also relies on openUILink / openLinkIn, I think? Which is in the same file. So I don't think that relying on this as well is fine.

Comment 14

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #13)
> It also relies on openUILink / openLinkIn, I think? Which is in the same
> file. So I don't think that relying on this as well is fine.

Eh, too many negations. But yeah, I think this is fine, tbh.

Comment 15

[Wladimir Palant]

(In reply to :Gijs Kruitbosch from comment #13)
> It also relies on openUILink / openLinkIn, I think? Which is in the same
> file. So I don't think that relying on this as well is fine.

Not actually the same thing - it's in the same file now because it moved there. This is documented API (https://developer.mozilla.org/de/docs/Codeschnipsel/Tabbed_browser#Opening_a_URL_in_the_correct_window.2Ftab) and has been stable for a long time. Quite a few extensions rely on it already which is probably why it is still around in that form. urlSecurityCheck() is neither old nor documented.

Comment 16

[Kris Maglione [:kmag]]

It's documented to the extent that someone wrote a code snippet that uses it, but that says nothing about it being supported, or its use being good practice. In fact, the en-US version of that page was marked as obsolete because it suggested a lot of bad practices.

I don't think this is worth worrying about. checkLoadURIStrWithPrincipal is no more of a public API than urlSecurityCheck, and it would be much easier to make changes to the latter (which is already used by many add-ons) without breaking compatibility than it would the former.

Comment 17

[Wladimir Palant]

Comment on attachment 8814076 [details] [diff] [review]
Patch v0.3

Review of attachment 8814076 [details] [diff] [review]:
-----------------------------------------------------------------

Well, if everybody agrees that this function is safe to use from an extension...

Comment 18

[Mike Kaply [:mkaply]]

Comment on attachment 8814076 [details] [diff] [review]
Patch v0.3

Other folks have reviewed.

Comment 19

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12c2619098c3

Comment 20

[:Gijs (he/him)]

This was merged yesterday but somehow the merge didn't comment here:

https://hg.mozilla.org/mozilla-central/rev/12c2619098c3

Comment 21

[Shane Caraveo (:mixedpuppy)]

This and bug 1320057 for 50.1?

Comment 22

[:Gijs (he/him)]

Comment on attachment 8814076 [details] [diff] [review]
Patch v0.3

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate, obvious html injection issue that might be remotely exploitable, trivial patch. I would be more sanguine about not having this on ESR if we weren't about to roll it into 50.1 giving adversaries 4 months to figure out how to exploit this together with 1320057 and other addon bugs into something higher severity (before it'll ship as part of 52esr)
User impact if declined: see above
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): pretty low. The changes are straightforward. We should make sure they get tested on release/beta/aurora, of course.

Approval Request Comment
[Is this code covered by automated tests?]: Some basic automated tests cover stuff this code changes, but not in any great deapth, sadly.
[Has the fix been verified in Nightly?]: not yet...
[Needs manual test from QE? If yes, steps to reproduce]:

from comment #1:

> Steps to reproduce (I tested with Firefox 53.0a1 nightly 2016-11-23 on macOS
> 10.12):
> 
> 1. Make sure that E10S is disabled (with E10S events from content won't
> bubble up to the browser).
> 2. Click the Pocket button to make sure that the extension is initialized.
> It doesn't matter whether anything happens after that, you don't need to
> sign up with Pocket.
> 3. Now open the attached webpage in Firefox and click the "Open
> about:preferences with Sync tab selected".
> 
> This button will open about:preferences#sync page, something that web pages
> normally cannot do.

In builds without the patch, the page will open. In builds with the patch, it shouldn't open.

The patch changes how some of the pocket internals function. The basic verification steps would amount to checking:

a) you can still successfully save pages in pocket
b) you can still tag them as you like
c) you can still remove pages once saved in pocket, as you would before.

[List of other uplifts needed for the feature/fix]: bug 1320057 is not necessary for this fix to work, but because both affect pocket and how easy it is to run code in its code, I would suggest both need uplift.
[Is the change risky?]: not very
[Why is the change risky/not risky?]: well, the worst that could happen is that we break pocket's tagging system. Manual tests by me suggests this patch doesn't do that. It's also pretty straightforward.

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8814076 [details] [diff] [review]
Patch v0.3

Fix a sec-moderate related to pocket. Beta51+ and Auror52+. Should be in 51 beta 6.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1681c600fd1

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/51a5f11b6d66

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8814076 [details] [diff] [review]
Patch v0.3

This meets the 50.1.0 bar, approving for esr45.6 as well.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

I don't think Pocket shipped with ESR45 (I don't see it in the extensions code)

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: esr45 patch

Comment 29

[Ryan VanderMeulen [:RyanVM]]

ESR45 is affected after all.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/7e87ed92ad46911d4668e60ea02055c196ad75e5
https://hg.mozilla.org/releases/mozilla-esr45/rev/21c615b650488bac52afd15e55bd201ce53ac75e

Comment 31

[Paul Silaghi, QA [:pauly]]

We verified this along with some spot checks on the affected areas on the following builds:
Fx 50.1.0 tinderbox, 45.5.2 ESR tinderbox, 51b6, 52.0a2 (2016-12-05), 53.0a1 (2016-12-06).
More details here: https://public.etherpad-mozilla.org/p/pocket-security-testing
No issues were found, marking as verified fixed.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

This needs to be added to the 50.1 Firefox advisory and ESR 45.5.2 (or 45.6 since I don't think there is a 45.5.2 ESR release) after it ships in ESR.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Ok. My mistake, there is a 45.6 release so this can be added once that ships.