878890 - Mixed content blocking activates even if extensions rewrite all resources to HTTPS

Status: RESOLVED WONTFIX

(Core :: DOM: Security, defect, P5)

Description

[Peter Eckersley]

Mixed content blocking in Firefox 23+ has apparently been implemented in such a way that, even if an extension like HTTPS Everywhere rewrites every request in the page to HTTPS, resources will be blocked anyway.

Comment 1

[Peter Eckersley]

Here's an reproduction example:

1. Install HTTPS Everywhere 4.0development.7: https://eff.org/files/https-everywhere-4.0development.7.xpi

2. Go to https://xkcd.com

3. Observe that mixed content blocking has activated and is preventing CSS from loading (at least while https://bugzilla.mozilla.org/show_bug.cgi?id=861961 is unfixed).

4. Now enable your favourite network request monitoring tool (Live HTTP Headers, Fireshark, ABP's "open blockable items", whatever

5. Manually disable mixed content blocking

6. Observe that no HTTP requests were sent after mixed content blocking was disabled.

This bug is going to massively reduce the usability and usefulness of HTTPS Everywhere if Firefox 23 ships with it unfixed :(.

Comment 2

[Peter Eckersley]

Historically, HTTPS Everywhere's method for rewriting requests was fairly complicated, but for the purposes of understanding/fixing this bug it can be assumed that it observes "http-on-modify-request" events and then invokes https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIHttpChannel#redirectTo()

Comment 3

[Tanvi Vyas[:tanvi]]

Hi Peter,

Take a look at https://blog.mozilla.org/tanvi/2013/04/10/mixed-content-blocking-enabled-in-firefox-23/#Appendix.  The Mixed Content Blocker does not account for redirects.  If an HTTPS resources loads on an HTTPS page and then redirects to a HTTP page, Mixed Content Blocker does not learn about the HTTP load and does not block the HTTP resource.

In the same way, if an HTTP resource attempts to load on an HTTPS page, the Mixed Content Blocker will block the request, even if the HTTP resource were going to be rewritten to an HTTPS load (by say HSTS).

This is because nsContentPolicy is run before our redirect code is called.  CSP and Mixed Content Blocker are 2 types of a handful of nsContentPolicy.  In order to fix this, we'd need to rewrite nsContentPolicy to take redirection into account.  Alternatively, we could add a hack to each content policy to handle redirects.  CSP tried to do this, although I'm not sure it's actually working as intended.  In the absence of a nsContentPolicy rewrite, we planned to do the same for Mixed Content Blocker in bugs https://bugzilla.mozilla.org/show_bug.cgi?id=418354 and https://bugzilla.mozilla.org/show_bug.cgi?id=456957.  This won't be in place in Firefox 23 (and I'm not sure what release it will be in).  As the blog post describes, we wanted to start blocking sooner than later to protect our users even though there were some edge cases that we hadn't worked out yet.

I'm not sure how HTTPS Everywhere works, but I imagine it is similar to HSTS.

Comment 4

[Peter Eckersley]

IRC conversation about the difficulty of fixing this and possible hackarounds: http://pastebin.com/KwWKzZMu

Comment 5

[Mike Perry]

It has long been known by addon developers that the nsIContentPolicy is *not* a security mechanism. Why was it chosen for MCB?

It has numerous exemptions where content elements (and their cookies!) can still be transmitted without nsIContentPolicy observer oversight. The HTTPS->HTTP redirect insecurity you mention above is but one of the many security issues you will encounter.

The initial nsIContentPolicy-based implementation of HSTS was abandoned for similar reasons. You might want to head over to Sid Stamm's office for details.

If you want to implement MCB as an observer, you should be using something comprehensive like "http-on-modify-request" like HTTPS-Everywhere does (and HSTS does in practice by being part of nsHttpChannel). You can use nsIRequest.cancel to block requests from that observer before they hit the wire.

This will solve a lot of problems you're going to hit by using nsIContentPolicy, not just this one..

Comment 6

[Mike Perry]

If you want to be polite to addons and HSTS, I believe all you need to do is add a call to your MCB evaluation from nsHttpChannel::BeginConnect, and that should allow you to catch and block *all* mixed HTTP activity, not just the subset covered by nsIContentPolicy.

https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4414
https://mxr.mozilla.org/mozilla-central/ident?i=BeginConnect

If you add that call after the AsyncCall, OMR observers like ours will still be able to perform redirects.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike Perry from comment #6)
> If you want to be polite to addons and HSTS, I believe all you need to do is
> add a call to your MCB evaluation from nsHttpChannel::BeginConnect, and that
> should allow you to catch and block *all* mixed HTTP activity, not just the
> subset covered by nsIContentPolicy.
> 
> https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#4414
> https://mxr.mozilla.org/mozilla-central/ident?i=BeginConnect
> 
> If you add that call after the AsyncCall, OMR observers like ours will still
> be able to perform redirects.

I agree, Mike. I don't think the right answer is to do anything with nsIContentPolicy and redirects; rather, I believe the right thing to do is to move the enforcement of mixed content blocking (and CSP) to nsHttpChannel for HTTP/HTTPS requests. In nsHttpChannel, after we've already done any internal redirects (including HSTS, Alternate-Protocol, or extension-defined redirects), then if the request is HTTP (not HTTPS) look at the nsILoadContext to see if the nsILoadContext indicates that we should block HTTP requests by default (i.e. the document is HTTPS and the user hasn't disabled mixed content blocking for the load). If so, then check the channel's (new) contentPolicyType property (of type nsContentPolicyType defaulting to TYPE_OTHER), and use that value to decide (using the existing mixed content blocking logic) whether to allow the load. This means that we would need to change the places that handle <img>, <video>, <audio>, and plugin loads to set the contentPolicyType property on the channels they create. We wouldn't need to set the contentPolicyType for other types of loads because TYPE_OTHER is blocked as "active".

Tanvi wrote:
> CSP tried to do this, although I'm not sure it's actually
> working as intended.

CSP can be fixed the same way, though we'd have to change *every* place a channel is created in content to set the contentPolicyType attribute.

> I'm not sure how HTTPS Everywhere works, but I imagine it is similar to HSTS.

Yes, uses the same mechanism that HSTS uses for "internal redirects".

I agree with Tanvi that this is probably not going to get fixed for Firefox 23, and probably not for Firefox 24 either unless I can find more time or somebody else can jump in to help.

Comment 8

[Mike Perry]

(In reply to Brian Smith (:bsmith) from comment #7)
> (In reply to Mike Perry from comment #6)
> > If you want to be polite to addons and HSTS, I believe all you need to do is
> > add a call to your MCB evaluation from nsHttpChannel::BeginConnect, and that
> > should allow you to catch and block *all* mixed HTTP activity, not just the
> > subset covered by nsIContentPolicy.
> > 
> > https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> > nsHttpChannel.cpp#4414
> > https://mxr.mozilla.org/mozilla-central/ident?i=BeginConnect
> > 
> > If you add that call after the AsyncCall, OMR observers like ours will still
> > be able to perform redirects.
> 
> I agree, Mike. I don't think the right answer is to do anything with
> nsIContentPolicy and redirects; rather, I believe the right thing to do is
> to move the enforcement of mixed content blocking (and CSP) to nsHttpChannel
> for HTTP/HTTPS requests. In nsHttpChannel, after we've already done any
> internal redirects (including HSTS, Alternate-Protocol, or extension-defined
> redirects), then if the request is HTTP (not HTTPS) look at the
> nsILoadContext to see if the nsILoadContext indicates that we should block
> HTTP requests by default (i.e. the document is HTTPS and the user hasn't
> disabled mixed content blocking for the load). If so, then check the
> channel's (new) contentPolicyType property (of type nsContentPolicyType
> defaulting to TYPE_OTHER), and use that value to decide (using the existing
> mixed content blocking logic) whether to allow the load. This means that we
> would need to change the places that handle <img>, <video>, <audio>, and
> plugin loads to set the contentPolicyType property on the channels they
> create. We wouldn't need to set the contentPolicyType for other types of
> loads because TYPE_OTHER is blocked as "active".

Why can't you use an API like the one from bug #769589 to determine the originating url-bar URI for a channel, and block *all* HTTP content elements that are loaded when the url-bar scheme is HTTPS?

That would be way more intuitive to the user, and the use of url-bar based permissions would also simplify the door-hanger UI that allows users to say "Sure, allow mixed content from nytimes.com" rather than "Sure, allow mixed content from a543-somenoise.us.datacenter4.akamai.com".

> Tanvi wrote:
> > CSP tried to do this, although I'm not sure it's actually
> > working as intended.
> 
> CSP can be fixed the same way, though we'd have to change *every* place a
> channel is created in content to set the contentPolicyType attribute.
> 
> > I'm not sure how HTTPS Everywhere works, but I imagine it is similar to HSTS.
> 
> Yes, uses the same mechanism that HSTS uses for "internal redirects".
> 
> I agree with Tanvi that this is probably not going to get fixed for Firefox
> 23, and probably not for Firefox 24 either unless I can find more time or
> somebody else can jump in to help.

It seems sad that Mozilla doesn't have the resources to do this right. I kind of like the idea of url-bar based permissions to block mixed content, especially if the door-hanger UI would allow me to opt back in on a url-bar basis. I assume there will be a pref to disable this partial implementation until it can be done properly?

Also, I guess either Tor or EFF will probably have to write a blog post explaining to our users why the Firefox implementation of mixed content blocking doesn't provide the security they think it does, and why it's safe to disable it until this is fixed.

Comment 9

[Peter Eckersley]

(In reply to Mike Perry from comment #8)

> I assume there will be a pref to disable this partial implementation
> until it can be done properly?
> 

Yes, if you read the IRC logs linked above Tanvi pointed me to an about:config setting we could use to disable MCB for our users.  Presumably that would only be for a Firefox version or two while we wait for a patch to land that moves MCB after http-on-modify-request / channel.redirectTo().

Alternatively to the about:config setting there is also a flag (https://mxr.mozilla.org/mozilla-central/search?string=LOAD_FLAGS_ALLOW_MIXED_CONTENT) which, if we figure out how to apply it to requests efficiently, could be used to disable MCB but only for domains where HTTPS Everywhere is responsible for the use of any HTTPS in the first place.

Obviously disabling MCB for HTTPS Everywhere has a mixture of positive and negative security implications depending on the domain.  Fortunately I don't disabling MCB is are ever going to make things worse than they are in Firefox 22 + HTTPS-E, which is a pretty high standard.  

And if we work out how to use LOAD_FLAGS_ALLOW_MIXED_CONTENT I think we can tweak the ruleset library to leave MCB in place for domains where it's doing important security work (a heuristic for such domains is approximately: even with HTTPS-E installed they have some http content; while even without HTTPS-E installed they set the secure flag correctly on their authentication cookies).

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Mike Perry from comment #8)
> (In reply to Brian Smith (:bsmith) from comment #7)
> > (In reply to Mike Perry from comment #6)
> 
> Why can't you use an API like the one from bug #769589 to determine the
> originating url-bar URI for a channel, and block *all* HTTP content elements
> that are loaded when the url-bar scheme is HTTPS?

1. The Firefox mixed content blocker, just like the mixed content blocker from MSIE, and just like the one in Chrome, determines whether to block mixed content based on the type of load. For example, <img src=http://...> is NOT blocked by any of these browsers' mixed content blockers, but <script src=http://...> is. So, if we move the logic to the place you correctly identified (AFAICT from limited looking) in nsHttpChannel, the nsHttpChannel needs to know the nsContentPolicyType (TYPE_IMAGE, TYPE_SCRIPT, etc.) to correctly calculate whether the content should be blocked or not.

2. CSP has very similar problems and could also benefit from being moved to nsHttpChannel, but then nsHttpChannel would need to know the nsContentPolicyType for similar reasons.

Luckily, the mixed content blocker case is simpler because, like I said, we can take advantage of the defaulting to TYPE_OTHER to minimize the number of places we need to set the nsContentPolicyType to get the mixed content blocker working correctly initially.

> That would be way more intuitive to the user, and the use of url-bar based
> permissions would also simplify the door-hanger UI that allows users to say
> "Sure, allow mixed content from nytimes.com" rather than "Sure, allow mixed
> content from a543-somenoise.us.datacenter4.akamai.com".

The doorhanger we have already shows a clearer message than either of those options.

> Also, I guess either Tor or EFF will probably have to write a blog post
> explaining to our users why the Firefox implementation of mixed content
> blocking doesn't provide the security they think it does, and why it's safe
> to disable it until this is fixed.

Why not write a patch to fix the issue instead? Since you wrote the patches for bug 765934, you are probably the person with the best skillset + motivation to get this done. If you are interested in writing a patch, I will help you. I actually started to take a stab at it a couple of months ago (for a different reason, namely HSTS), but I didn't get very far before more pressing issues came up, so I can review the patch for you in a timely manner and I can also help answer any questions you may have. I really do want to work together but I simply don't have time to write the patch and tests myself.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Peter Eckersley from comment #9)
> Yes, if you read the IRC logs linked above Tanvi pointed me to an
> about:config setting we could use to disable MCB for our users.  Presumably
> that would only be for a Firefox version or two while we wait for a patch to
> land that moves MCB after http-on-modify-request / channel.redirectTo().

Setting this pref silently is against the addon security guidelines and would cause the addon to fail the AMO review.

> Alternatively to the about:config setting there is also a flag
> (https://mxr.mozilla.org/mozilla-central/
> search?string=LOAD_FLAGS_ALLOW_MIXED_CONTENT) which, if we figure out how to
> apply it to requests efficiently, could be used to disable MCB but only for
> domains where HTTPS Everywhere is responsible for the use of any HTTPS in
> the first place.

Addons that trigger mixed content situations won't pass the AMO review process. This is a longstanding rule; see bug 875607 comment 1. So, addons shouldn't be silently setting the LOAD_FLAGS_ALLOW_MIXED_CONTENT flag.

Every time we allow <scrpt src=http://...> and similar in an HTTPS context, we provide an avenue for a MITM to completely destroy the integrity of that HTTPS origin--perhaps permanently. That is the whole reason we implemented the mixed content blocker in the first place, and this is the reason for the longstanding rule against addons that create mixed content situations.

I know that HTTPS Everywhere is fighting the good fight and trying to make things better. I understand that it is very counter-intuitive that it is possible for an upgrade of http://example.org/ to https://example.org/ to *increase* the susceptibility to attacks by a MITM (when it causes mixed active content to be loaded into the https://example.org origin), but that's a reality.

Realistically, HTTPS Everywhere is and always has been a "best effort" security improvement. Sometimes you'll have to reduce the scope of the effect that HTTPS Everywhere has to deal with realities of how the browser is working.

> Obviously disabling MCB for HTTPS Everywhere has a mixture of positive and
> negative security implications depending on the domain.  Fortunately I don't
> disabling MCB is are ever going to make things worse than they are in
> Firefox 22 + HTTPS-E, which is a pretty high standard.  
> 
> And if we work out how to use LOAD_FLAGS_ALLOW_MIXED_CONTENT I think we can
> tweak the ruleset library to leave MCB in place for domains where it's doing
> important security work (a heuristic for such domains is approximately: even
> with HTTPS-E installed they have some http content; while even without
> HTTPS-E installed they set the secure flag correctly on their authentication
> cookies).

Please don't do these things that are just going to cause all of us trouble. It would make a lot more sense to work together to just fix this bug. (But, I think that even with this bug fixed, there will still be quite a few rulesets in HTTPS Everywhere that would cause mixed content and so will need to be disabled.)

Comment 12

[Peter Eckersley]

(In reply to Brian Smith (:bsmith) from comment #11)

> Setting this pref silently is against the addon security guidelines and
> would cause the addon to fail the AMO review.

Of course we wouldn't do this silently; we want to give users the best available outcome and 
sensible control over their browser's behaviour.

> Addons that trigger mixed content situations won't pass the AMO review
> process. This is a longstanding rule; see bug 875607 comment 1. So, addons
> shouldn't be silently setting the LOAD_FLAGS_ALLOW_MIXED_CONTENT flag.
> 
Per the original description at the top of this bug, when we rewrite http://xkcd.com to https://xkcd.com, we temporarily create a mixed content situation that we then proceed to fix by rewriting all of the CDN domains to https as well.  However in Firefox 23, MCB kicks in half way through the process.  We could selectively set LOAD_FLAGS_ALLOW_MIXED_CONTENT when we know that the mixed content is not real.

> Every time we allow <scrpt src=http://...> and similar in an HTTPS context,
> we provide an avenue for a MITM to completely destroy the integrity of that
> HTTPS origin--perhaps permanently. That is the whole reason we implemented
> the mixed content blocker in the first place, and this is the reason for the
> longstanding rule against addons that create mixed content situations.
> 
> I know that HTTPS Everywhere is fighting the good fight and trying to make
> things better. I understand that it is very counter-intuitive that it is
> possible for an upgrade of http://example.org/ to https://example.org/ to
> *increase* the susceptibility to attacks by a MITM (when it causes mixed
> active content to be loaded into the https://example.org origin), but that's
> a reality.

I agree that this is the reality.  Do you think I reasonably characterised the situations in which that occurs in the text quoted below?

> > (a heuristic for such domains is approximately: even
> > with HTTPS-E installed they have some http content; while even without
> > HTTPS-E installed they set the secure flag correctly on their authentication
> > cookies).
> 
> Please don't do these things that are just going to cause all of us trouble.
> It would make a lot more sense to work together to just fix this bug.

Agreed.  I have been planning to sit down with Mike and Tanvyi to try and update the MCB patch to operate after http-on-modify-observer callbacks have finished.

> (But,
> I think that even with this bug fixed, there will still be quite a few
> rulesets in HTTPS Everywhere that would cause mixed content and so will need
> to be disabled.)

That was my assumption prior to encountering this bug (https://trac.torproject.org/projects/tor/ticket/6975) and I think that's still probably the best approach.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Mike Perry from comment #5)
> The initial nsIContentPolicy-based implementation of HSTS was abandoned for
> similar reasons. You might want to head over to Sid Stamm's office for
> details.

Let me save you a trip.  HSTS rewrites URLs (redirects them, really).  nsIContentPolicy is not intended to allow that; it's intended to allow/deny loads.  *That* is why HSTS did not work as an nsIContentPolicy -- this decision had nothing to do with security, but rather state consistency.  ShouldLoad is intended to just answer yes/no, not change the request.

CSP uses an nsIContentPolicy for some things because it's a reasonable tool for saying "no" to loads.  This is how we (per-site) block image loads or script loads.  (See nsContentBlocker.cpp, nsWebBrowserContentPolicy.cpp).

Granted, it could use some engineering love to make it more compatible with redirects.

Comment 14

[Peter Eckersley]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13)

> *That* is why HSTS did not work as an nsIContentPolicy -- this
> decision had nothing to do with security, but rather state consistency. 

Well, it's also the case that nsIContentPolicy doesn't police favicon.ico requests (https://bugzilla.mozilla.org/show_bug.cgi?id=437014) and maybe (I'm not sure) some other obscure cases like OCSP requests to a victim domain?  So a hypothetical version of HSTS implemented with it would have leaked cookies, too.  

I guess the current design for MCB does not aim to prevent cookie leakage unless the site uses the secure cookie flag (which I personally think is missing an opportunity to fix the now easier to solve equivalent of https://bugzilla.mozilla.org/show_bug.cgi?id=642675) these gaps in nsIContentPolicy may be deemed to not to matter for MCB.

> Granted, it could use some engineering love to make it more compatible with
> redirects.

So we could try to do two different things here.  Solution (A) is to move ContentPolicy so that it runs after http-on-modify-request is done.  Solution (B) is to change MCB to be something that's like an http-on-modify-request observer, but always running last.

In favour of (A) that will (probably) make things saner for any future interactions between the two APIs

Against (A) reordering these interfaces could conceivably cause weird bugs in numerous extensions (in the past we've seen things like https://trac.torproject.org/projects/tor/ticket/1685 although things are probably much saner now that we have channel.redirectTo).

In favour of (B) is that it would let strict MCB keep cookies safe.

Against (B) is that the different API situation might require changes to the way that MCB is plumbed and engineered, although I haven't checked that.

Comment 15

[Peter Eckersley]

Mike Perry and I are currently working on this.

We're thinking of adding a new nsIObserver topic "on-final-request-veto", which runs after "on-modify-request" and is used for this purpose.  Any reasons to avoid that and have this hardwired into the Channel machinery?

Implementation-wise, the observers would call nsIRequest.cancel(), and nsHttpChannel::BeginConnect would check mCanceled to see if it should bail out immediately after notifying the on-final-request-veto observers.

Comment 16

[Peter Eckersley]

In order to replicate the stanza at https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#319 which figures out aRequestingLocation to decide if the situation is mixed content at all, we could try a couple of approaches.

(1) is to use the nsIContentPolicy and store aRequestingLocation inside the channel using the exact logic that's currently in place.  (2) is to try to figure out the equivalent answer using the notificationCallbacks, loadgroup, and other stuff that's already in the channel.

Does anyone know how feasible (2) is?

Comment 17

[Peter Eckersley]

We are currently inclined to effectively ditch all of the scheme-related logic at https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#287 .  We believe that real-world mixed content vulnerabilities only occur in one case, which is content loaded over HTTP from HTTPS parents, and it's much simpler for us to move MCB into nsIHTTPChannel than all of the nsIChannel implementations.

If we wish to preserve the logic of blocking FTP or other weird script schemes from HTTPS pages, we will need to modify each of those Channel implementations to fire the new "on-final-request-veto" notification.  Does anyone want to argue that we should do that?

And aside from FTP, Gopher is dead, right?  View source channels aren't relevant.  What other kinds of channels go to the network?

Comment 18

[Peter Eckersley]

comment #16 is hopefully obsolete, we're using the channel's owner property.

(In reply to Peter Eckersley from comment #16)
> In order to replicate the stanza at
> https://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsMixedContentBlocker.cpp#319 which figures out aRequestingLocation to
> decide if the situation is mixed content at all, we could try a couple of
> approaches.
> 
> (1) is to use the nsIContentPolicy and store aRequestingLocation inside the
> channel using the exact logic that's currently in place.  (2) is to try to
> figure out the equivalent answer using the notificationCallbacks, loadgroup,
> and other stuff that's already in the channel.
> 
> Does anyone know how feasible (2) is?

Comment 19

[Peter Eckersley]

Attachment: Work-in-progress from mikeperry: moves MCB from ContentPolicy to the last moment in nsIHTTPChannel

I'm on sabbatical for a few months as of today.  Mike Perry has a work-in-progress patch (which does't yet compile and hasn't been tested, but should hopefully end up working) which I'm attaching here to encourage work on this ;).  In the mean time, Micah Lee and Lisa Yao are working on mitigating the chaos this bug is going to cause for HTTPS Everywhere users in FF 23+.

Comment 20

[Tanvi Vyas[:tanvi]]

This bug covers the long term fix for the compatibility issues with Mixed Content Blocker and HTTPS Everywhere.  This will definitely not be fixed for Firefox 23.

HTTPS Everywhere is working on a short term fix for FF23.  I'm hoping the short term fix is to disable the rulesets that cause Mixed Active Content.  I believe that is what they did for their Chrome extension when Chrome added a Mixed Content Blocker last summer.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Thanks Tanvi, not tracking in that case.

Comment 22

[Marti Raudsepp]

Ping, any update on this? This issue makes writing HTTPS Everywhere rules for some websites impossible and introduces subtle bugs for other websites which load most content from https with exceptions.

Comment 23

[Jorge Villalobos [:jorgev] (he/him)]

Mike, has there been any progress on the patch from comment #19, or is it something you need help with?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Jorge, I was about to look into this problem, taking the provided WIP-patch as a starting point. If you are are all up for it, you can have it though.

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> Jorge, I was about to look into this problem, taking the provided WIP-patch
> as a starting point. If you are are all up for it, you can have it though.

No, I was trying to keep the bug going. I don't have the time to look into it with the level of detail it needs. If you can take this, that'd be great :)

Comment 26

[Mike Perry]

I won't have time to work on this until much later in our FF24 rebase effort (December at the earliest). I'll keep an eye out for updates on this bug.

We're willing to try out any early patches in the FF24-based Tor Browser for this, too.

Also, FYI: The patch Peter attached was modified in a last minute attempt to address some comments Brian Smith had, and IIRC did not compile. I'll attach an earlier patch that I think actually compiled, so you can see a slightly different approach for reference.

Comment 27

[Mike Perry]

Attachment: For reference: Earlier WIP patch with a more direct wrapper around existing ShouldLoad call

This was my effort prior to the hack day patch that Peter attached. In some ways this patch might be a simpler approach (since it just creates a contentpolicy-like observer that can be used to wrap the existing MCB code pretty much as-is).

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_878890_mcb_rewrite_https.patch

Thanks to Peter and Mike for providing the WIP patch!

Enhancing on the WIP-code I am attaching a patch that eliminates the |ShouldLoad| call from the contentPolicy and instead calls |EvaluateMixedContent| via an observer shortly before the nsHTTPChannel requests data from a server.

Some thoughts/questions:
Is it ok to default to 'TYPE_OTHER' when not explicitly setting the contentPolicyType on the channel? I left some 'NEEDINFO' remarks in the patch to start the discussion, not covering all cases though. Basically we would need to set the contentPolicyType before every call to ShouldLoad. Defaulting to TYPE_OTHER seems to work fine (because TYPE_OTHER is blocked as active by MCB), even though not providing an accurate contenPolicyType in most cases.
Probably CSP wants to get away from |ShouldLoad| too - then it would make sense to update/set the contentPolicyType in all the different places, hence allowing precise contentTypes needed for CSP.

The patch provided seems to pass tests locally - probably we miss some edge cases though - pushing to TRY:
  https://tbpl.mozilla.org/?tree=Try&rev=c10c888429bd

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_878890_mcb_rewrite_https.patch

I slightly modified the patch to set the contentPolicyType on the channel wherever CSP calls channelPolicy->SetLoadType. This allows us to have a (somehow) consistent way of using the same policyType across different security features (MCB, CSP). This modification of the patch seems to work fine. However, there remain two challenging tasks:

a) Is there a reliable way to identify if a channel is used for SafeBrowsing or OCSP? I guess those cases should not be evaluated using MCB. Even though Safebrowsing starts off using https, it's redirected to http, which would then cause the MCB to block the request. Right now, it seems to me we can not identify such a channel reliably, therefore it might make sense to introduce a new LoadType (enhancing the current list [1]). We could then set this contentPolicyType whenever we create a safebrowsing channel [2]. Should work somehow similar for OCSP.

b) What is the right way to cancel an XMLHttpRequest? |channel->Cancel(NS_BINDING_ABORTED);| seems to work fine for all cases, except XMLHttpRequests. Looking at |nsXMLHttpRequest::Abort()| [3], it seems that canceling an XML request leads to a lot of cleanup work. How can we do that efficiently? (Knowing that we only have a channel available!)

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl#50
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#102
[3] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1206

Comment 30

[u408661]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #29)
> a) Is there a reliable way to identify if a channel is used for SafeBrowsing
> or OCSP? I guess those cases should not be evaluated using MCB. Even though
> Safebrowsing starts off using https, it's redirected to http, which would
> then cause the MCB to block the request. Right now, it seems to me we can
> not identify such a channel reliably, therefore it might make sense to
> introduce a new LoadType (enhancing the current list [1]). We could then set
> this contentPolicyType whenever we create a safebrowsing channel [2]. Should
> work somehow similar for OCSP.

For both OCSP and SafeBrowsing requests, I don't think you'll ever have a nsIDOMWindow, so your check on line 270 of nsMixedContentBlocker.cpp will take hold, and you won't cancel the channel.

> b) What is the right way to cancel an XMLHttpRequest?
> |channel->Cancel(NS_BINDING_ABORTED);| seems to work fine for all cases,
> except XMLHttpRequests. Looking at |nsXMLHttpRequest::Abort()| [3], it seems
> that canceling an XML request leads to a lot of cleanup work. How can we do
> that efficiently? (Knowing that we only have a channel available!)

For this one, I believe that you can (at the point your observer will be called) get the nsIXMLHttpRequest from the channel's notification callbacks, and you can then SlowAbort the XHR, which will cancel the channel.

However, let's be clear - this is all stuff I'm semi-guessing at from spending some time manually tracing through the code for OCSP, SafeBrowsing, and XHR, so it's highly likely I'm missing something that invalidates my answers :) Best to ask Patrick if he sees anything wrong with my assessment (or if my assessment is technically right, but still unsafe in some way or another).

Comment 31

[Mike Perry]

Hey Christoph! Thanks for picking this up!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #28)
> Probably CSP wants to get away from |ShouldLoad| too - then it would make
> sense to update/set the contentPolicyType in all the different places, hence
> allowing precise contentTypes needed for CSP.

Yes, IIRC this was the reason for the NS_CSP_[SG]etContentPolicyType() helpers in the version that Peter attached, though they ended up causing compilation issues and the CSP property bag also struck me as an ugly place to store this info. There may be a good reason why the property bag approach was taken for the CSP, though, and it probably is good to try to unify them somehow.

I'm also wondering why you wouldn't want to name the observer something that suggests it can be used generally by addons who want to block requests with more information and reliability than the nsIContentPolicy provides? That's why I chose the name "on-final-request-veto", anyways. Otherwise, you might want to ditch the observer route entirely and just try to do a direct call?

I'm not sure about the details of the individual request type questions you asked, so I'll leave that for others. But in general, the new patch seems like it will still behave properly with HTTPS-Everywhere (and other redirect cases), FWIW.

Comment 32

[Patrick McManus [:mcmanus]]

what nick says in comment 30 sgtm, though I would have to walk through a transaction to be absolutely certain.

sorry about the delay in responding.

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

It seems that we need special case handling for TYPE_OBJECT and TYPE_MEDIA which do not have a window or node which we could use as the context.
Having only a channel available, is there a possibility to query
a) the element for TYPE_MEDIA [1], and
b) the content for TYPE_OBJECT [2].

Smaug, do you know by any chance?

[1] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#1095
[2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#1307

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

It seems there is no possibility to query the correct context for TYPE_MEDIA and TYPE_OBJECT from the channel. I guess the best solution is to stuff not only the contentPolicyType into the channel, but also the correct context.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

I uploaded a new patch in Bug 418354, which also fixes this problem.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #33)
> It seems that we need special case handling for TYPE_OBJECT and TYPE_MEDIA
> which do not have a window or node which we could use as the context.
> Having only a channel available, is there a possibility to query
> a) the element for TYPE_MEDIA [1], and
> b) the content for TYPE_OBJECT [2].
> 
> Smaug, do you know by any chance?
Don't quite understand the question, but in which case do you not have window or node (and the
request isn't coming from chrome, so principal isn't system principal)?

Comment 38

[Pander]

Blocking bug https://bugzilla.mozilla.org/show_bug.cgi?id=1006881 has been fixed. Does that mean fixing this bug can make a step forward?

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Pander from comment #38)
> Blocking bug https://bugzilla.mozilla.org/show_bug.cgi?id=1006881 has been
> fixed. Does that mean fixing this bug can make a step forward?

AFAIK Bug 1006881 has not been fixed yet. Also Bug 1006881 has a lot of dependencies which we are working on at the moment - but we are getting there to get this bug fixed.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Putting this in the backlog for now - it still might take some time till Bug 1006881 is resolved.

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:freddy, could you have a look please?

For more information, please visit BugBot documentation.

Comment 43

[Frederik Braun [:freddy]]

HTTPS Everywhere was given up in light of Firefox providing "HTTPS Only" and "HTTPS First" mode. I think this can be closed.