1320057 - (CVE-2016-9901) Remote code execution vulnerability in Pocket extension

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Wladimir Palant]

The following line will process tags received from a remote server (getpocket.com) as HTML code without doing any sanitizing:

https://hg.mozilla.org/mozilla-central/file/34fce7c12173bdd6dda54c2ebf6d344252f1ac48/browser/extensions/pocket/content/panels/js/saved.js#l33

If this server is compromised it will be able to run JavaScript code in the context of the about:pocket-saved page whenever the user saves something to Pocket. This page isn't privileged and it is properly isolated in <iframe type="content">. However, at the very least it has access to the Pocket extension's messaging API already outlined in bug 1320039 - and unlike with the proof-of-concept there the communication can go both ways here.

Comment 1

[Wladimir Palant]

It appears that this vulnerability is already known (see bug 1172226 and bug 1236755). Why it was "fixed" by dropping chrome privileges but the actual bug was left alone is beyond me. Code injection into extension context is a rejection reason on AMO at least, even if the context in question is a content script - the injected code can communicate with the extension backend, and this is often enough to do significant damage.

Comment 2

[:Gijs (he/him)]

Attachment: Patch v0.1

This is the fix Jared provided in bug 1172226, but that never seems to have made it in, which looks sane to me.

Furthermore, I found a similar issue with how we process suggested tags, so I fixed that too.

Nate, can you please check that this patch looks OK, and that your APIs produce text rather than htmlentity'd stuff, which is something you raised questions about in bug 1172226? There's already other code that relies on this, e.g. https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/content/panels/js/saved.js#217 and https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pocket/content/panels/js/saved.js#264 use tag.text() and so will get plaintext rather than html-escaped things.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> Created attachment 8814073 [details] [diff] [review]
> Patch v0.1
> 
> This is the fix Jared provided in bug 1172226, but that never seems to have
> made it in, which looks sane to me.
> 
> Furthermore, I found a similar issue with how we process suggested tags, so
> I fixed that too.

Heh, which is actually the line Wladimir cited in comment #0, which I didn't check - I've just been looking at the bugs already on file based on comment #1 :-\

Comment 4

[Kris Maglione [:kmag]]

It would be nice if we could also patch the version of jQuery that Pocket is using so that it strips inline scripts, and ideally also turn on CSP for those frames.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

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

Stealing, since I was looking into this anyway.

Comment 6

[Wladimir Palant]

(In reply to Kris Maglione [:kmag] from comment #4)
> It would be nice if we could also patch the version of jQuery that Pocket is
> using so that it strips inline scripts

How would that work? It would have to be a full-fetched HTML sanitizer, one that will recognize <iframe onload="..."> or <a href="javascript:...">.

Before somebody makes that effort, it should be way simpler to get rid of jQuery altogether - most of it is trivially replaced by querySelector() here, and the remaining code isn't hard to convert to safe DOM methods either.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Wladimir Palant from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #4)
> > It would be nice if we could also patch the version of jQuery that Pocket is
> > using so that it strips inline scripts
> 
> How would that work? It would have to be a full-fetched HTML sanitizer, one
> that will recognize <iframe onload="..."> or <a href="javascript:...">.

It would, but fortunately we already have such a sanitizer built into Firefox. It would take a bit of effort to expose it to unprivileged Pocket frames, but not much.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No comments, but if you look at this diff it's pretty blindingly obvious

Which older supported branches are affected by this flaw?
everything

If not all supported branches, which bug introduced the flaw?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
trivial. Also because this is a go faster add-on, so we should be able to update it out of band, I think? Mixedpuppy and mkaply would know better than me.

How likely is this patch to cause regressions; how much testing does it need?
very unlikely. Should do some sanity testing that we're not breaking pocket outright, I guess, esp. when combined with bug 1320039.

Note that this and bug 1320039 don't have sec ratings. I'm uncomfortable landing them without a plan about how we intend this to hit branches and release. While we don't currently know of a way that this would lead to RCE at a chrome level, it's definitely... iffy. I think combined with the stuff in bug 1320039 this might be sec-moderate, but maybe I'm underestimating the severity. Reasons it's not sec-high is that, as far as I can tell, you can't actually do much inside this frame with an XSS vector, and likewise you can't do too much with the newly opened tabs to arbitrary URLs (e.g. you can open arbitrary file:/// URLs but you have no access to the resulting window so there's no disclosure of contents or even existence).

Comment 9

[Wladimir Palant]

(In reply to :Gijs Kruitbosch from comment #8)
> as far as I can tell, you can't
> actually do much inside this frame with an XSS vector, and likewise you
> can't do too much with the newly opened tabs to arbitrary URLs (e.g. you can
> open arbitrary file:/// URLs but you have no access to the resulting window
> so there's no disclosure of contents or even existence).

There is also getCurrentUrl so after opening a page you can check its URL (this requires bidirectional communication so not in bug 1320039). If a page changes its URL after loading (either by setting location.hash or by redirecting) this will leak information about the user. At the very least, on many websites you can use this to detect whether the user is logged in. But I can imagine that some pages will leak session identifiers or other information - e.g. what if one calls an OAuth endpoint this way and retrieves the final URL?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

Given that this is sec-moderate, I don't need sec-approval.

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/577914241a79

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/577914241a79

Comment 13

[:Gijs (he/him)]

Shane, what's the quickest/best way to get this and the other fix in bug 1320039 "go faster"'d to release? I'm guessing we need to start by merging both of these into the github repo - but then what?

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #13)
> Shane, what's the quickest/best way to get this and the other fix in bug
> 1320039 "go faster"'d to release? I'm guessing we need to start by merging
> both of these into the github repo - but then what?

We need to uplift all the way to beta first, then we can initiate a request to gofaster.  However, is the severity on this enough to push out a system addon update just two weeks prior to 50.1?  Could this just land in that instead?

Comment 15

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Shane, what's the quickest/best way to get this and the other fix in bug
> > 1320039 "go faster"'d to release? I'm guessing we need to start by merging
> > both of these into the github repo - but then what?
> 
> We need to uplift all the way to beta first, then we can initiate a request
> to gofaster.  However, is the severity on this enough to push out a system
> addon update just two weeks prior to 50.1?  Could this just land in that
> instead?

Also fine by me, but then it needs release approval. I'll do uplift requests tomorrow.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Riu, Liz, if we can push this and bug 1308688 50.1 that would be great.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Correction to comment 16, bug 1320039 (another pocket bug)

Comment 18

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

This should be able to make it into 50.1 (less than 2 weeks away.)  Ritu, what do you think? I'd rather we do that, than try to ship a system add-on this late in the release cycle.  Tracking 50/51/52 to make sure we uplift.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

[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 1320039 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 pretty trivial. We should make sure they get tested on release/beta/aurora, of course.
String or UUID changes made by this patch: no.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: see above.
[User impact if declined]: see above.
[Is this code covered by automated tests?]: there aren't really thorough automated tests for pocket, unfortunately. There are some basic tests that indicate we haven't broken the world.
[Has the fix been verified in Nightly?]: not yet.
[Needs manual test from QE? If yes, steps to reproduce]: 
1. open nightly and sign into pocket
2. open about:config
3. set the pref "extensions.pocket.settings.tags" to

 ["a<img src='z' onerror='alert(\"hello\")'>"]

(the [ and " and so on are *all* important - just copy-paste)

4. save a page through pocket
5. in the tags field, type 'a' (without quotes)

Expected: no alert

on builds without the fix: alert shows up.

Otherwise, for verifying we haven't broken anything, check that after reverting that pref in about:config you can normally use tags in pocket and suggestions continue to work.

[List of other uplifts needed for the feature/fix]: bug 1320039 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 very small and straightforward.
[String changes made/needed]: nope

Comment 20

[Gerry Chang [:gchang]]

Hi Florin, 
Can you help find someone to verify this and see if you can run some pocket regression tests based on changes of bug 1320039?

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

Fix a vulnerability in Pocket extension. Beta51+ and Aurora52+. Should be in 51 beta 6.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a985f25512a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8f1569d056b

Comment 24

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

Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

This meets the bar for 50.1, also approving for esr45.

Comment 25

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

I don't think pocket ships with ESR, https://hg.mozilla.org/releases/mozilla-esr45/file/tip/extensions

Comment 26

[Shane Caraveo (:mixedpuppy)]

I'm not clear if 45 is affected or not, but 45 is pre-system addon so these patches would not apply as-is.

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: esr45 patch

FWIW here is a patch against esr45

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/90deea58afe14d1d6f95e64090cce26494583c9a
https://hg.mozilla.org/releases/mozilla-esr45/rev/c15e5afc0430f8d57fadf930d8cbc80d9743a53d

Comment 29

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Moving over to Paul to verify this and run some Pocket regression tests before releasing 50.1.0 and ESR 45.6.0. Paul see comment 19 and comment 20 for more details.

Comment 30

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> Created attachment 8816256 [details] [diff] [review]
> esr45 patch
> 
> FWIW here is a patch against esr45

I was out last night so I missed all this - thanks Shane! Need to buy you a drink in Hawaii. :-)

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. We have an ESR45.6 going out with 50.1. This advisory should come out with both releases.