776766 - sidebar can use window.location to redirect to a non-same origin page

Status: RESOLVED INVALID

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

This bug is to split out one part of the issue from bug 775336

We want to limit the sidebar to loading same-origin pages.  Currently you can redirect to a different origin using window.location.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

As per the suggestion in bug 775336, I'm looking at using nsIContentPolicy for this.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Use nsIContentPolicy to prevent the sidebar going off-domain

Just looking for feedback as this patch can't land until bugs 775779 and 778409 are applied.

This patch creates an xpcom component implementing nsIContentPolicy to avoid the sidebar assigning to window.location a URL in a different origin.  Note that due to the way nsIContentPolicy works, it is impossible to tell the difference between window.location being set and an <a> tag being clicked on - so in both cases, if the origin is different we use getMostRecentWindow("navigator:browser") to locate a window to use to open the requested URL in a new tab.  As a result, isAppTab no longer needs to be set on the browser element so that is removed (there is no point setting something that has no effect!)

There is a commented block in SocialContentPolicy.js which doesn't seem needed - any thoughts on how true that is are appreciated!  There is also a log function which is enabled - I intend keeping the function but commenting out the 'dump' it does.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated patch that also prevents the panel going off-origin

Same comment as previous patch, except:  this patch also prevents the notification panel from going off-domain, but note there are no specific tests for the notification panel case (the sidebar is tested though)

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 647824 [details] [diff] [review]
updated patch that also prevents the panel going off-origin

looks good, I think it may need to be rebased on top of bugs 777176, 779686 and 779923.  

+    if (aContext instanceof Ci.nsIDOMNode && aContext.localName == "browser" &&
+        aContext.id == "social-sidebar-browser" ||
+        aContext.id == "social-notification-browser") {

We should be able to use aContext.hasAttribute("origin") here as any social browser will have an origin attribute.  That would let us avoid an update to the content policy every time we add a new content space (e.g. bugs 777176, 779686 and 779923)

+        let gBrowser = Services.wm.getMostRecentWindow("navigator:browser").gBrowser;
+        gBrowser.selectedTab = gBrowser.addTab(aContentLocation.spec);

This doesn't check against popups using window type navigator:browser.  Might be able to use openLinkIn which uses getTopWin:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#60

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: Ensure the "provider state" and sidebar is in a sane state after test, and refactor sidebar tests ready for more tests.

These are just test related but make part b easier to grok.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated per comments

Like the old attachment, this still have some commented out code and logging enabled (hence feedback requested - not quite ready for landing yet).  Feedback on whether that commented code is necessary would be welcome.

This new version doesn't use getMostRecentWindow etc at all - seeing it also have a reference to the social widget, the ownerDocument.defaultView of the parameter must be the correct top window.

This also assumes any <browser> elt with an "origin" attribute is a social one, so specific element ID checks are avoided.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 649997 [details] [diff] [review]
Updated per comments


>+        // We are going to reject it, so open the URL externally.  We do this
>+        // as we can't tell the difference between the content setting
>+        // window.location (which we could just reject without opening
>+        // externally), versus an <a> tag (which we do want to open.)
>+        // We want to use whatever gBrowser is associated with aContext...
>+        let gBrowser = aContext.ownerDocument.defaultView.gBrowser;
>+        gBrowser.selectedTab = gBrowser.addTab(aContentLocation.spec);
>+        logResult("REJECT", providerOrigin + " != " + uri.prePath);
>+        return Ci.nsIContentPolicy.REJECT_SERVER;


This still isn't going to work for the service window.  I think we could just use:

let win = aContext.ownerDocument.defaultView;
win.openUILink(aContentLocation.spec, "tab");

After thinking about this further, I'm concerned that this is an attack vector for bad providers.  Simply changing window.location will open new tabs on a user.  

If the content policy simply rejects the change, will isAppTab allow user initiated events to open in new tabs?

Everything else looks fine.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 649997 [details] [diff] [review]
Updated per comments

I don't think there's a clear need for this limitation, given that the sidebar has no special privileges, and this doesn't seem like a useful mitigation against provider mistakes.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> I don't think there's a clear need for this limitation, given that the
> sidebar has no special privileges, and this doesn't seem like a useful
> mitigation against provider mistakes.

Agreed.  INVALID.