1146724 - (CVE-2015-2718) Untrusted page can see webchannel responses

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

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

Attachment: Demonstration of wrong page getting message response.

Consider:
* Untrusted page http://evil.com hosts trusted page http://good.com in an iframe.
* http://good.com dispatches a WebChannelMessageToChrome event - http://evil.com does not seem to have access to this event, but content.js does see it and dispatches it to the parent process - so far, so good.
* WebChannel.jsm sees the event as coming from http://good.com so allows it to be processed.
* WebChannel listener does it's thing and calls channel.send() to send the reply.
* http://good.com does *not* see the response, but http://evil.com does.

The following attachment demonstrates this in a test.  browser_web_channel_iframe.html is good.com in this scenario, and browser_web_channel.html is evil.com.  browser_web_channel.html sees the response to the message from browser_web_channel_iframe.html.

[Setting as a security bug as a precaution - eg, we send back about:support information in a webchannel response, and if the page we allow this on doesn't explicitly prevent itself being loaded in an iframe we could have a bad information leak]

Comment 1

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

Attachment: possible-fix.patch

The following patch seems to fix the issue:

* content.js sends the event target to the parent process as a CPOW.

* WebChannel.jsm is changed so that the "sender" object is no longer the content sending the message, but a regular object with "messageSender" and "eventSender" properties.  This is passed to the channel listener as the "target" param, as all-but-1 such listeners treat this as an opaque object - I wanted to avoid making the listener take a 3rd param and remember to pass that 3rd object back to .send().

* When the listener calls .send with that new sender object, the caller deconstructs it back to the message sender and the event target CPOW.  It then passes that event target back down to content.js.

* content.js then sends the message to the correct node instead of unconditionally sending it to content.

With this patch my test case works and the parent no longer sees the response.

The 1 listener that *does not* treat |target| as opaque is loop, which uses it to close the content window.  That would need fixing too, but I haven't bothered fixing that as this is just a proof-of-concept.

FTR, the much simpler "fix" for this would be for content.js to refuse to accept any such event where event.target != content, but that would prevent iframes from working at all - and it seems Firefox Accounts would like to have this work in iframes so, eg, the first-run page can host accounts.firefox.com in an iframe and have it talk to the browser via channels.

Not sure who to request feedback from, but I'm sure some will come anyway ;)

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8582212 [details] [diff] [review]
possible-fix.patch

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

::: browser/base/content/content.js
@@ +560,4 @@
>  // Add message listener for "WebChannelMessageToContent" messages from chrome scripts
>  addMessageListener("WebChannelMessageToContent", function (e) {
>    if (e.data) {
> +    let eventTarget = e.objects.target;

Doesn't this mean that chrome can't send a message to content without content sending one first? IIRC that wasn't the case before.

Comment 3

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

Comment on attachment 8582212 [details] [diff] [review]
possible-fix.patch

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

::: browser/base/content/content.js
@@ +560,4 @@
>  // Add message listener for "WebChannelMessageToContent" messages from chrome scripts
>  addMessageListener("WebChannelMessageToContent", function (e) {
>    if (e.data) {
> +    let eventTarget = e.objects.target;

Ah - in that case, I imagine:

let eventTarget = e.objects.target || content;

would work for that and still solve the issue.

Comment 4

[Chris Karlof [:ckarlof]]

Comment on attachment 8582212 [details] [diff] [review]
possible-fix.patch

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

Good start, but we have a few issues to figure out.

::: browser/base/content/content.js
@@ +560,4 @@
>  // Add message listener for "WebChannelMessageToContent" messages from chrome scripts
>  addMessageListener("WebChannelMessageToContent", function (e) {
>    if (e.data) {
> +    let eventTarget = e.objects.target;

If there's a page/frame load between the content -> chrome message and the chrome -> content reply, then we could also be delivering the response to the wrong origin. Do we also need an origin check here? We send the principal up, so it would probably be easy to send it back down and verify it here.

::: toolkit/modules/WebChannel.jsm
@@ +67,5 @@
>    _listener: function (event) {
>      let data = event.data;
> +    let sender = {
> +      messageSender: event.target,
> +      eventSender: event.objects.target,

These names are a bit confusing to me. event.objects.target is the thing that actually sent the WebChannel message. messageSender refers to the thing that sent the cross-process message, I suppose.

This also changes the format of sender. We bubble up the sender when call channel.deliver(), so I'm not sure if we're breaking any WebChannel consumers. If consumers are just passing it back to .send(), then there shouldn't be any problem. ¯\_(ツ)_/¯

At the very least, we'd be breaking FxAccountsOAuthClient.jsm's use of it: https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsOAuthClient.jsm#213. Not a hard fix, though.

Comment 5

[Chris Karlof [:ckarlof]]

Shane, if this WIP unblocks you, you can continue to work on your patch and content server work. We'll need to address these issues eventually, but as we knew going into this, markh has his hands full with RL.

Comment 6

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

Gavin, I'm not going to have time to look at this over the next day or 2 at least.  ISTM we should get *some* fix in relatively quickly, and comment 4 implies that my previous comment:

> FTR, the much simpler "fix" for this would be for content.js to refuse to accept any such event 
> where event.target != content, but that would prevent iframes from working at all

isn't actually going to be enough.

Comment 7

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

OK, so this feature was introduced in Firefox 34 (bug 1022064), and the particular problematic uses here are bug 1047130 (Loop FxA login via oauth, added in Firefox 34) and bug 1079563 (remote troubleshooting info, added in Firefox 35).

We can mitigate the risk that this is abused in practice by ensuring that the two sites we have whitelisted for troubleshooting info (input.mozilla.org and support.mozilla.org) only ever request this info (i.e. fire WebChannelMessageToChrome) from pages that have anti-iframing protection (CSP/X-Frame-Options).

I'm not sure what the potential impact to the FxA oauth case is exactly, but https://accounts.firefox.com pages already seem to have X-Frame-Options: DENY, so that's good.

Comment 8

[:Gijs (he/him)]

While making those arrangements on our server(s) is great, shouldn't we also enforce this on the client, ie refuse to listen to these events except from toplevel documents and enforce that they have the relevant CSP criteria set?

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #8)
> While making those arrangements on our server(s) is great, shouldn't we also
> enforce this on the client, ie refuse to listen to these events except from
> toplevel documents and enforce that they have the relevant CSP criteria set?

But the FxA guys actually *want* this to work when they are hosted in an iframe.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

It's too late for 36 and 37. We should target a fix for 38.

Comment 11

[Shane Tomlinson [:stomlinson]]

> I'm not sure what the potential impact to the FxA oauth case is exactly, but https://accounts.firefox.com pages already seem to have X-Frame-Options: DENY, so that's good.

The top level FxA page uses X-Frame-Options: DENY, but the /oauth/* pages do not. This is so reliers can embed the OAuth flow in a lightbox. We perform a postMessage based origin check - we wait for a pre-defined postMessage and check the postMessage's event.origin to ensure it matches the one expected for that relier's OAuth client_id.

For the new first run experience (see bug #1142046), we are probably going to relax the X-Frame-Options restriction for the top level page to allow the firstrun page to frame FxA. We'll performing a similar postMessage based origin check.

Comment 12

[Will Kahn-Greene [:willkg] ET needinfo? me]

cc:ing Mike (SUMO) so he has some context, too.

Comment 13

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > While making those arrangements on our server(s) is great, shouldn't we also
> > enforce this on the client, ie refuse to listen to these events except from
> > toplevel documents and enforce that they have the relevant CSP criteria set?
> 
> But the FxA guys actually *want* this to work when they are hosted in an
> iframe.

Hosted in an iframe by whom? Aren't we the parent frame in that case? Seems like we could forward the messages from the content side.

I'm uncomfortable with the idea that the desktop implementation's safety relies on our remembering that we have to set the right CSP options on all the implementing servers (and that those are honoured and not preffed off or disabled by an add-on or whatever).

Comment 14

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

(In reply to :Gijs Kruitbosch from comment #13)
> Hosted in an iframe by whom? Aren't we the parent frame in that case? Seems
> like we could forward the messages from the content side.

about:home hosting accounts.firefox.com in an iframe.

> I'm uncomfortable with the idea that the desktop implementation's safety
> relies on our remembering that we have to set the right CSP options on all
> the implementing servers (and that those are honoured and not preffed off or
> disabled by an add-on or whatever).

Desktop's safety will not rely on remembering to set CSP options once this bug is fixed. Comment 7 was describing short term mitigations only (i.e. what we can do while this bug is not fixed).

Comment 15

[Chris Karlof [:ckarlof]]

As Shane mentioned, FxA is already iframed in some circumstances (e.g., about:accounts, some OAuth flows), but in general we don't allow FxA to be iframed. Furthermore, I don't believe there anything currently being sent from chrome -> content in the current uses of WebChannel by FxA (i.e., it doesn't use channel.send()). In summary, I don't believe FxA is effected by the vuln. I appears the remote troubleshooting patch does use channel.send(). It's using the "permission" check (vs. the origin check), so I don't fully understand the ramifications of that, but:

> We can mitigate the risk that this is abused in practice by ensuring that the two sites we have whitelisted for troubleshooting info (input.mozilla.org and support.mozilla.org) only ever request this info (i.e. fire WebChannelMessageToChrome) from pages that have anti-iframing protection (CSP/X-Frame-Options).

I agree that would likely be a good short-term mitigation until we have a more general fix landed in Fx.

For the current effort (allow FxA to be iframed by the firstrun page to log in to Sync), I'd prefer we not "pile on the legacy" by proxying the chrome -> content messages through the firstrun page (although firstrun pages should also have anti-framing protections in place). I don't believe the fix is hard (WIP patch here), but it's work, and Desktop team is already pretty swamped for 38.

Comment 16

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

Does this need to remain confidential? It sounds like there's no action to take for the only consumer I was concerned about.

Comment 17

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

We still need to fix this in Firefox, and even if we feel like the currently whitelisted sites are protected, I'd rather not have people trying to figure out how to work around those protections.

Comment 18

[Shane Tomlinson [:stomlinson]]

Attachment: bug-1146724-webchannel-iframe-and-origin-001.patch

I took Mark's original patch and made several significant changes. 

1. Allow the browser to send unsolicited messages to a window.
2. Unsolicited messages can be sent to a target window's iframe descendents.
3. Before sending a message, check the target's principal matches the expected principal.

Comment 19

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

Comment on attachment 8591619 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-001.patch

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

I think this patch should just do the target+origin check while ensuring "allow unsolicited" semantics don't break. Then a separate not-secure bug with any iframe semantics can track the other stuff we need for embedded FxA frames.

::: browser/base/content/content.js
@@ +591,5 @@
>    // if target is window then we want the document principal, otherwise fallback to target itself.
>    let principal = e.target.nodePrincipal ? e.target.nodePrincipal : e.target.document.nodePrincipal;
>  
>    if (e.detail) {
> +    sendAsyncMessage("WebChannelMessageToChrome", e.detail, { target: e.target }, principal);

We have to capture the principal/origin here - we want to guard against redirections between the send and response. We'd pass it to the parent, which passes it back, and we re-check the origin of target still reflects target.

Comment 20

[Shane Tomlinson [:stomlinson]]

> We have to capture the principal/origin here - we want to guard against redirections between the send and response. We'd pass it to the parent, which passes it back, and we re-check the origin of target still reflects target.

Thanks Mark, I was wondering how to best do this. I originally implemented the patch using origin, and then converted to principal. Is the principal insufficient? In WebChannel.jsm, I capture the principal from sendAsyncMessage:

> +    let sender = {
> +     browser: event.target,
> +     window: event.objects.target,
> +     principal: event.principal
> +   };

I then send this principal back down to the frame script and ensure the principal of the target and the expected principal match.

> +    if (doesTargetMatchExpectedPrincipal(target, e.principal)) {


I tried to search for documentation on the purpose of principal whenever passed to sendAsyncMessage, both MDN and dxr leave me scratching my head.

Converting the patch to do an origin check should be pretty straight forward, as is splitting the patch up into multiple patches. I'll split this into two patches, one to do the origin check, the next to take care of what I need for an iframed Sync.

Comment 21

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

Comment on attachment 8591619 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-001.patch

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

::: browser/base/content/content.js
@@ +596,1 @@
>    } else  {

Doh, yes, please ignore my previous comments - it looks like principal is being managed correctly here and checked correctly below (although I'm not sure about the "target.nodePrincipal || target.document.nodePrincipal" check, but that's only because I know almost zero about these things).  We'll probably need someone else to check that part.

Comment 22

[Shane Tomlinson [:stomlinson]]

> Doh, yes, please ignore my previous comments - it looks like principal is being managed correctly here and checked correctly below 

Thanks for re-checking. Do you still prefer this patch be split into two, one for the principal check to fix the security issue, and another to handle messaging into an iframe?

> (although I'm not sure about the "target.nodePrincipal || target.document.nodePrincipal" check, but that's only because I know almost zero about these things).  We'll probably need someone else to check that part.

nodePrincipal is defined on a node (imagine that)[1]. All nodePrincipal checks [2] use the principal from the document. Since doesTargetMatchExpectedPrincipal accepts a window, only the target.document.nodePrincipal portion of that line should be needed.

[1] - https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Node.webidl#109
[2] - https://dxr.mozilla.org/mozilla-central/search?q=nodePrincipal&case=true

Comment 23

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

(In reply to Shane Tomlinson [:stomlinson] from comment #22)

> Thanks for re-checking. Do you still prefer this patch be split into two,
> one for the principal check to fix the security issue, and another to handle
> messaging into an iframe?

I think so, yeah. Do you even need that now for your fxa work?

> only the
> target.document.nodePrincipal portion of that line should be needed.

right - I guess that's what I was getting at :)

Comment 24

[Shane Tomlinson [:stomlinson]]

> I think so, yeah. Do you even need that now for your fxa work?

I will need a portion of it, yes. More specifically, I'll need the portion that sends a response back to the originating iframe window. I do not need the portion that searches for child iframes that match the principal if no window is specified (the uninitiated send case). I added that because I was already digging around and saw an unhandled corner case.

Comment 25

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

(In reply to Shane Tomlinson [:stomlinson] from comment #24)
> I do not need
> the portion that searches for child iframes that match the principal if no
> window is specified

Perfect!

Comment 26

[Shane Tomlinson [:stomlinson]]

Mark, currently, no prinicpal check is performed in the unsolicited send case. The browser can send a message to any window even if the target window's origin does not match the channel's `_originOrPermission`. Is this expected?

A principal can be created from the channel's `_originOrPermission.prePath` and passed to the frame script so a principal check can take place.

I took that approach in the initial patch [1], but want to ensure this restriction is necessary.

[1] - https://bugzilla.mozilla.org/attachment.cgi?id=8591619&action=diff#a/toolkit/modules/WebChannel.jsm_sec4

Comment 27

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

(In reply to Shane Tomlinson [:stomlinson] from comment #26)
> Mark, currently, no prinicpal check is performed in the unsolicited send
> case. The browser can send a message to any window even if the target
> window's origin does not match the channel's `_originOrPermission`. Is this
> expected?

It's probably not ideal, but I don't think it is a risk for this bug (ie, I don't think there's an identified scenario where this additional check would fail). I think it's fine to do something like I said in comment 3 - IOW, solve *just* the known potential vuln in the sanest way and leave the rest to feature bugs.

Comment 28

[Shane Tomlinson [:stomlinson]]

Attachment: bug-1146724-webchannel-iframe-and-origin-002.patch

This is a simplified patch that only fixes the security issue. A WebChannel response is only sent to the initiating window, if the initiating window has not been redirected to a different origin.

Comment 29

[Shane Tomlinson [:stomlinson]]

Comment on attachment 8592417 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-002.patch

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

::: browser/base/content/test/general/browser_web_channel.js
@@ +155,5 @@
>        });
>      }
> +  },
> +  {
> +    desc: "WebChannel unsolicited send",

This test is added to ensure the semantics of an unsolicited send has not changed.

::: browser/base/content/test/general/browser_web_channel_iframe.html
@@ +94,5 @@
> +    });
> +
> +    window.dispatchEvent(readyMessage);
> +
> +    setTimeout(function () {

The setTimeout is to give a chances for messages to be received in lines 71-89.

Comment 30

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

Comment on attachment 8592417 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-002.patch

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

Looking pretty good to me, although note FxAccountsOAuthClient.jsm will also need a fix.

I'm going to want another reviewer here too, just to cover my/our ass ;) In the next version, please also request feedback from MattN - although note he probably isn't available for a week or so - let me know if that delay will cause a problem and I can talk to Gavin about another reviewer (I'd also suggest ckarlof, but I think his availability will have the same problem)

::: browser/base/content/content.js
@@ +600,5 @@
>  
>  // Add message listener for "WebChannelMessageToContent" messages from chrome scripts
>  addMessageListener("WebChannelMessageToContent", function (e) {
> +  function doesTargetWindowMatchPrincipal(targetWindow, principal) {
> +    return !principal || targetWindow.document.nodePrincipal.equals(principal);

I think this should just be inline (it's a little strange to have a doesMatch function accept a null principal, but inline with a comment about why a null principal is allowed seems clearer

::: browser/base/content/test/general/browser_web_channel.html
@@ +29,5 @@
> +        break;
> +      case "unsolicited":
> +        test_unsolicited();
> +        break;
> +      case "unexpected":

I can't see test_unexpected here.  A default and a dump might be OK as we'd expect all tests to fail if they don't enter their correct case.

@@ +102,5 @@
> +  function test_iframe() {
> +    // XXX - note that this message is the response to the message sent
> +    // by the iframe!  This is bad, as this page is *not* trusted.
> +    window.addEventListener("WebChannelMessageToContent", function(e) {
> +      echoEventToChannel(e, "echo");

It looks like the dump should go (or be changed to better reflect the error situation) and a comment indicating that the test parent will fail if the echo message is received in this test.

Also remove the XXX above as this block doesn't imply a "todo" or limitation in the test - IIUC, it's doing exactly what it should - so I think those first 2 comment lines should have the "XXX" removed and text appended to indicate the echo will cause test failure as it should never be hit.

@@ +107,5 @@
> +
> +      dump("SAW IFRAME EVENT: " + JSON.stringify(e.detail) + "\n");
> +    });
> +
> +    // only attach the iframe for the iframe test to avoid

wouldn't the iframe in the html with about:blank be ok?  I guess in a perfect world we would have content from a different origin loaded and somehow cause failure if it saw any of these events - but that sounds like alot of effort for marginal gain...

::: browser/base/content/test/general/browser_web_channel.js
@@ +61,5 @@
>      }
>    },
>    {
> +    desc: "WebChannel two way communication in an iframe",
> +    run: function* () {

a nit, but this function (like the ones you copied) aren't actually generators - so the '*' should be removed (and I think you might as well remove them from the existing functions too)

::: browser/base/content/test/general/browser_web_channel_iframe.html
@@ +52,5 @@
> +    window.dispatchEvent(firstMessage);
> +  }
> +
> +
> +  function test_iframe_pre_redirect() {

I think brief comments about how the redirect tests work would help.

@@ +77,5 @@
> +      });
> +
> +      window.dispatchEvent(echoMessage);
> +
> +      // XXX - this message is the response to the message sent

same comment here as before re dump/XXX. Also, comment that the fact the message isn't either 'ready' or 'done' will cause test failure (and if I'm reading browser_web_channel.js correctly, the test will actually timeout - it looks as though it wants another '} else { reject(...) }'?

@@ +95,5 @@
> +
> +    window.dispatchEvent(readyMessage);
> +
> +    setTimeout(function () {
> +      var doneMessage = new window.CustomEvent("WebChannelMessageToChrome", {

We can probably avoid the timeout by having the .js code post to the preRedirectChannel and then post another message to the postRedirectChannel - once the post redirect channel echos its response back we can probably assume that if the message to the preRedirectChannel was going to have been delivered, it would have been.

::: toolkit/modules/WebChannel.jsm
@@ +65,5 @@
>     * @private
>     */
>    _listener: function (event) {
>      let data = event.data;
> +    let sender = {

As Chris mentioned in comment 4, "sender" probably isn't a great name - maybe renaming sender to, say, sendingContext or something might be better.

@@ +118,5 @@
>     *        Error message
>     * @private
>     */
> +  _sendErrorEventToContent: function (id, target, errorMsg) {
> +    let { targetBrowser, targetWindow, targetPrincipal } = normalizeTarget(target);

Do we really need this normalize here? It looks to me as though this is only called via ._listener, which always has the "new improved" sender object.

@@ +251,5 @@
>     *        The <browser> object that has a "messageManager" that sends messages
>     *
>     */
>    send: function (message, target) {
> +    let { targetBrowser, targetWindow, targetPrincipal } = normalizeTarget(target);

so we have the normalize here for the "unsolicited" case, right?  Assuming I'm correct about not needing to normalize in the error handler above, I'd be inclined to unpack that inline here with a comment talking about the unsolicited send case - ends up as 2 lines instead of many. I also wonder about sending targetWindow and targetPrincipal this way when they are undefined but I guess that's OK.

Comment 31

[Shane Tomlinson [:stomlinson]]

> Looking pretty good to me, although note FxAccountsOAuthClient.jsm will also need a fix.

Updated.

> I'm going to want another reviewer here too, just to cover my/our ass ;)

100% fair. This touches a lot of parts.

> In the next version, please also request feedback from MattN - although note he probably isn't available for a week or so - let me know if that delay will cause a problem and I can talk to Gavin about another reviewer (I'd also suggest ckarlof, but I think his availability will have the same problem)

If review is delayed another week, will uplift into Fx 39 be at risk? Would Vlad Filippov or Zach Carter be capable of review? Both have fairly intimate knowledge of WebChannels.

I have addressed your other comments, new patch incoming. Thanks for the quick turn around!

Comment 32

[Shane Tomlinson [:stomlinson]]

Attachment: bug-1146724-webchannel-iframe-and-origin-003.patch

Comment 33

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

Comment on attachment 8592799 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-003.patch

I didn't look at the tests again, but looking good.

Gavin, I'd like another reviewer here and suggested MattN, but Shane's concerned with uplift for 39 if we wait until he gets back - see comment 31. So I've requested feedback from you, but I fully expect you to nominate someone else - including Matt if you think uplifting to 39 will still be OK if we wait for him to return.

Comment 34

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

Comment on attachment 8592799 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-003.patch

Matt is around - he said he'd review by tomorrow.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8592799 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-003.patch

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

I'll finish taking a look at this tomorrow. I don't see any fundamental issues yet.

::: browser/base/content/test/general/browser_web_channel.html
@@ +30,5 @@
> +      case "unsolicited":
> +        test_unsolicited();
> +        break;
> +      default:
> +        dump(`INVALID TEST NAME ${testName}`);

How about throwing so the test harness has a chance to catch this?
throw new Error(`INVALID TEST NAME ${testName}`)

::: toolkit/modules/WebChannel.jsm
@@ +67,5 @@
>    _listener: function (event) {
>      let data = event.data;
> +    let sendingContext = {
> +      browser: event.target,
> +      window: event.objects.target,

The DOM event could have been dispatched as bubbling on a child of the window so the target isn't necessarily a window. If we just call this target then it's easy to get it mixed up with the message target so maybe we can call it eventTarget. Can you add a testcase for the event being dispatched on something other than a window (e.g. document or an element inside of it) with bubbles: true. I just want to make sure that this patch is adding an assumption that the event target is a content window.

Note that I don't feel strongly that we need to support non-window targets but we currently do work to support it in the event listener of content.js so we should be consistent.

Comment 36

[Matthew N. [:MattN]]

Comment on attachment 8592799 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-003.patch

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

::: browser/base/content/test/general/browser.ini
@@ +18,5 @@
>    browser_ssl_error_reports_content.js
>    browser_star_hsts.sjs
>    browser_tab_dragdrop2_frame1.xul
>    browser_web_channel.html
> +  browser_web_channel_iframe.html

I don't see browser_web_channel_iframe.html in this patch

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8592799 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-003.patch

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

::: toolkit/modules/WebChannel.jsm
@@ +257,5 @@
> +    let { browser: targetBrowser, window: targetWindow, principal: targetPrincipal } = target;
> +
> +    // If targetBrowser does not exist, this is an unsolicited send and target should be a <browser> object.
> +    if (!targetBrowser) {
> +      targetBrowser = target;

Can we just update existing consumers/tests to pass an object with a lone `browser` property to this method so we don't have to keep this baggage around?

Comment 38

[Shane Tomlinson [:stomlinson]]

Attachment: bug-1146724-webchannel-iframe-and-origin-004.patch

Important changes from v003:

* Ensure events can be received from and sent to non-window elements.
* Throw errors in .html pages instead of dumping to console.

Comment 39

[Shane Tomlinson [:stomlinson]]

Thanks for the feedback MattN, you were correct in pointing out that I assumed the event target was a window. I have issued a new patch ensuring the any element can be an event target and that responses are sent to the correct element.

Comment 40

[Matthew N. [:MattN]]

Comment on attachment 8594492 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-004.patch

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

Code review done. Reviewing the tests now.

::: browser/base/content/content.js
@@ +289,5 @@
> +    // otherwise fallback to target itself.
> +    let targetPrincipal = eventTarget.nodePrincipal ? eventTarget.nodePrincipal : eventTarget.document.nodePrincipal;
> +
> +    // a null principal is possible if the message is unsolicited.
> +    if (!e.principal || targetPrincipal.equals(e.principal)) {

We may want to loosen this so .subsumes(…) in the future but I think this is okay for now. Maybe not since cases where we want to broadcast to all mozilla.org tabs may hit the !e.principal case if they're unsolicited.

@@ +292,5 @@
> +    // a null principal is possible if the message is unsolicited.
> +    if (!e.principal || targetPrincipal.equals(e.principal)) {
> +      // if eventTarget is a window, it is the targetWindow, otherwise
> +      // find the window that owns the eventTarget.
> +      let targetWindow = eventTarget.CustomEvent ? eventTarget : eventTarget.ownerDocument.defaultView;

It seems like you're trying to do sniffing here in an indirect manner which is something that we generally avoid since it's hard to understand the intention. If you want to know whether eventTarget is a Window then just check that instead of something you assume should only be on a Window (which would be wrong if a custom "CustomEvent" property was added to any other event target Element by a web developer). I think you want `eventTarget instanceof Window`.

::: toolkit/modules/WebChannel.jsm
@@ +249,5 @@
>     *        The message object that will be sent
> +   * @param target {Object}
> +   *        A <target> with the information of where to send the message.
> +   *        @param target.browser {EventTarget}
> +   *               EventTarget with a "messageManager" that will be used to

EventTarget doesn't seem right here. Isn't it still a browser XULElement in most cases (like the previous comment)? Do we (want to) support passing a nsIDOMChromeWindow which also has a messageManager property?

/me wonders why the types and argument names are in reversed order in JSDoc for this file.

@@ +256,5 @@
> +   *               Optional eventTarget within the browser, use to send to a
> +   *               specific element, e.g., an iframe.
> +   *        @param [target.principal] {Principal}
> +   *               Optional principal of the target. Prevents messages from
> +   *               being dispatched to unexpected origins.

I would kinda prefer that we would make this required so developers have to consciously choose to send to all origins. For that we would use subsumes instead of equals and allow the origin here to be the system principal when a broadcast to all origins was intended.

@@ +267,3 @@
>          id: this.id,
>          message: message
> +      }, { eventTarget: eventTarget }, targetPrincipal);

Nit: You don't need the `: eventTarget` portion since both sides have the same name.

@@ +279,5 @@
>     *
>     * @param data {Object}
>     *        Message data
> +   * @param sendingContext {Object}
> +   *        Message sending context.

It would be good to document this param (at least listing some of the valid properties) since it's an exported and public method.

Comment 41

[Shane Tomlinson [:stomlinson]]

Attachment: bug-1146724-webchannel-iframe-and-origin-005.patch

Differences from 004.patch:

WebChannel.jsm

* target.principal is now required by `send`.
* simplified variable names in `send`
* Add documentation about the `sendingContext` for `deliver`

content.js

* use `eventTarget instanceof Ci.nsIDOMWindow` to detect whether the eventTarget is a window.
* The principal check now uses `subsumes` to allow the system principal to be defined to send to any domain.

Tests

* Tests added to ensure sending to the system principal, the target origin's principal, and a mismatched principal all function as expected.

Comment 42

[Matthew N. [:MattN]]

Comment on attachment 8595945 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-005.patch

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

Awesome job! I love the tests to address the review comments :)

No need for re-review from me if you're just addressing these comments and the changes are straight-forward.

::: browser/base/content/test/general/browser_web_channel.html
@@ +118,5 @@
> +  }
> +
> +  function test_iframe_pre_redirect() {
> +    var iframe = document.createElement('iframe');
> +    iframe.setAttribute('src', IFRAME_SRC_ROOT + '?iframe_pre_redirect');

Nit: There are some added lines with single-quotes in this file.

::: browser/base/content/test/general/browser_web_channel.js
@@ +70,5 @@
> +        parentChannel.listen(function (id, message, sender) {
> +          iframeChannel.stopListening();
> +          parentChannel.stopListening();
> +          gBrowser.removeTab(tab);
> +          reject(new Error('WebChannel message incorrectly sent to parent'));

Nit: We use double-quotes for most code in mozilla-central.

@@ +121,5 @@
> +         *    WebChannel does not perform a valid origin check, the response
> +         *    will be received by origin B. If the WebChannel does perform
> +         *    a valid origin check, the response will not be sent.
> +         * 6. the test parent sends a `done` message to origin B, which origin
> +         *    B echoes back. If the response to origin A is not been echoed but

Nit: "is not been"

@@ +205,5 @@
> +
> +        waitForTab(() => {
> +          let targetBrowser = gBrowser.getBrowserForTab(tab);
> +          let systemPrincipal = Cc['@mozilla.org/systemprincipal;1']
> +                                  .createInstance(Ci.nsIPrincipal);

It seems to be more common to do `Services.scriptSecurityManager.getSystemPrincipal();` so I would guess that's preferred.

@@ +354,5 @@
>      finish();
>    });
>  }
> +
> +function waitForTab(aCallback) {

We now have helpers in BrowserTestUtils.jsm so we don't need to duplicate this logic. You can replace this with usage of openNewForegroundTab and/or withNewTab.

Comment 43

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

Comment on attachment 8595945 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-005.patch

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

I've nothing to add to Matt's very thorough review - nice job Shane!  Please fix Matt's review comments and add checkin-needed to the new patch.

Comment 44

[Ryan Kelly [:rfkelly]]

In the interests of moving along quickly, should Shane set any additional flags to get this smoothly into 39?

Comment 45

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

(In reply to Ryan Kelly [:rfkelly] from comment #44)
> In the interests of moving along quickly, should Shane set any additional
> flags to get this smoothly into 39?

The typical process is to let it land and get to m-c, then to request approval‑mozilla‑aurora, and approval‑mozilla-beta on the patch. Once approved, the sheriffs typically handle the uplift.

Comment 46

[Shane Tomlinson [:stomlinson]]

Attachment: bug-1146724-webchannel-iframe-and-origin-006.patch

Patch 006 addresses MattN's comments about replacing `'` with `"`, and using BrowserTestUtils.withNewTab instead of a local copy of `waitForTab`

Comment 47

[Matthew N. [:MattN]]

Since this is sec-high I think this needs security approval before landing and we may want to land this without the tests to reduce disclosure of the issue. The commit message may also need to be neutered.

Shane, can you also do a try run?

See https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 48

[Matthew N. [:MattN]]

Comment on attachment 8596607 [details] [diff] [review]
bug-1146724-webchannel-iframe-and-origin-006.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The code changes aren't super obvious without the current message but the comments in the tests make it very obvious.

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

Which older supported branches are affected by this flaw?
Landed in Firefox 34 (according to comment 7) so I ESR 31 shouldn't be affected.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't think there's been too much churn recently for uplift. 

How likely is this patch to cause regressions; how much testing does it need?
This only affects 2 Mozilla-specific APIs (remote troubleshooting and FxA/Sync) which should be tested with automated tests.

Comment 49

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Yes, this needs to land without tests. We don't check in the tests, normally, until the issue is fixed in a release.

This will want Aurora and Beta patches made and nominated as well.

Comment 50

[Matthew N. [:MattN]]

I'll actually push this to try now with a vaguer commit message and no tests since I'm pushing something else anyways.

Comment 51

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

Attachment: Sanitized commit message and without tests

Same patch but with a sanitized commit message and without tests.

Try for this commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6494015a8090

(I've already mentioned this to Al and Matt, but I screwed up and pushed a try job with the original commit (ie, original message and tests). Fortunately the combination of it being unlikely this can be exploited and the likelihood of the commit being lost in the try-noise means it shouldn't be too bad)

Comment 52

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

Attachment: tests

Comment 53

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

Attachment: without tests - aurora version.

Try against Aurora with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe59d73a978

Approval Request Comment
[Feature/regressing bug #]: Bug 1022064
[User impact if declined]: See comment 48 for more context.
[Describe test coverage new/current, TreeHerder]: No tests in this patch.
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 54

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

Attachment: without tests - beta version

Try against Beta with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e583748188c5

Approval Request Comment
[Feature/regressing bug #]: Bug 1022064
[User impact if declined]: See comment 48 for more context.
[Describe test coverage new/current, TreeHerder]: No tests in this patch.
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 55

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

Attachment: 0001-Bug-1146724-Use-a-SendingContext-for-WebChannels.-r-.patch

*sigh* - one of the test changes really was necessary, although that doesn't give anything away. This version has that change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=347dd8e6ea5f

Comment 56

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

Attachment: Tests

Comment 57

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

Comment on attachment 8596925 [details] [diff] [review]
without tests - aurora version.

Try is failing for a different reason than the one against -central did.

Comment 58

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

Attachment: beta version

This is the same (rebased) patch I'll land on central - it has one test change necessary for the existing tests to pass.

Try is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb6d135f565b - note it has e10s failures (which is expected) and also jetpack failures (which don't seem to be run on beta, and while I can't see how this could possibly be related, someone should probably check)


Approval Request Comment
[Feature/regressing bug #]: Bug 1022064
[User impact if declined]: See comment 48 for more context.
[Describe test coverage new/current, TreeHerder]: No tests in this patch.
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 59

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

Attachment: Aurora version

This is the same (rebased) patch I'll land on central - it has one test change necessary for the existing tests to pass.

Try is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5fbb765b671 - note it has e10s failures (which is expected).

Approval Request Comment
[Feature/regressing bug #]: Bug 1022064
[User impact if declined]: See comment 48 for more context.
[Describe test coverage new/current, TreeHerder]: No new tests in this patch.
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 60

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

https://hg.mozilla.org/integration/fx-team/rev/780cf81ab0d3

Comment 61

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

Making this up as I go along; leave-open until the tests land.

Comment 62

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

"leave-open for tests" is too confusing, let's track that separately somehow (in-testsuite? or another bug).

Comment 63

[Al Billings [:abillings - ex-MoCo]]

I see in-testsuite? used commonly for that.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/780cf81ab0d3

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ab01d355c93

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/56d740d0769f

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56d740d0769f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7c4b49739ece
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/5f2c57b580ff
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/5f2c57b580ff

Comment 68

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Would someone mind documenting these changes on the WebChannel.jsm MDN page?

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/WebChannel.jsm

Comment 69

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

tests never landed and simple devdoc changes remain outstanding - ni? myself to fix that.

Al, this is fixed in 38, so we can open this up now, right?

Comment 70

[Al Billings [:abillings - ex-MoCo]]

Yes, I expect we can open this up but I'm deferring to Dan here. He opens the bugs to the public and shifts groups normally.

Comment 71

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

Attachment: 0001-Bug-1146724-test-for-WebChannel-SendingContext.-r-Ma.patch

The tests bit-rotted - I fixed that, but skipped the refactoring of the existing tests (ie, I only added the new tests). They pass locally and I'll push them if try is green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=beb29137a218

Comment 72

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

Attachment: 0001-Bug-1146724-test-for-WebChannel-SendingContext.-r-Ma.patch

Try failed on e10s. It turns out the tests were relying on timing - that the channels would be created before the content loaded, but that's not always true with e10s. I refactored the new tests to setup the channel and the listener before calling BrowserTestUtils.withNewTab.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=340b48952f36

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/675ef3c3fb45