Closed Bug 1172226 Opened 9 years ago Closed 9 years ago

Arbitrary code execution via pocket tags

Categories

(Firefox :: Pocket, defect)

41 Branch
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 41
Iteration:
41.3 - Jun 29
Tracking Status
firefox38 --- unaffected
firefox39 + fixed
firefox38.0.5 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: dchan, Assigned: Dolske)

Details

(Keywords: csectype-priv-escalation, sec-high)

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.81 Safari/537.36

Steps to reproduce:

Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0

1. Login to Pocket
2. Go to about:chrome
3. Edit the preferences
browser.pocket.settings.tags
to
["a<img src='z' onerror='alert(Components.stack)'>"]
4. Visit any site
5. Click on "Save to Pocket" icon
6. Type 'a' into the tags

The Pocket chrome code pulls a list of recently used tags from the Pocket API server. [1] Under normal circumstances, this data is well-formed. However this can be used as an attack vector if the endpoint is malicious or if the transport layer is compromised. 

[1] - http://mxr.mozilla.org/mozilla-central/source/browser/components/pocket/pktApi.js#512 getTags()


Actual results:

Alert popup showing Components.stack output


Expected results:

No code execution
Attached image alert (obsolete) —
Testing information to setup a malicious endpoint for server MALICIOUS

1. Use a TLS enabled server
2. Setup the server to respond to the following route
https://MALICIOUS/v3/firefox/save

with the following JSON data

{
"status": 1,
"tags": ["a<img src='z' onerror='alert(Components.stack)'>"]
}

3. Visit about:config and change the setting
browser.pocket.api

to

MALICIOUS

You don't need to specify a scheme. The endpoint is generated by
52     var pocketAPIhost = Services.prefs.getCharPref("browser.pocket.api"); 	// api.getpocket.com
54     var baseAPIUrl = "https://" + pocketAPIhost + "/v3";

4. Restart Firefox
5. Proceed with step 4 from comment#0
Component: Untriaged → Pocket
Ironically, I forgot to use the bounty form. Could someone set that flag please?
Pretty sure that this is sg:low at most -- the code involved here should be running as plain unprivileged content in the Pocket panel's iframe, and all XHR requests to Pocket are made over a TLS connection.
Flags: sec-bounty?
Assignee: nobody → jaws
Status: UNCONFIRMED → ASSIGNED
Iteration: --- → 41.3 - Jun 29
Points: --- → 2
Ever confirmed: true
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch Patch (obsolete) — Splinter Review
The core bug exists in the tokeninput library that is being used. It is constructing HTML fragments by concatenating raw text. I have fixed the issue in your version of tokeninput (which is not actually minimized though the filename says it is[1]).

I've also added a line to pktApi.js to filter specific characters from the server response.

Nate, do you have a way to get the tokeninput changes fixed upstream? There are a few other places within the tokeninput.min.js file that you may want to take a look at, specifically http://mxr.mozilla.org/mozilla-central/source/browser/components/pocket/panels/js/vendor/jquery.tokeninput.min.js?rev=77d92f6d7679#713 and maybe some other places.

[1] This was made not-minimized by https://bugzilla.mozilla.org/show_bug.cgi?id=1163111
Attachment #8616874 - Flags: review?(nate)
Attachment #8616874 - Flags: review?(dolske)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Created attachment 8616874 [details] [diff] [review]
> Patch
> 

There also appears to be some string concatenation in saved.js [1]

[1] - http://mxr.mozilla.org/mozilla-central/source/browser/components/pocket/panels/js/saved.js#31


(In reply to Justin Dolske [:Dolske] from comment #3)
> Pretty sure that this is sg:low at most -- the code involved here should be
> running as plain unprivileged content in the Pocket panel's iframe, and all
> XHR requests to Pocket are made over a TLS connection.

Unfortunately, it appears some part of the Pocket panel code runs with chrome since it can access Components.stack . I agree that the use of TLS reduces the risk of an outsider attacker. However the ability for a third party to execute privileged code in Firefox isn't desirable
(In reply to David Chan [:dchan] from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > Created attachment 8616874 [details] [diff] [review]
> > Patch
> > 
> 
> There also appears to be some string concatenation in saved.js [1]

The code in question there looks to be pulling values from about:config. I agree it would be good to fix this as well, but I think it is less severe since it will also require an attack that allows writing to the preferences. I don't consider users manually editing about:config a security risk since they already have physical access to the machine.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> (In reply to David Chan [:dchan] from comment #5)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > > Created attachment 8616874 [details] [diff] [review]
> > > Patch
> > > 
> > 
> > There also appears to be some string concatenation in saved.js [1]
> 
> The code in question there looks to be pulling values from about:config. I
> agree it would be good to fix this as well, but I think it is less severe
> since it will also require an attack that allows writing to the preferences.
> I don't consider users manually editing about:config a security risk since
> they already have physical access to the machine.

Ah, I didn't realize that was only for config entries. I agree that the config attack vector isn't an issue. I used it as a quick and dirty way to not have to setup a HTTPS server.
Going to go with sec-high since it's not expected that Pocket can run arbitrary code in Firefox (apart from the code-drops we include and presumably audit). Also concerned about the impact of the other places the broken token code is used but that commonly leads to at least XSS type bugs.
Comment on attachment 8616874 [details] [diff] [review]
Patch

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

Had Nick review, he said both changes looked fine.

::: browser/components/pocket/pktApi.js
@@ +269,1 @@
>                      }

With the change to tokeninput, do need this?

I took a look at how we handle this code on our end, for example in our web app, and there we just sanitize the text right before printing it. 

I believe doing this will actually strip out valid characters from tags. For example ";" is a valid character in a tag for Pocket. (Note, whether or not they should be valid characters could be a question but given they are already out there in user accounts, not sure we can tackle that scope here).

Also, I didn't test this to confirm if this ever actually comes out of our API, but for example, if a tag has a UTF character or comes through as an entity like &lt; stripping the & will leave the remaining string "lt;"
Hm, the code I was commenting on didn't come through in Comment 9. I am referring to the changes in pktApi.js
The pktApi.js changes shouldn't be strictly necessary, it is more of a defense-in-depth practice to try to provide protection at multiple levels.
(In reply to Daniel Veditz [:dveditz] from comment #8)
> Going to go with sec-high since it's not expected that Pocket can run
> arbitrary code in Firefox

I still don't think that's appropriate, as I said in comment 3. This really should be identical to the existing Pocket SocialAPI button, which also runs code in a content iframe. The only reason we're doing exactly the same thing is because of performance -- SocialAPI panel contents can feel laggy when it's downloading/checking content over the network.

However, this is a bit moot, because of the following:


(In reply to David Chan [:dchan] from comment #5)

> Unfortunately, it appears some part of the Pocket panel code runs with
> chrome since it can access Components.stack.

Uhh! That absolutely should not be happening and _is_ a problem. And yet I can confirm it is by adding a console.log of this from browser/components/pocket/panels/signup.html, and clicking the Pocket button. D:

I don't understand why that's happening -- it's an <iframe type=content>. We did change this slightly in bug 1164940 (to create it lazily), but reverting that has no effect.

bholley -- any ideas what's happening here?
Flags: needinfo?(bobbyholley)
(I belatedly realized dveditz's comment was after the Components.stack bit in comment 5, so maybe that was already taken into account. Sorry for any confusion.)
As discussed on irc, iframe type="content" has absolutely no effect on the principal of docshell loads - it just affects window.top and whatnot. It should be used only to achieve observables, not as any sort of sandboxing mechanism.

Docshell loads from chrome:// give you system principal, full stop. [1]

[1] iframe sandbox might be an exception, but I wouldn't recommend relying on that.
Flags: needinfo?(bobbyholley)
Attached patch Remove chrome privs. (obsolete) — Splinter Review
At least the fix is simple.

This is the important fix for this bug -- the other patch isn't a bad idea, but it's the minor issue.
Attachment #8621375 - Flags: review?(jaws)
(I did some testing with the fix, and everything seems to still work fine. Except that the "Sign up for Pocket. It's free." text in the signup panel shows some mojibake, so there's some charset stuff to fixup too.)
Attached patch Remove chrome privs, v.2 (obsolete) — Splinter Review
Attachment #8616365 - Attachment is obsolete: true
Attachment #8621379 - Flags: review?(jaws)
I suppose it's worth noting:

* None of the code which we were expecting to be unprivileged runs until the Pocket panel is shown. So if a user never clicks the button (or has outright removed it from the UI), there is no risk.

* From a quick skim, I don't see any obvious places where the signup.html panel is looking at data returned from the server. So if that's true, users who have not actually signed in to Pocket should not be at risk (just users who have signed in to Pocket, and then click the button to save a page).
Attachment #8621375 - Attachment is obsolete: true
Attachment #8621375 - Flags: review?(jaws)
(In reply to (Limited avail. until June 16)  Jared Wein [:jaws] (please needinfo? me) from comment #11)
> The pktApi.js changes shouldn't be strictly necessary, it is more of a
> defense-in-depth practice to try to provide protection at multiple levels.

Just so that it doesn't break actual user tags, I'd suggest A) removing this or B) escaping the characters instead of deleting (ie convert ">" to "&gt;")
Boris, is loading things from resource:///chrome/... kosher?
Flags: needinfo?(bzbarsky)
I... have no idea.  I don't understand our resource protocol handler that well.  I _think_ Benjamin does, so punting the question to him.
Flags: needinfo?(bzbarsky) → needinfo?(benjamin)
(In reply to Justin Dolske [:Dolske] from comment #17)
> Created attachment 8621379 [details] [diff] [review]
> Remove chrome privs, v.2

Waiting on needinfo response for Benjamin before reviewing.
resource:///chrome will work, but this looks like pretty unusual usage. Typically if you're going to point the browser at an unprivileged URL, we do that using about: so that this would be something like about:pocketsignup?...

Certainly having a URL of the form resource:///chrome/browser/content/browser/pocket/panels/signup.html? relies on details of how we package chrome into omnijar which could change in the future and shouldn't be relied-on.
Flags: needinfo?(benjamin)
Comment on attachment 8621379 [details] [diff] [review]
Remove chrome privs, v.2

We should use an about: URI here.
Attachment #8621379 - Flags: review?(jaws) → review-
Use about:pocket-saved / about:pocket-signup. Reverified that privs are still dropped.
Attachment #8621379 - Attachment is obsolete: true
Attachment #8623905 - Flags: review?(jaws)
Attachment #8623905 - Flags: review?(jaws) → review+
Attachment #8616874 - Attachment is obsolete: true
Attachment #8616874 - Flags: review?(nate)
Attachment #8616874 - Flags: review?(dolske)
Assignee: jaws → dolske
We should probably do the fix in attachment 8616874 [details] [diff] [review] as a followup belt-n-suspenders fix, even though I don't believe it's a serious problem itself -- once unprivileged. Given how close we are to the 39 release, and that there's still some details to sort out (comment 9 / comment 19), no need to block on that.
Comment on attachment 8623905 [details] [diff] [review]
Remove chrome privs, v.3

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The basic problem is described comment 0, but I think only someone with effective control of the Pocket server could exploit this. The connections are all SSL, and comment 18 describes further limitations on when this can potentially happen.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Maybe, depends on how knowledgeable someone is about how this stuff works. But I'd assume "yes".

Which older supported branches are affected by this flaw?

Pocket was added in 38.0.5.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch should be nearly identical on all branches, since little has changed in Pocket when we landed it everywhere for 38.0.5.

How likely is this patch to cause regressions; how much testing does it need?

Seems low risk, and would be constrained to the Pocket feature itself. Manual testing I've done verifies that everything seems to be working.
Attachment #8623905 - Flags: sec-approval?
Attachment #8623905 - Flags: sec-approval? → sec-approval+
Comment on attachment 8623905 [details] [diff] [review]
Remove chrome privs, v.3

Approval Request Comment
[Feature/regressing bug #]: Pocket
[User impact if declined]: A compromised Pocket server could inject code into a chrome-privileged context in some cases.
[Describe test coverage new/current, TreeHerder]: Manual testing
[Risks and why]: Risk contained to Pocket itself, manual testing verifies that it functions correctly after this patch.
[String/UUID change made/needed]: n/a
Attachment #8623905 - Flags: approval-mozilla-beta?
Attachment #8623905 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/39cd581fcc14
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8623905 [details] [diff] [review]
Remove chrome privs, v.3

Approved for uplift to beta and aurora. sec-high, sounds bad.
Attachment #8623905 - Flags: approval-mozilla-beta?
Attachment #8623905 - Flags: approval-mozilla-beta+
Attachment #8623905 - Flags: approval-mozilla-aurora?
Attachment #8623905 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-release/rev/924287d37077

For future reference, security approval typically isn't noted in the commit messages lest it draw extra attention to the commit.
Thanks Ryan. Also thanks for catching the beta approvals meant for m-r!!
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
This bug just came to my attention.  Per review conversations on bug 1215694 we removed about urls and are using chrome urls.  Sounds like I need to go back to about urls.
Flags: needinfo?(dolske)
regression being handled in bug 1236755
Flags: needinfo?(dolske)
Can someone cc me on bug 1236755 ?
Just thought i would mention here that if this has to do with about: URLs, or the way they work changing, then I would very much like to see the changes.  I'm currently getting heavily involved in click jacking work, and part of it has to do with pages like about:blocked.  I don't want to say anymore than that, but for those that have access see bug 1236139, and bug 1236989.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.