Closed Bug 1236755 Opened 4 years ago Closed 4 years ago

Moving Pocket to an addon reintroduced bug 1172226?

Categories

(Firefox :: Pocket, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- unaffected
firefox46 + fixed
firefox-esr38 --- unaffected

People

(Reporter: Dolske, Assigned: mixedpuppy)

References

Details

(Keywords: sec-high)

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(mixedpuppy)
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... :-(
I'll add back the about pages.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Keywords: sec-high
Attached patch pocket about pages (obsolete) — Splinter Review
Attachment #8705418 - Flags: review?(gijskruitbosch+bugs)
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? :-)
Attachment #8705418 - Flags: review?(gijskruitbosch+bugs)
Oh, hmpf, I guess the human-readable description for the registerFactory call should be dynamic as well. Sorry!
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. :-\
Attached patch pocketaboutSplinter Review
Attachment #8705418 - Attachment is obsolete: true
Attachment #8706678 - Flags: review?(gijskruitbosch+bugs)
Attachment #8706678 - Attachment is patch: true
Comment on attachment 8706678 [details] [diff] [review]
pocketabout

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

LGTM!
Attachment #8706678 - Flags: review?(gijskruitbosch+bugs) → review+
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*
(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...
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.
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/d0d15f0e1083
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: Firefox 46 → ---
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.)
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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.
Group: firefox-core-security → core-security-release
Group: core-security-release
[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
You need to log in before you can comment on or make changes to this bug.