799346 - Expand mixed content blocking and make it a whitelist instead of a blacklist

Status: RESOLVED DUPLICATE of bug 782654

(Core Graveyard :: Security: UI, defect, P1)

Description

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

Attachment: Expand mixed content blocking and make it a whitelist instead of a blacklist

+++ This bug was initially created as a clone of Bug #62178 +++

I will use the Splinter tool to comment on this patch, which is not ready for review yet. It will be much easier to read my comments through splinter.

Comment 1

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

Comment on attachment 669404 [details] [diff] [review]
Expand mixed content blocking and make it a whitelist instead of a blacklist

I will use the Splinter tool to comment on this patch, which is not ready for review yet. It will be much easier to read my comments through splinter.

Comment 2

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

Comment on attachment 669404 [details] [diff] [review]
Expand mixed content blocking and make it a whitelist instead of a blacklist

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

It seems like there are a lot of tests that should be added to make sure we're really blocking everything we should block.

Also, when I was working on this, I accidentally had a bug that caused active mixed content to be blocked by default. This caused multiple existing mochitests to fail. We'll have to modify a few (at least 5) existing tests before we can turn on mixed content blocking.

::: content/base/public/nsIContentPolicy.idl
@@ +12,5 @@
>  
>  /**
> + * The type of nsIContentPolicy::TYPE_*
> + */
> +typedef unsigned long nsContentPolicyType;

This changes makes code that passes these values around more self-documenting.

@@ +33,5 @@
> +   * way they treat unknown types, because existing users of TYPE_OTHER may be
> +   * converted to use new content types; all uses of TYPE_OTHER within Gecko
> +   * have been replaced already.
> +   *
> +   * const unsigned long TYPE_OTHER = 1;

This patch removes TYPE_OTHER because "other" is too meaningless to be helpful, for a nsIContentPolicy implementation that actually cares about the types of things.

However, this change will break compatibility with (JS-based) addons that use Ci.nsIContentPolicy.TYPE_OTHER. I'm not sure of the compatibility vs. sanity tradeoff.

TODO: s/unsigned long/nsContentPolicyType/

::: content/base/public/nsReferencedElement.h
@@ +54,2 @@
>     * @param aFrom the source element for context
>     * @param aURI the URI containing a hash-reference to the element

TODO: @param aCPType ...

::: content/base/src/contentSecurityPolicy.js
@@ -52,5 @@
>  
>    csp._MAPPINGS=[];
>  
> -  /* default, catch-all case */
> -  csp._MAPPINGS[cp.TYPE_OTHER]             =  cspr_sd.DEFAULT_SRC;

Review this part carefully.

@@ +323,5 @@
>              var contentPolicy = Cc["@mozilla.org/layout/content-policy;1"]
>                                    .getService(Ci.nsIContentPolicy);
> +            if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_CSP_REPORT,
> +                                         chan.URI, this._request,
> +                                         null, null, null)

Besides changing the type reported, Tanvi and I discussed this an decided we should pass the document URI as the context.

::: content/base/src/nsMixedContentBlocker.cpp
@@ +97,5 @@
>                                    nsIPrincipal* aRequestPrincipal,
>                                    int16_t* aDecision)
>  {
> +  // We access sBlockMixedScript and sBlockMixedDisplay without synchronization
> +  MOZ_ASSERT(NS_IsMainThread());

TODO: add this same assertion to the constructor.

@@ +102,4 @@
>  
> +  // Assume active (high risk) content and blocked by default
> +  MixedContentBlockedTypes classification = eBlockedMixedScript;
> +  bool rejectIfMixed = sBlockMixedScript;

Tanvi and I (and Dan?) agreed that this content policy should work like a whitelist, instead of like a blacklist.

@@ +103,5 @@
> +  // Assume active (high risk) content and blocked by default
> +  MixedContentBlockedTypes classification = eBlockedMixedScript;
> +  bool rejectIfMixed = sBlockMixedScript;
> +
> +  // TODO: mixed content on tbpl

TODO: file a dogfooding bug against tinderboxpushlog about mixed content on tbpl: If you set the active mixed content blocking pref, you cannot use parts of TBPL!

@@ +110,5 @@
> +  //
> +  // TYPE_ANIMATION_OR_EFFECT: Block because they are like stylesheets,
> +  // and because they should already be blocked by same-origin checks.
> +  //
> +  // TYPE_DTD: A DTD can contain entity definitions that expand to scripts.

This is not blocked in the original patch that landed, but it should be blocked.

@@ +120,5 @@
> +  //
> +  // TYPE_OBJECT_SUBREQUEST could actually be either active content (e.g. a
> +  // script that a plugin will execute) or display content (e.g. Flash video
> +  // content). For now, we will rely on the plugin to implement its own active
> +  // mixed content protection.

This is not blocked in the original patch that landed, and I continue not to block it, because I assume that it will break too many sites, especially Flash video sites.

@@ +124,5 @@
> +  // mixed content protection.
> +  //
> +  // TYPE_CSP_REPORT: High-risk because they directly leak information about
> +  // the content of the page, and because blocking them does not have any
> +  // negative effect on the page loading.

Tanvi and I decided that mixed-content CSP reports should be blocked because they reveal information about some contents of the encrypted page.

@@ +130,5 @@
> +  // TYPE_PING: Ping requests are POSTS, not GETs like images and media.
> +  // Also, PING requests have no bearing on the rendering or operation of
> +  // the page when used as designed, so even though they are lower risk than
> +  // scripts, blocking them is basically risk-free as far as compatibility is
> +  // concerned.

Currently, we do not block pings because they are not "active." I propose we block pings for this rationale, which is similar to the rationale used for blocking fonts.

@@ +154,5 @@
> +  // mixed-content XHR will already be blocked by that check. We suspect that
> +  // HTTPS-to-HTTP XHR with CORS is relatively uncommon. The same security
> +  // concerns mentioned above for WebSockets apply to XHR, and XHR should have
> +  // the same security properties as WebSockets w.r.t. mixed content. XHR's
> +  // handling of redirects amplifies these concerns.

It seems like Google Chrome intentionally does not block mixed  content XHR. They seem to feel like CORS is enough of a security mechanism. I disagree, as described above.

We should ask other browser makers to block XHR, ping, and fonts the same way we would with this patch.

@@ +160,5 @@
> +  MOZ_STATIC_ASSERT(TYPE_DATAREQUEST == TYPE_XMLHTTPREQUEST,
> +                    "TYPE_DATAREQUEST is not a synonym for "
> +                    "TYPE_XMLHTTPREQUEST");
> +
> +  LOG_DEBUG(("Hello %d", (PRIntn) aContentType));

TODO: clean this up.

@@ +170,5 @@
> +      return NS_OK;
> +
> +    // See the note above about mixed websockets being blocked in a different
> +    // place. Also, secure websockets use the "wss:" scheme, not "https:" like
> +    // we check for below, so we just return early to avoid that issue.

Note: I suspect (but haven't verified) that the currently-checked-in code may be blocking wss:// websockets, even though they are protected by TLS, because "wss" != "https". This is why I moved the checking of the type to be ahead of the checking of the schemes.

@@ +196,5 @@
> +    case TYPE_PING:
> +    case TYPE_SCRIPT:
> +    case TYPE_STYLESHEET:
> +    case TYPE_SUBDOCUMENT:
> +    case TYPE_XBL:

I also added XBL, which isn't blocked in the currently-checked-in code.

@@ +219,5 @@
> +  // calculating aRequestingLocation unless we have to, and to avoid blocking
> +  // the HTTPS content because of a missing (null) aRequestLocation.
> +  bool isHttps;
> +  nsresult rv = aContentLocation->SchemeIs("https", &isHttps);
> +  if (NS_FAILED(rv)) {

The original code did not block the content when SchemeIs failed. Here, I change the code to be failsafe.

@@ +231,5 @@
>    }
>  
>    // We need aRequestingLocation to pull out the scheme. If it isn't passed
>    // in, get it from the DOM node.
>    if (!aRequestingLocation) {

I find it *very* surprising that we have to do this. IMO, we should find any and all callers where this is necessary, and fix them. It is very surprising, and likely leads to bugs in nsIContentPolicy implementations.

It would be very useful to know why this is currently needed, if anybody has an idea.

@@ +241,5 @@
>      }
>      // If we still don't have a requesting location then we can't tell if
>      // this is a mixed content load.  Deny to be safe.
>      if (!aRequestingLocation) {
> +      __debugbreak();

TODO: remove this Windows-ism, which I was using for debugging.

@@ +291,5 @@
> +#if !defined(DEBUG)
> +  // ShouldLoad should have already blocked any content that needs blocking,
> +  // so don't bother double-checking it here.
> +  *aDecision = true;
> +  return NS_OK;

Besides the plugin case, I don't know when/why shouldProcess would get called without calling shouldLoad.

In the cases where shouldLoad was already called, it is wasteful to call shouldLoad again as the currently-checked-in code does. However, I admit I don't understand all the issues here. It would be great to get an explanation so I can improve the documentation of nsIContentPolicy regarding this.

Comment 3

[Devdatta Akhawe [:devd]]

> >    // We need aRequestingLocation to pull out the scheme. If it isn't passed
> >    // in, get it from the DOM node.
> >    if (!aRequestingLocation) {
> 
> I find it *very* surprising that we have to do this. IMO, we should find any
> and all callers where this is necessary, and fix them. It is very
> surprising, and likely leads to bugs in nsIContentPolicy implementations.
> 
> It would be very useful to know why this is currently needed, if anybody has
> an idea.
>

Yeah. I suggest using the (new) aRequestPrincipal argument, and getting the URI from that. This way, you don't need to break existing nsIContentPolicy consumers. 

> Besides the plugin case, I don't know when/why shouldProcess would get
> called without calling shouldLoad.
>

Did you test this? I recall that shouldLoad and shouldProcess were distinct calls: i.e., it is already the case that if shouldLoad is called, then shouldProcess isn't called. I think consumers forward the shouldProcess call to shouldLoad just to be safe. MXR tells me shouldProcess is only called in two places:
http://mxr.mozilla.org/mozilla-central/search?string=NS_CHECKCONTENTPROCESSPOLICY

Also, see https://bugzilla.mozilla.org/show_bug.cgi?id=380556


Finally, a couple of comments about the patch:

I wonder if it makes sense to break the patch up into 2: one for changes to nsIContentPolicy and one for changes to mixedcontentblocking.

I disagree with blocking pings and CSP report. I think the meaning of 'active' content and 'passive' content is very clear: active content executes inside the https context. Image loads could also have information that is important, but we block them only for passive content blocking. By the same reasoning, ping and CSP reports should also be blocked if passive content blocking is on. Changing the semantics of the blocker based on how commonly features doesn't seem right.

Comment 4

[Daniel Veditz [:dveditz]]

Overall I agree with the intent of this bug.

(In reply to Brian Smith (:bsmith) from comment #2)
> +   * const unsigned long TYPE_OTHER = 1;
> 
> This patch removes TYPE_OTHER because "other" is too meaningless to be
> helpful, for a nsIContentPolicy implementation that actually cares about the
> types of things.
> 
> However, this change will break compatibility with (JS-based) addons that
> use Ci.nsIContentPolicy.TYPE_OTHER. I'm not sure of the compatibility vs.
> sanity tradeoff.

Thank-you for keeping all the other TYPE_ values the same -- that will probably save some breakage. Removing TYPE_OTHER, however, will also result in some breakage and I think we should leave it in the IDL. It will break at least 5 addons we know of including the most popular (AdBlock Plus) and the popular NoScript. I'm sure Wladimir and Giorgio could code around it but we don't know who else we'll be breaking that's not hosted on AMO.

https://mxr.mozilla.org/addons/search?string=TYPE_OTHER&find=&findi=&filter=nsIContentPolicy&hitlimit=&tree=addons

We should leave your comment saying not to use it, and to back this up change nsContentPolicy::CheckPolicy to issue an NS_WARNING if it sees TYPE_OTHER and then unconditionally block the load without passing on to the registered content policies. THAT will stop people using it, and not break any content policy addons.

Might break some add-ons that are loading content and calling ShouldLoad with type_other though. Didn't see any in the indexed AMO addons, but it's not inconceivable that someone extending the browser will add a new type of content that doesn't fall into any of the existing categories. In real life they probably won't know to check content policies, but if they try to do the correct thing TYPE_OTHER is the only thing they can use because they can't extend the IDL. I guess they could pick a really big content type and hope someone else doesn't pick that same number. Existing content policies shouldn't blow up on the unexpected type because they already have to deal with the fact that we periodically add new types.

will try to get further later

Comment 5

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

(In reply to Daniel Veditz [:dveditz] from comment #4)
> > However, this change will break compatibility with (JS-based) addons that
> > use Ci.nsIContentPolicy.TYPE_OTHER. I'm not sure of the compatibility vs.
> > sanity tradeoff.
> 
> Thank-you for keeping all the other TYPE_ values the same -- that will
> probably save some breakage. Removing TYPE_OTHER, however, will also result
> in some breakage and I think we should leave it in the IDL.

OK, I will do so.

> We should leave your comment saying not to use it, and to back this up
> change nsContentPolicy::CheckPolicy to issue an NS_WARNING if it sees
> TYPE_OTHER and then unconditionally block the load without passing on to the
> registered content policies. THAT will stop people using it, and not break
> any content policy addons.
> 
> Might break some add-ons that are loading content and calling ShouldLoad
> with type_other though.

I think that is going to far. I think it is better to just have nsMixedContentBlocker treat all unknown (including TYPE_OTHER) content as active content by default.

> Didn't see any in the indexed AMO addons, but it's
> not inconceivable that someone extending the browser will add a new type of
> content that doesn't fall into any of the existing categories. In real life
> they probably won't know to check content policies, but if they try to do
> the correct thing TYPE_OTHER is the only thing they can use because they
> can't extend the IDL.

They *can* extend the IDL, by sending us a patch. I think there is no problem with accepting such patches. If we agree on this, I will update the patch to document that addon authors should feel free to submit patches to add new types, and that they should use TYPE_OTHER until their patch is accepted.

Comment 6

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

(In reply to Devdatta Akhawe from comment #3)
> Yeah. I suggest using the (new) aRequestPrincipal argument, and getting the
> URI from that. This way, you don't need to break existing nsIContentPolicy
> consumers. 

Thanks. Will look into this.

> Did you test this? I recall that shouldLoad and shouldProcess were distinct
> calls: i.e., it is already the case that if shouldLoad is called, then
> shouldProcess isn't called. I think consumers forward the shouldProcess call
> to shouldLoad just to be safe. MXR tells me shouldProcess is only called in
> two places:
> http://mxr.mozilla.org/mozilla-central/
> search?string=NS_CHECKCONTENTPROCESSPOLICY
> 
> Also, see https://bugzilla.mozilla.org/show_bug.cgi?id=380556

OK, I will look at this and improve the nsIContentPolicy documentation, if we all agree this is how it is supposed to work.

> I disagree with blocking pings and CSP report. I think the meaning of
> 'active' content and 'passive' content is very clear: active content
> executes inside the https context. Image loads could also have information
> that is important, but we block them only for passive content blocking. By
> the same reasoning, ping and CSP reports should also be blocked if passive
> content blocking is on. Changing the semantics of the blocker based on how
> commonly features doesn't seem right.

First, let's separate the classification issue from the "whether it should be blocked issue." I agree that we should not classify XHR/Websockets/Ping/CSP-reports as "active" content. We should create a third classification for them. We could have classifications:

* moderate security risk, high compatibility risk: images, video, audio, maybe XHR
* moderate security risk, low compatibility risk: ping, CSP reports, Websockets, maybe XHR
* High security risk: everything else (i.e. we assume something is high risk unless we explicitly put them in one of the other buckets).

I don't think our original intent was ever to block *only* active content. We would block all mixed content if it wouldn't break too many websites. I'd like to change the code to use this three-bucket classification.

Then, the second question is, where do we draw the line? Above or below "moderate security risk, low compatibility risk"? I'm proposing that we all draw the line above it; i.e. blocking that class of content by default.

Even for "moderate security risk, high compatibility risk," I'd like to continue to explore how to reduce the risk of leaking information from the encrypted page; e.g. by stripping HTTP auth and/or cookies from mixed content requests of those types, and/or automatically "fixing" requests by s/http/https/ as was proposed in another bug. The work done in bug 62178 is just the initial step. But, before we can move forward with blocking these high-compatibility-risk things, we need to have more data.

Comment 7

[Devdatta Akhawe [:devd]]

> OK, I will look at this and improve the nsIContentPolicy documentation, if
> we all agree this is how it is supposed to work.
>

Well, agreeing on how it is supposed to work is a whole new fight. I personally think we should just kill shouldProcess, or implement it properly. But that is a whole another discussion with lots of opinions. 


> First, let's separate the classification issue from the "whether it should
> be blocked issue." I agree that we should not classify
> XHR/Websockets/Ping/CSP-reports as "active" content. We should create a
> third classification for them. We could have classifications:
> 
> * moderate security risk, high compatibility risk: images, video, audio,
> maybe XHR
> * moderate security risk, low compatibility risk: ping, CSP reports,
> Websockets, maybe XHR
> * High security risk: everything else (i.e. we assume something is high risk
> unless we explicitly put them in one of the other buckets).
> 
> I don't think our original intent was ever to block *only* active content.
> We would block all mixed content if it wouldn't break too many websites. I'd
> like to change the code to use this three-bucket classification.
> 
> Then, the second question is, where do we draw the line? Above or below
> "moderate security risk, low compatibility risk"? I'm proposing that we all
> draw the line above it; i.e. blocking that class of content by default.
> 

Aah this makes more sense. We can have options for all three cases, and the default, as you say, can be above the moderate sec risk, low compat risk line


> Even for "moderate security risk, high compatibility risk," I'd like to
> continue to explore how to reduce the risk of leaking information from the
> encrypted page; e.g. by stripping HTTP auth and/or cookies from mixed
> content requests of those types, and/or automatically "fixing" requests by
> s/http/https/ as was proposed in another bug. The work done in bug 62178 is
> just the initial step. But, before we can move forward with blocking these
> high-compatibility-risk things, we need to have more data.

Agreed. I would extend measurements and possibly automatically "fixing" for ALL high compat items, even the "high security risk, high compat" (e.g., scripts)

Comment 8

[Daniel Veditz [:dveditz]]

> But, before we can move forward with blocking these high-compatibility-risk
> things, we need to have more data.

Is there telemetry in the mixedContentBlocker? We'd learn a lot from some simple counts the should not be privacy invasive:
 * The number of pages that contain mixed content
 * The number of insecure loads of each type (e.g. IMG, STYLE, etc)

Would we want the number actually loaded, or the number attempted whether we blocked it or not? Currently we probably only want the latter, but once we have blocking override we might want both to see how often users feel the need to do that to unbreak their sites. (or we could simply count the number of time the "override" option is used -- probably more direct.)

Comment 9

[Larissa Co [:lco]]

 
> Is there telemetry in the mixedContentBlocker? We'd learn a lot from some
> simple counts the should not be privacy invasive:
>  * The number of pages that contain mixed content
>  * The number of insecure loads of each type (e.g. IMG, STYLE, etc)

I'd love to have some measurements here. I spoke to Tanvi today about thinking of user-facing metrics even (though I suppose that falls under Test Pilot)

Comment 10

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

I talked with Tanvi today. This bug doesn't need to block bug 782654. I am happy to merge my changes (if any) on top of hers.

Comment 11

[Tanvi Vyas[:tanvi]]

Once again blocks 782654.  Particularly the changes to CSP to add a content type for reports.  And perhaps for other reasons as well, but decided not to investigate all the mochitest failures.

Comment 12

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

Attachment: Enhancements to nsIContentPolicy and existing nsIContentPolicy usage (removing use of TYPE_OTHER)

Comment 13

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

Attachment: Expand and refactor mixed content blocking in nsMixedContentBlocker.cpp

Tanvi, you will likely have to modify this patch to fix things the following things:

1. Use the principal instead of aRequestOrigin. This may require changing callers of ShouldLoad to pass in the principal.

2. Re-classify different types of content according to the three-category system I mentioned in a previous comment in this bug.

3. Maybe change the mixed content blocking prefs to a single multi-level pref that allows the blocking level to be set according to the categorization in #2.

Unfortunately, I will not have time to work on this for a while.

Comment 14

[Tanvi Vyas[:tanvi]]

Thanks on your work on this Brian.  I am going to create a new bug for removing TYPE_OTHER, so that it doesn't get caught up in all the other changes too nsMixedContentBlocker.

Comment 15

[Tanvi Vyas[:tanvi]]

(In reply to Brian Smith (:bsmith) from comment #12)
> Created attachment 672484 [details] [diff] [review]
> Enhancements to nsIContentPolicy and existing nsIContentPolicy usage
> (removing use of TYPE_OTHER)

This is now part of bug 802833.

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: Expand and refactor mixed content blocking in nsMixedContentBlocker.cpp - unbitrotted

I have changed a bit of this patch and un-bitrotted it so it applies on top of my patches from bug 782654.  I don't think this makes sense in a different bug, since all the work is can't be neatly separated from the work in bug 782654.

I'm going to make this bug a duplicate of bug 782654 and continue the work there.