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

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tanvi, Assigned: tanvi)

Tracking

(Blocks: 1 bug)

35 Branch
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm5+, firefox38 fixed)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Summary: e10s - Mixed content detection does not take redirection into account → [e10s] Mixed content detection does not take redirection into account
Assignee: nobody → tanvi
tracking-e10s: --- → m5+
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?
Flags: needinfo?(tanvi)
(Assignee)

Updated

4 years ago
See Also: → bug 704320
(Assignee)

Comment 2

4 years ago
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?
Flags: needinfo?(tanvi)
(Assignee)

Comment 3

4 years ago
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.
Flags: needinfo?(mrbkap)
(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.
Flags: needinfo?(mrbkap)
(Assignee)

Updated

4 years ago
See Also: → bug 820466
(Assignee)

Updated

4 years ago
No longer blocks: 1006881
(Assignee)

Comment 5

4 years ago
Created attachment 8566075 [details] [diff] [review]
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!
Attachment #8566075 - Flags: review?(mrbkap)
(Assignee)

Comment 6

4 years ago
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
(Assignee)

Comment 7

4 years ago
Created attachment 8566197 [details] [diff] [review]
Bug1084504-enable-e10s-tests.patch

Enable mixed content redirect tests for e10s.
Attachment #8566197 - Flags: review?(mrbkap)
(Assignee)

Updated

4 years ago
No longer depends on: 1082837
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.
Attachment #8566075 - Flags: review?(mrbkap) → review+
Attachment #8566197 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/f835c33057fd
https://hg.mozilla.org/mozilla-central/rev/e3ecf5768337
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.