Bug 1320057 (CVE-2016-9901)

Remote code execution vulnerability in Pocket extension

VERIFIED FIXED in Firefox 50

Status

()

Firefox
Pocket
VERIFIED FIXED
8 months ago
4 months ago

People

(Reporter: Wladimir Palant, Assigned: Gijs)

Tracking

({csectype-priv-escalation, sec-moderate})

Trunk
Firefox 53
csectype-priv-escalation, sec-moderate
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox50+ verified, firefox51+ verified, firefox52+ verified, firefox53+ verified, firefox-esr4550+ verified)

Details

(Whiteboard: [adv-main50.1+][adv-esr45.6+])

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
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.
(Reporter)

Comment 1

8 months ago
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.
(Assignee)

Comment 2

8 months ago
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.
Flags: needinfo?(nate)
Attachment #8814073 - Flags: review?(nate)
(Assignee)

Comment 3

8 months ago
(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

8 months ago
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.
Flags: needinfo?(nate)

Comment 5

8 months ago
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.
Attachment #8814073 - Flags: review?(nate) → review+
(Reporter)

Comment 6

8 months ago
(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

8 months ago
(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.
(Assignee)

Comment 8

8 months ago
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).
Attachment #8814073 - Flags: sec-approval?
(Assignee)

Updated

8 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Reporter)

Comment 9

8 months ago
(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?
Flags: sec-bounty?
Keywords: csectype-priv-escalation, sec-moderate
(Assignee)

Comment 10

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

Given that this is sec-moderate, I don't need sec-approval.
Attachment #8814073 - Flags: sec-approval?
(Assignee)

Comment 11

8 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/577914241a79
(Assignee)

Comment 12

8 months ago
https://hg.mozilla.org/mozilla-central/rev/577914241a79
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Comment 13

8 months ago
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?
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

8 months ago
status-firefox-esr45: --- → affected
(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?
Flags: needinfo?(mixedpuppy) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 15

8 months ago
(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.
Riu, Liz, if we can push this and bug 1308688 50.1 that would be great.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Correction to comment 16, bug 1320039 (another pocket bug)
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.
tracking-firefox50: --- → +
tracking-firefox51: --- → +
tracking-firefox52: --- → +
Flags: needinfo?(lhenry)
(Assignee)

Comment 19

8 months ago
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8814073 - Flags: approval-mozilla-release?
Attachment #8814073 - Flags: approval-mozilla-esr45?
Attachment #8814073 - Flags: approval-mozilla-beta?
Attachment #8814073 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

8 months ago
Flags: qe-verify?
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?
Flags: needinfo?(florin.mezei)
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.
Attachment #8814073 - Flags: approval-mozilla-beta?
Attachment #8814073 - Flags: approval-mozilla-beta+
Attachment #8814073 - Flags: approval-mozilla-aurora?
Attachment #8814073 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a985f25512a
status-firefox52: affected → fixed
tracking-firefox53: --- → ?
tracking-firefox-esr45: --- → ?
https://hg.mozilla.org/releases/mozilla-beta/rev/a8f1569d056b
status-firefox51: affected → fixed

Updated

8 months ago
tracking-firefox53: ? → +
tracking-firefox-esr45: ? → 51+
Flags: needinfo?(rkothari)

Comment 24

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

This meets the bar for 50.1, also approving for esr45.
Attachment #8814073 - Flags: approval-mozilla-release?
Attachment #8814073 - Flags: approval-mozilla-release+
Attachment #8814073 - Flags: approval-mozilla-esr45?
Attachment #8814073 - Flags: approval-mozilla-esr45+
I don't think pocket ships with ESR, https://hg.mozilla.org/releases/mozilla-esr45/file/tip/extensions
Attachment #8814073 - Flags: approval-mozilla-esr45+ → approval-mozilla-esr45-
status-firefox-esr45: affected → unaffected
I'm not clear if 45 is affected or not, but 45 is pre-system addon so these patches would not apply as-is.
Created attachment 8816256 [details] [diff] [review]
esr45 patch

FWIW here is a patch against esr45

Comment 28

8 months ago
uplift
https://hg.mozilla.org/releases/mozilla-release/rev/90deea58afe14d1d6f95e64090cce26494583c9a
https://hg.mozilla.org/releases/mozilla-esr45/rev/c15e5afc0430f8d57fadf930d8cbc80d9743a53d
status-firefox50: affected → fixed
status-firefox-esr45: unaffected → fixed
tracking-firefox-esr45: 51+ → ?
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.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(florin.mezei)
(Assignee)

Comment 30

8 months ago
(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. :-)
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.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox52: fixed → verified
status-firefox53: fixed → verified
status-firefox-esr45: fixed → verified
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
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.
Flags: needinfo?(abillings)
Ok. We have an ESR45.6 going out with 50.1. This advisory should come out with both releases.
Flags: needinfo?(abillings)
Whiteboard: [adv-main50.1+][adv-esr45.6+]
Alias: CVE-2016-9901
Group: firefox-core-security → core-security-release
tracking-firefox-esr45: ? → 50+
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.