1236755 - Moving Pocket to an addon reintroduced bug 1172226?

Status: RESOLVED FIXED

(Firefox :: Pocket, defect)

Description

[Justin Dolske [:Dolske]]

I just noticed https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/browser/extensions/pocket/content/main.js#165

Is this not reintroducing the security issued fixed by bug 1172226? This code was never really audited for running in a chrome context.

Comment 1

[:Gijs (he/him)]

Ugh. My fault for suggesting that in feedback and not realizing there was a security reason for using about: here. Sorry. On the plus side, this is only on Nightly so far... :-(

Comment 2

[Shane Caraveo (:mixedpuppy)]

I'll add back the about pages.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: pocket about pages

Comment 4

[:Gijs (he/him)]

Comment on attachment 8705418 [details] [diff] [review]
pocket about pages

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

::: browser/extensions/pocket/bootstrap.js
@@ +70,5 @@
>    })
>    return element;
>  }
>  
> +let AboutSaved = {

This and the other one are completely identical except for the URI they point to and point at, and their classID. Can we reduce this to just 1 JS object with a prototype + constructor that we create two instances of? Something like:

XPCOMUtils.defineLazyGetter(this, "AboutSaved", function() {
  return new PocketAboutPage("chrome://pocket/content/panels/saved.html", "pocket-saved", "{3e759f54-37af-7843-9824-f71b5993ceed}");
});

XPCOMUtils.defineLazyGetter(this, "AboutSignup", function() {
  return new PocketAboutPage("chrome://pocket/content/panels/signup.html", "pocket-signup", "{8548329d-00c4-234e-8f17-75026db3b56e}");
});

function PocketAboutPage(chromeURL, aboutHost, classID) {
  this.chromeURL = chromeURL;
  this.aboutHost = aboutHost;
  this.classID = Components.ID(classID);
}

PocketAboutPage.prototype = {
  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutModule]),
  getURIFlags: function(aURI) {
    return Ci.nsIAboutModule.ALLOW_SCRIPT |
           Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT |
           Ci.nsIAboutModule.HIDE_FROM_ABOUTABOUT |
           Ci.nsIAboutModule.MAKE_UNLINKABLE;
  },

  newChannel: function(aURI) {
    let channel = Services.io.newChannel(this.chromeURL, null, null);
    channel.originalURI = aURI;
    return channel;
  },

  createInstance: function(outer, iid) {
    if (outer != null) {
      throw Cr.NS_ERROR_NO_AGGREGATION;
    }
    return AboutSaved.QueryInterface(iid);
  },

  register: function() {
    Cm.QueryInterface(Ci.nsIComponentRegistrar).registerFactory(
      this.classID, "About Pocket Saved",
      "@mozilla.org/network/protocol/about;1?what=" + this.aboutHost, this);
  },

  unregister: function() {
    Cm.QueryInterface(Ci.nsIComponentRegistrar).unregisterFactory(
      this.classID, this);
  }
};

I've not tested this code, but I believe it should work? :-)

Comment 5

[:Gijs (he/him)]

Oh, hmpf, I guess the human-readable description for the registerFactory call should be dynamic as well. Sorry!

Comment 6

[:Gijs (he/him)]

And:

    return AboutSaved.QueryInterface(iid);

should be this.QueryInterface... - ok, note to self: don't write more than 5 lines of code in a bugzilla comment again. :-\

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: pocketabout

Comment 8

[:Gijs (he/him)]

Comment on attachment 8706678 [details] [diff] [review]
pocketabout

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

LGTM!

Comment 9

[Cody Crews]

I'm just curious here, I do know that there was an RCE issue but doesn't pocket have about pages that get a moz-safe-about principal like the rest?

*Starts digging through the original RCE issue and it's reintroduction*

Comment 10

[:Gijs (he/him)]

(In reply to Cody Crews from comment #9)
> I'm just curious here, I do know that there was an RCE issue but doesn't
> pocket have about pages that get a moz-safe-about principal like the rest?

The problem here was that we accessed them using their chrome: URI instead of as about: pages. The patch moves them back to (unprivileged) about: pages.

Of course, the other followup belt + suspenders fix would be to actually audit the pocket code against RCE issues...

Comment 11

[Cody Crews]

Yeah I remembered why I I didn't get all thrilled by seeing it as soon as I looked.  It requires javascript to be set as a page through a user pref which is something we definitely have to fix but it would mainly pose a social engineering danger.

I'll be looking over pocket in a lot more depth, I have a bug related to click jacking to get filed although it's sub par even compared to requiring a user set pref being js.  I should have a testcase for it ready in a few minutes.

After that I have some issues that are promising but I could take a little time to look over pocket in depth.

Comment 12

[:Gijs (he/him)]

(In reply to Cody Crews from comment #11)
> Yeah I remembered why I I didn't get all thrilled by seeing it as soon as I
> looked.  It requires javascript to be set as a page through a user pref
> which is something we definitely have to fix but it would mainly pose a
> social engineering danger.

AIUI that was just to make it easy to reproduce, but pocket actually fetches a list of tags remotely. If that list is compromised on the server (ie having access to your account in whatever way) that would result in the same thing. Having pocket account/API/MITM access meaning RCE immediately is already an issue worth fixing.

Comment 13

[Cody Crews]

So it actually renders elements in a a page from a chrome URI?  Yeah wow, it seems like i remember reading that but how did that even make it past a review in the first place?

I've done my fair share of sec-audits on ff and I believe the very first thing always in mind is no direct access to anything chrome level.  I'll have to have that look now, should have that testcase i mentioned ready in about 10 minutes.

Comment 14

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/d0d15f0e1083fd52230e7e0bd55b07c1799a4bbb
Bug 1236755 use about urls for panel iframes, r=Gijs

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0d15f0e1083

Comment 16

[Cody Crews]

Just letting everybody know that I'm sorry I haven't got that new bug on file yet so I can audit pocket.  I had the testcase working, but I like my stuff to work on as many systems as possible and I had forgot to consider element placement.

I'm currently doing the work to figure out how to make sure these elements are properly aligned on as many systems with different screen sizes and resolution as possible.

I'm also having to tackle another problem, but I believe I have just figured it out.  Since click jacking is involved I'm having to figure out a way to be sure a certain action has in fact happened and the click jacking itself messes up the obvious event handlers one could use for this.

I'm fairly sure I've found what I need.  The interesting part of click jacking is I can move on to testing nearly the same setups in other browsers without having to change much of a testcase.

I'm getting there, it *should* be ready by tomorrow, then I can get to pocket.

Comment 17

[Justin Dolske [:Dolske]]

The cause and fix for this bug is clearly understood. Please take discussion on other topics to a different bug. (And please watch out for clobbering bug flags, I've set them back to the expected state.)

Comment 18

[Cody Crews]

I was referring to auditing pocket for additional security related bugs which was mentioned here.  Sorry about pollution by discussing another bug.

Also sorry about the flag tampering, I don't even whitelist mozilla.org in noscript and since I forgot to toggle scripts it didn't load their current states.  Very sorry.

Comment 19

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected