Last Comment Bug 1320057 - (CVE-2016-9901) Remote code execution vulnerability in Pocket extension
(CVE-2016-9901)
: Remote code execution vulnerability in Pocket extension
Status: VERIFIED FIXED
[adv-main50.1+][adv-esr45.6+]
: csectype-priv-escalation, sec-moderate
Product: Firefox
Classification: Client Software
Component: Pocket (show other bugs)
: Trunk
: Unspecified Unspecified
-- normal (vote)
: Firefox 53
Assigned To: :Gijs
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-24 03:51 PST by Wladimir Palant
Modified: 2017-02-09 08:02 PST (History)
19 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
+
verified
50+
verified


Attachments
Patch v0.1 (2.95 KB, patch)
2016-11-24 05:27 PST, :Gijs
kmaglione+bmo: review+
gchang: approval‑mozilla‑aurora+
gchang: approval‑mozilla‑beta+
rkothari: approval‑mozilla‑release+
lhenry: approval‑mozilla‑esr45-
Details | Diff | Splinter Review
esr45 patch (2.55 KB, patch)
2016-12-01 13:11 PST, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review

Description User image Wladimir Palant 2016-11-24 03:51:30 PST
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 User image Wladimir Palant 2016-11-24 04:34:19 PST
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 User image :Gijs 2016-11-24 05:27:43 PST
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.

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 User image :Gijs 2016-11-24 05:29:29 PST
(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 User image Kris Maglione [:kmag] 2016-11-24 08:45:13 PST
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 User image Kris Maglione [:kmag] 2016-11-24 09:07:30 PST
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 User image Wladimir Palant 2016-11-24 11:14:54 PST
(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 User image Kris Maglione [:kmag] 2016-11-24 11:46:23 PST
(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 User image :Gijs 2016-11-25 03:38:22 PST
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 User image Wladimir Palant 2016-11-25 04:47:20 PST
(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 User image :Gijs 2016-11-29 01:59:17 PST
Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

Given that this is sec-moderate, I don't need sec-approval.
Comment 13 User image :Gijs 2016-11-30 06:18:32 PST
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 User image Shane Caraveo (:mixedpuppy) 2016-11-30 14:30:47 PST
(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 User image :Gijs 2016-11-30 14:34:53 PST
(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 User image Shane Caraveo (:mixedpuppy) 2016-11-30 14:37:05 PST
Riu, Liz, if we can push this and bug 1308688 50.1 that would be great.
Comment 17 User image Shane Caraveo (:mixedpuppy) 2016-11-30 14:39:08 PST
Correction to comment 16, bug 1320039 (another pocket bug)
Comment 18 User image Liz Henry (:lizzard) (needinfo? me) 2016-11-30 17:11:08 PST
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 User image :Gijs 2016-12-01 03:04:30 PST
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 User image Gerry Chang [:gchang] 2016-12-01 05:27:34 PST
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 User image Gerry Chang [:gchang] 2016-12-01 05:39:33 PST
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 User image Ryan VanderMeulen [:RyanVM] 2016-12-01 07:33:01 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a985f25512a
Comment 23 User image Ryan VanderMeulen [:RyanVM] 2016-12-01 07:47:05 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/a8f1569d056b
Comment 24 User image Ritu Kothari (:ritu) 2016-12-01 11:53:27 PST
Comment on attachment 8814073 [details] [diff] [review]
Patch v0.1

This meets the bar for 50.1, also approving for esr45.
Comment 25 User image Liz Henry (:lizzard) (needinfo? me) 2016-12-01 12:18:46 PST
I don't think pocket ships with ESR, https://hg.mozilla.org/releases/mozilla-esr45/file/tip/extensions
Comment 26 User image Shane Caraveo (:mixedpuppy) 2016-12-01 12:22:13 PST
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 User image Shane Caraveo (:mixedpuppy) 2016-12-01 13:11:48 PST
Created attachment 8816256 [details] [diff] [review]
esr45 patch

FWIW here is a patch against esr45
Comment 29 User image Florin Mezei, QA (:FlorinMezei) 2016-12-02 02:20:41 PST
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 User image :Gijs 2016-12-02 02:49:08 PST
(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 User image Paul Silaghi, QA [:pauly] 2016-12-06 06:52:07 PST
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 User image Al Billings [:abillings] 2016-12-07 12:25:44 PST
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 User image Al Billings [:abillings] 2016-12-07 12:43:49 PST
Ok. We have an ESR45.6 going out with 50.1. This advisory should come out with both releases.

Note You need to log in before you can comment on or make changes to this bug.