1084504 - [e10s] Mixed content detection does not take redirection into account

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

In e10s, nsMixedContentBlocker does not handle mixed content redirects properly.  This is because nsMixedContentBlocker relies on loadInfo->loadingNode, which can't be passed from child to parent.  nsMixedContentBlocker::AsyncOnChannelRedirect() is first called on the parent, and then the child.  Since the parent blocks the request, the child never has a chance to properly handle the request.  Instead, we should only call this on the child.  See https://bugzilla.mozilla.org/show_bug.cgi?id=418354#c65 and https://bugzilla.mozilla.org/show_bug.cgi?id=418354#c66

What is the impact of this?
In e10s, when a page includes an https subresource that redirects to http:
* The mixed content is blocked regardless of the about:config preference that determines whether it should be blocked
** For mixed display content, the default is to allow the content.  The default will be ignored and mixed display content will not load on the page.
** For mixed active content, if the user changes the default to allow the content, the users choice will be ignored and the content will be blocked anyway.

* The UI to un-block the content does not appear (i.e. no shield doorhanger)

* The usual webconsole error messages for mixed content will not appear.


Example urls:
https://people.mozilla.org/~tvyas/mixedcontentredir.html - mixed script redirect
https://people.mozilla.org/~tvyas/mixeddisplayredir.html - mixed display redirect

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

From bug 418354, comment 74:

I was thinking about this the other day. Wouldn't it be enough, in the parent, to do:

nsCOMPtr<nsIParentChannel> is_ipc_channel;
NS_QueryNotificationCallbacks(channel, is_ipc_channel);
if (is_ipc_channel) {
  // Let the child process take care of this notification.
}

jduell seemed to think that would work. Tanvi, want to try that?

Comment 2

[Tanvi Vyas[:tanvi]]

Comments from bug 418354:
(In reply to Blake Kaplan (:mrbkap) from comment #74)
> I was thinking about this the other day. Wouldn't it be enough, in the
> parent, to do:
> 
> nsCOMPtr<nsIParentChannel> is_ipc_channel;
> NS_QueryNotificationCallbacks(cnanel, is_ipc_channel);
> if (is_ipc_channel) {
>   // Let the child process take care of this notification.
> }

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #75)
> re: comment 74. Good idea Blake, that ought to work perfectly.  We could do
> that test and then return early in
> nsMixedContentBlocker:AsynconChannelRedirect if that's the case. Tanvi, does
> that sound doable?  File a new bug?

Comment 3

[Tanvi Vyas[:tanvi]]

Comments from bug 418354:
(In reply to Blake Kaplan (:mrbkap) from comment #74)
> I was thinking about this the other day. Wouldn't it be enough, in the
> parent, to do:
> 
> nsCOMPtr<nsIParentChannel> is_ipc_channel;
> NS_QueryNotificationCallbacks(cnanel, is_ipc_channel);
> if (is_ipc_channel) {
>   // Let the child process take care of this notification.
> }

Blake, just want to confirm that this is enough to guarantee:
1) We are running e10s
2) We are in the parent process
3) A child process does exist (and hence the code will get triggered later).

If this is the case, this fix looks good to me.  I can add it to the code and flag it for review.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Blake, just want to confirm that this is enough to guarantee:
> 1) We are running e10s

For the record "running e10s" is a fuzzy concept. We have the ability to launch child processes at pretty much any time. That being said, we won't create any nsIParentChannels if we have no child processes.

> 2) We are in the parent process

Yes.

> 3) A child process does exist (and hence the code will get triggered later).

Yes. nsIParentChannels are created explicitly in order to proxy network requests from child processes.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Bug1084504-02-19-15.patch

Patch checks if the channel is a parent process and skips Mixed Content checks on redirects in that case; it lets the child handle them.

Blake, I have one question inline in the patch.  Should I be checking the OldChannel (the original url that triggered the redirect) or the NewChannel (the one we are about to load) to see if we are in the parent process?  My guess is the NewChannel, but wanted to check with you.

Thanks!

Comment 6

[Tanvi Vyas[:tanvi]]

As for tests, we can enable browser_mcb_redirect.js for e10s.  This was the test in the original bug for mixed content blocking redirects - bug 418354.
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_mcb_redirect.js

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Bug1084504-enable-e10s-tests.patch

Enable mixed content redirect tests for e10s.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8566075 [details] [diff] [review]
Bug1084504-02-19-15.patch

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

::: dom/security/nsMixedContentBlocker.cpp
@@ +229,5 @@
> +  // the docShell.  If that's the case, ignore mixed content checks
> +  // on redirects in the parent.  Let the child check for mixed content.
> +  nsCOMPtr<nsIParentChannel> is_ipc_channel;
> +  // ?? - Should I be checking the oldchannel or the new redirected channel?
> +  NS_QueryNotificationCallbacks(aNewChannel, is_ipc_channel);

It doesn't matter which one you check because we set the notification callbacks on the new channel before calling into this code.

Doing it on the new channel seems clearer to me, though, so this looks good.

Comment 9

[Tanvi Vyas[:tanvi]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f835c33057fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ecf5768337

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f835c33057fd
https://hg.mozilla.org/mozilla-central/rev/e3ecf5768337