1198572 - Add telemetry for how often HSTS would fix mixed content problems

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Richard Barnes [:rbarnes]]

Mixed content is non-HTTPS content loaded from an HTTPS page.  HSTS changes HTTP URIs to HTTPS URIs.  In principle, one might imagine allowing HSTS-upgraded loads to be exempted from mixed content blocking.

We should add telemetry to measure how often this sort of upgrade would actually turn mixed content into not-mixed content.

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.0.patch (WIP)

I think this patch has the basics of what we want here, but unfortunately it doesn't work, and in e10s it crashes.

ckershb: Can you advise on whether this looks like it's on the right track?

keeler: Thoughts on how best to query HSTS info from the child process?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Here's some copypasta from the docshell: https://dxr.mozilla.org/mozilla-central/rev/f61c3cc0eb8b7533818e7379ccc063b611015d9d/docshell/base/nsDocShell.cpp#4805

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Richard Barnes [:rbarnes] from comment #1)
> Created attachment 8652859 [details] [diff] [review]
> bug-1198572.0.patch (WIP)
> 
> I think this patch has the basics of what we want here, but unfortunately it
> doesn't work, and in e10s it crashes.
> 
> ckershb: Can you advise on whether this looks like it's on the right track?

That looks pretty much exactly to what I would have proposed. What's failing exactly? Can you paste a stacktrace?

Worth mentioning is that ::AsyncOnChannelRedirect() is called in the child and the parent in e10s, hence we have that special code [1]; but according to your question for keeler you are only interested in the child anyway, right?

Also CC'ing Tanvi, since she bascially owns mixed content.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#256

Comment 4

[Richard Barnes [:rbarnes]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> (In reply to Richard Barnes [:rbarnes] from comment #1)
> > Created attachment 8652859 [details] [diff] [review]
> > bug-1198572.0.patch (WIP)
> > 
> > I think this patch has the basics of what we want here, but unfortunately it
> > doesn't work, and in e10s it crashes.
> > 
> > ckershb: Can you advise on whether this looks like it's on the right track?
> 
> That looks pretty much exactly to what I would have proposed. What's failing
> exactly? Can you paste a stacktrace?

I was trying to access nsISiteSecurityService directly from the child process.  Adapting the copypasta from keeler appears to have made things work.


> Worth mentioning is that ::AsyncOnChannelRedirect() is called in the child
> and the parent in e10s, hence we have that special code [1]; but according
> to your question for keeler you are only interested in the child anyway,
> right?

Well, given that non-e10s still exists, we need to cover both cases.  But that's an easy enough switch.

Comment 5

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.1.patch

This patch fixes the e10s issues that were causing crashes.  (Thanks, Keeler!)  I have tested it in non-e10s mode [1], and the measurements properly show up in about:telemetry.  In e10s mode, I haven't verified directly that it works (since results don't show up in about:telemetry), but at least it doesn't crash any more.

The one slight oddity is that when display content is not blocked, it gets counted twice.  I think that's bearable, since the constant is known, and in any case, you can compare within a class (display or script) because HSTS and non-HSTS get double-counted by the same amount.  I've added a comment to document this oddity.

[1] https://ipv.sx/mixed-hsts-test/

Comment 6

[Richard Barnes [:rbarnes]]

Comment on attachment 8653004 [details] [diff] [review]
bug-1198572.1.patch

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

Couple of stray things left over from debugging the e10s issue.  Will remove while addressing any other review comments.

::: dom/security/nsMixedContentBlocker.cpp
@@ +81,5 @@
> +      return;
> +    }
> +  }
> +
> +  int result = -1;

Remove.

@@ +93,5 @@
> +      else       { result = 3; Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 3); }
> +    break;
> +  }
> +
> +  printf("\n\n>>> Telemetry::Accumulate(MIXED_CONTENT_HSTS, %d) \n\n", result);

Remove.

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 8653004 [details] [diff] [review]
bug-1198572.1.patch

+    case eMixedDisplay:
+      if (!hsts) { result = 0; Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 0); }
+      else       { result = 1; Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 1); }
+    break;
+    case eMixedScript:
+      if (!hsts) { result = 2; Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 2); }
+      else       { result = 3; Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 3); }
+    break;
+  }
+
+  printf("\n\n>>> Telemetry::Accumulate(MIXED_CONTENT_HSTS, %d) \n\n", result);
+}

Nit: indent the breaks and remove the printf.

+  nsCOMPtr<nsIURI> originalUri;
+  rv = aOldChannel->GetOriginalURI(getter_AddRefs(originalUri));
+  NS_ENSURE_SUCCESS(rv, rv);
+
   int16_t decision = REJECT_REQUEST;
   rv = ShouldLoad(nsContentUtils::InternalContentPolicyTypeToExternal(contentPolicyType),
                   newUri,
                   requestingLocation,
                   loadInfo->LoadingNode(),
                   EmptyCString(),       // aMimeGuess
-                  nullptr,              // aExtra
+                  originalUri,          // aExtra
...
+  bool redirect = false;
+  nsCOMPtr<nsIURI> originalURI = do_QueryInterface(aExtra);
+  if (originalURI) {
+    redirect = true;
+  }

This is tricky.  We don't usually mess with the aExtra parameter because it is reserved for addons to use in their own custom content policies.  We don't know what they use it for or what they set it to.  Overwriting it to null for Mixed Content Blocker's ShouldLoad() call shouldn't be a problem since the addons can't use it here.  Writing it to something else is also not a problem.  But depending on its value is a problem.  If we call AsyncOnChannelRedirect->ShouldLoad, then gecko set aExtra to originalUri.  If we call NS_ContentCheckLoadPolicy->mcb's ShouldLoad() then aExtra might be set by an addon to a uri.  In ShouldLoad() we don't know if we set the uri because of a redirect or the addon set the uri.

To add an extra parameter to ShouldLoad to indicate if the call came from AsyncOnChannelRedirect, you will need to have AsyncOnChannelRedirect call this version of ShouldLoad() directly and add a parameter to it:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#357.  I’m not entire sure that will work though.

Also, why do you nee originalURI instead of just using a bool isRedirect?  True if it went through AsyncOnChannelRedirect, false if it didn’t.

And why don’t we want to count redirects?  Are we trying to avoid counting internal HSTS redirects?  For external ones, it might make sense to include them:
https://a.com embeds https://b.com.  https://b.com redirects to http://c.com and we go through asynconchannelredirect.  http://c.com has the HSTS header set and would have gotten upgraded.  But we don’t count that.
Distinguishing between internal/external maybe tricky though so it’s okay if we just decide to ignore what happens after a redirect.  (Christoph has some changes to loadinfo in mind that will help disambiguate external/internal redirects.)

+// Note: When mixed display content is not blocked, it is counted twice.  So
+// telemetry results for display (0/1) will be roughly a factor of 2 off from
+// results for script (2/3).  Make sure to take this into account when computing
+// the overall average impact of HSTS, across both display and script.
+void AccumulateMixedContentHSTS(nsIURI* aURI, MixedContentTypes aClassification)
It’s not clear to me why display content would get double counted.  The telemetry is reported before we decide to block or load.  If it were in the ShouldLoad() and Run() method I can see why display might get double counted frequently, but it’s just in ShouldLoad().

Comment 8

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> To add an extra parameter to ShouldLoad to indicate if the call came from
> AsyncOnChannelRedirect, you will need to have AsyncOnChannelRedirect call
> this version of ShouldLoad() directly and add a parameter to it:
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsMixedContentBlocker.cpp#357.  I’m not entire sure that will work though.
> 
This will work.  But thinking about it more, I'm not sure if we should overcomplicate this code by adding more parameters to ShouldLoad for the purposes of telemetry.  What if we go ahead and rely on the aExtra parameter?  Worst case, if a popular addon sets aExtra to a uri we will get reports for redirected resources when we didn't expect to.  We could run this by Dan, but first lets see if we should just go ahead and count all redirected resources.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> This is tricky.  We don't usually mess with the aExtra parameter because it
> is reserved for addons to use in their own custom content policies.  We
> don't know what they use it for or what they set it to. 

The reason I suggested using aExtra is the following: we can set aExtra from within AsyncOnChannelRedirect() because addons *can't* call that, they can only call ContentPolicies and it's very unlikely that they will set aExtra to an nsIURI (which would lead to a false positive). If we want to be 100% sure though, we could have our own little helper interface, assign it to aExtra in asyncOnChannelRedirect() and then QI into that custom interface.

Comment 10

[Tanvi Vyas[:tanvi]]

Comment on attachment 8653004 [details] [diff] [review]
bug-1198572.1.patch

+  // We have to access HSTS information differently depending on whether or not
+  // e10s is enabled
+  if (XRE_IsParentProcess()) {
+    nsresult rv;
...

Although this code is very similar to https://dxr.mozilla.org/mozilla-central/rev/f61c3cc0eb8b7533818e7379ccc063b611015d9d/docshell/base/nsDocShell.cpp#4805, we should probably still get smaug to review this section.

Removing review flag until we settle how to handle the aExtra parameter.

Comment 11

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.2.patch

smaug: Could you please review the IPC bits of this to make sure they're correct? (calling nsSiteSecurityService from both parent and child)

Tanvi: I think the low-friction thing to do here is to change the signature of the static ShouldLoad, which is already not part of the "published" interface, even if it is public.  There's only one other caller (image/imgLoader.cpp), which I went ahead and fixed in this patch.

Comment 12

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

Comment on attachment 8654059 [details] [diff] [review]
bug-1198572.2.patch

>+// Record information on when HSTS would have made mixed content not mixed
>+// content (regardless of whether it was actually blocked)
>+void AccumulateMixedContentHSTS(nsIURI* aURI, MixedContentTypes aClassification)
>+{
>+  bool hsts = false;
>+
>+  // We have to access HSTS information differently depending on whether or not
>+  // e10s is enabled
>+  if (XRE_IsParentProcess()) {
>+    nsresult rv;
>+    nsCOMPtr<nsISiteSecurityService> sss = do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
>+    if (NS_FAILED(rv)) { return; }
if (NS_FAILED(rv)) {
  return;
}


>+    rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aURI, 0, &hsts);
>+    if (NS_FAILED(rv)) { return; }
ditto



>+  } else {
>+    mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton();
>+    mozilla::ipc::URIParams uri;
>+    SerializeURI(aURI, uri);
>+    if (!cc->SendIsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri, 0, &hsts)) {
>+      return;
>+    }
>+  }
Not about this bug, but odd that we just don't hide the multiprocess setup inside nsISiteSecurityService implementation.


But, SendIsSecureURI is synchronous. We block child process while waiting for response from parent and that can
be rather bad for performance. How often is this code called, and could we possible do all the telemetry stuff on parent side by sending the URL
asynchronously to parent.



>+
>+  switch (aClassification) {
>+    case eMixedDisplay:
>+      if (!hsts) { Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 0); }
>+      else       { Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 1); }
if (expr) {
  ...
} else {
  ...
}
>+  bool redirect = false;
>+  nsCOMPtr<nsIURI> originalURI = do_QueryInterface(aExtra);
>+  if (originalURI) {
>+    redirect = true;
>+  }
I don't see 'redirect' variable being used anywhere

Comment 13

[Tanvi Vyas[:tanvi]]

Comment on attachment 8654059 [details] [diff] [review]
bug-1198572.2.patch

-  rv = ShouldLoad(nsContentUtils::InternalContentPolicyTypeToExternal(contentPolicyType),
+  rv = ShouldLoad(true, // aIsRedirect
+                  false, // aHadInsecureImageRedirect
+                  nsContentUtils::InternalContentPolicyTypeToExternal(contentPolicyType),
                   newUri,
                   requestingLocation,
                   loadInfo->LoadingNode(),
                   EmptyCString(),       // aMimeGuess
-                  nullptr,              // aExtra
+                  nullptr,          // aExtra
Fix indenting of inline comments.


 NS_IMETHODIMP
 nsMixedContentBlocker::ShouldLoad(uint32_t aContentType,
                                   nsIURI* aContentLocation,
                                   nsIURI* aRequestingLocation,
                                   nsISupports* aRequestingContext,
                                   const nsACString& aMimeGuess,
                                   nsISupports* aExtra,
                                   nsIPrincipal* aRequestPrincipal,
                                   int16_t* aDecision)
 {
   // We pass in false as the first parameter to ShouldLoad(), because the
   // callers of this method don't know whether the load went through cached
   // image redirects.  This is handled by direct callers of the static
   // ShouldLoad.
-  nsresult rv = ShouldLoad(false,   //aHadInsecureImageRedirect
+  nsresult rv = ShouldLoad(false,   // aIsRedirect

Fix this comment to talk about both parameters and why we are passing false.  Also add a space before aHadInsecureImageRedirect

+  bool redirect = false;
+  nsCOMPtr<nsIURI> originalURI = do_QueryInterface(aExtra);
+  if (originalURI) {
+    redirect = true;
+  }
 You no longer need this code.


+   * @param aIsRedirec
+   *        boolean flag indicating whether this request is a redirect
Add a t after redirect


diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -7259,16 +7259,22 @@
     "description": "Accumulates type of content (mixed, mixed passive, unmixed) per page load"
   },
   "MIXED_CONTENT_UNBLOCK_COUNTER": {
     "expires_in_version": "never",
     "kind": "enumerated",
     "n_values": 3,
     "description": "A simple counter of daily mixed-content unblock operations and top documents loaded"
   },
+  "MIXED_CONTENT_HSTS": {
+    "expires_in_version": "never",
+    "kind": "enumerated",
+    "n_values": 10,
+    "description": "How often would blocked mixed content be allowed if HSTS upgrades were allowed? 1=display/no-HSTS, 2=display/HSTS, 3=active/no-HSTS, 4=active/HSTS"
+  },

This will need to go through a telemetry privacy review, and at that time they will probably tell you set an expiration.  You can put it 15 versions out.  At that point, we will likely know if we need to make changes to hsts/mixedcontentblocker.

diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -639,17 +639,18 @@ ShouldLoadCachedImage(imgRequest* aImgRe
...
-      rv = nsMixedContentBlocker::ShouldLoad(insecureRedirect,
+      rv = nsMixedContentBlocker::ShouldLoad(false, // not a redirect
+                                             insecureRedirect,
                                              nsIContentPolicy::TYPE_IMAGE,
                                              contentLocation,
                                              requestingLocation,
                                              aLoadingContext,
                                              EmptyCString(), //mime guess
                                              nullptr,
                                              aLoadingPrincipal,
                                              &decision);

This is very confusing.  The first parameter says its not a redirect and the second says whether the request went through an insecure (http) redirect.  The first parameter probably depends on the value of the second.  I'd have to look back at this code a little closer to remind myself exactly how it works.

But before I do that, can you tell me again why we don't want to to count redirects?

Comment 14

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.3.patch

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #12)
> Comment on attachment 8654059 [details] [diff] [review]
> bug-1198572.2.patch
> 
> >+  } else {
> >+    mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton();
> >+    mozilla::ipc::URIParams uri;
> >+    SerializeURI(aURI, uri);
> >+    if (!cc->SendIsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri, 0, &hsts)) {
> >+      return;
> >+    }
> >+  }
> Not about this bug, but odd that we just don't hide the multiprocess setup
> inside nsISiteSecurityService implementation.

This is something we should probably fix in the long term (but as a follow-on to this bug).  We're thinking about having MCB make use of HSTS information, which would mean tons of sync IPC calls in the current architecture.  If we go down that road, we probably want to make nsSiteSecurityService handle the IPC stuff more elegantly, e.g., by keeping a cache in the child process.  But like I said, follow-on.


> But, SendIsSecureURI is synchronous. We block child process while waiting
> for response from parent and that can
> be rather bad for performance. How often is this code called, and could we
> possible do all the telemetry stuff on parent side by sending the URL
> asynchronously to parent.

Thanks for the guidance here.  I moved the telemetry stuff to a static method on nsMCB, and indirected that through an async IPC channel.


(In reply to Tanvi Vyas [:tanvi] from comment #13)
> Comment on attachment 8654059 [details] [diff] [review]
> bug-1198572.2.patch
> 
> diff --git a/toolkit/components/telemetry/Histograms.json
> b/toolkit/components/telemetry/Histograms.json
> --- a/toolkit/components/telemetry/Histograms.json
> +++ b/toolkit/components/telemetry/Histograms.json
> @@ -7259,16 +7259,22 @@
>      "description": "Accumulates type of content (mixed, mixed passive,
> unmixed) per page load"
>    },
>    "MIXED_CONTENT_UNBLOCK_COUNTER": {
>      "expires_in_version": "never",
>      "kind": "enumerated",
>      "n_values": 3,
>      "description": "A simple counter of daily mixed-content unblock
> operations and top documents loaded"
>    },
> +  "MIXED_CONTENT_HSTS": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "description": "How often would blocked mixed content be allowed if
> HSTS upgrades were allowed? 1=display/no-HSTS, 2=display/HSTS,
> 3=active/no-HSTS, 4=active/HSTS"
> +  },
> 
> This will need to go through a telemetry privacy review, and at that time
> they will probably tell you set an expiration.  You can put it 15 versions
> out.  At that point, we will likely know if we need to make changes to
> hsts/mixedcontentblocker.

I don't really see any way this could be privacy-sensitive, but if you think review is necessary, who's the right reviewer?

I thought about putting an expiration, and decided that this is likely to be useful in perpetuity.  Even after we make a decision about the specific feature we're worried about now, this will be a useful metric for how big a deal scheme changing really is.


> diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp
> --- a/image/imgLoader.cpp
> +++ b/image/imgLoader.cpp
> @@ -639,17 +639,18 @@ ShouldLoadCachedImage(imgRequest* aImgRe
> ...
> -      rv = nsMixedContentBlocker::ShouldLoad(insecureRedirect,
> +      rv = nsMixedContentBlocker::ShouldLoad(false, // not a redirect
> +                                             insecureRedirect,
>                                               nsIContentPolicy::TYPE_IMAGE,
>                                               contentLocation,
>                                               requestingLocation,
>                                               aLoadingContext,
>                                               EmptyCString(), //mime guess
>                                               nullptr,
>                                               aLoadingPrincipal,
>                                               &decision);
> 
> This is very confusing.  The first parameter says its not a redirect and the
> second says whether the request went through an insecure (http) redirect. 
> The first parameter probably depends on the value of the second.  I'd have
> to look back at this code a little closer to remind myself exactly how it
> works.

It is correct for the aIsRedirect paramter to be false here, because this is a primary request (for the cache load).  The "insecureRedirect" parameter is historical.  However...


> But before I do that, can you tell me again why we don't want to to count
> redirects?

Now that you mention it, we probably do want to count redirects.  I had been worried about multiple counting down a redirect chain, but we only count when we the decision is REJECT_REQUEST -- which means the chain goes no further.  In other words:

* Load https:...
* Redirect to https:...
* Redirect to https:...
* Redirect to http:...  <-- count this

So I think all the aIsRedirect mumbo jumbo here is unnecessary.  I've backed it out.

Comment 15

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

Comment on attachment 8656023 [details] [diff] [review]
bug-1198572.3.patch

>+  // At this point we know that the request is mixed content, and the only
>+  // question is whether we block it.  Record telemetry at this point as to
>+  // whether HSTS would have fixed things by making the content location
>+  // into an HTTPS URL.
>+  //
>+  // Note that we count this for redirects as well as primary requests. This
>+  // doesn't result in double-counting because we only count the last thing in
>+  // the chain (i.e., where the blocking happened).
>+  bool active = (classification == eMixedScript);
>+  if (XRE_IsParentProcess()) {
>+    AccumulateMixedContentHSTS(aContentLocation, active);
>+  } else {
>+    // Ask the parent process to do the same call
>+    mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton();
>+    if (cc) {
>+      mozilla::ipc::URIParams uri;
>+      SerializeURI(aContentLocation, uri);
>+      cc->SendAccumulateMixedContentHSTS(uri, active);
>+    }
>+  }
Do we know roughly how often this is called? Since if this causes lots of IPC traffic, we should probably cache the results somewhere on child side, and
update parent only occasionally.

Comment 16

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

Comment on attachment 8656023 [details] [diff] [review]
bug-1198572.3.patch

I asked #perf about telemetry.
Vladan said: "child process has its own telemetry service, there's basically no ipc from telemetry logging in child process"


So, could we possibly somehow execute IsSecureURI on child process and then just
use child process telemetry service?

Comment 17

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

Also, "GetCanRecordBase or GetCanRecordExtended (depending on whether the probe is opt-in or opt-out) should be enough to figure out if measuring needs to happen"

Comment 18

[Tanvi Vyas[:tanvi]]

(In reply to Richard Barnes [:rbarnes] from comment #14)
> Created attachment 8656023 [details] [diff] [review]
> bug-1198572.3.patch
> > +  "MIXED_CONTENT_HSTS": {
> > +    "expires_in_version": "never",
> > +    "kind": "enumerated",
> > +    "n_values": 10,
> > +    "description": "How often would blocked mixed content be allowed if
> > HSTS upgrades were allowed? 1=display/no-HSTS, 2=display/HSTS,
> > 3=active/no-HSTS, 4=active/HSTS"
> > +  },
> > 
> > This will need to go through a telemetry privacy review, and at that time
> > they will probably tell you set an expiration.  You can put it 15 versions
> > out.  At that point, we will likely know if we need to make changes to
> > hsts/mixedcontentblocker.
> 
> I don't really see any way this could be privacy-sensitive, but if you think
> review is necessary, who's the right reviewer?
> 
> I thought about putting an expiration, and decided that this is likely to be
> useful in perpetuity.  Even after we make a decision about the specific
> feature we're worried about now, this will be a useful metric for how big a
> deal scheme changing really is.
> 
Privacy review is now required for every new probe.  And the expiration too.  See the red box here https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

Comment 19

[Tanvi Vyas[:tanvi]]

Comment on attachment 8656023 [details] [diff] [review]
bug-1198572.3.patch

Now that you have taken out all the complexity in trying to not double count redirects, I thought of a case where we may double count :(

Assume https page loads https image.  https image redirects to an http-non-HSTS server.  We have mixed passive content, we record telemetry, and we likely allow the load since we don't block display by default.  Then the http-non-HSTS server redirects to http-HSTS server.  And we record telemetry again.  (Or the http-HSTS server could have gone first and redirected to the http-non-HSTS server).

This can also happen for mixed active content when protection has been disabled for this page or for the whole browser.  We know that is rare for this page based on telemetry (MIXED_CONTENT_UNBLOCK_COUNTER) but don't have telemetry for the whole browser yet (https://bugzilla.mozilla.org/show_bug.cgi?id=1150602).

I'm okay with this duplication, since we are kind of recording two separate requests that have equal potential for HSTS upgrades.  If they are both for the same server, then yes we are really double counting.  We could check the security state of the page to see if it is already broken because of mixed passive and not count if it is, but then we lose data when multiple subresources are mixed passive.  So that's not great either.

But overall, I don't like the idea of making this code more complex for the sake of telemetry.  So I think we should leave this as is, knowing that a redirect might cause multiple reports for each hop in the redirect until an "allowed" mixed resource finally loads.

We also have a problem with aHadInsecureImageRedirect=true loads - http://mxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#600.  This was an unfortunate hack we had to put in until imagelib is rewritten to use the necko cache.  Anyway, it calls mixed content blocker twice if we are loading a cached resource that involved an HTTP url in the redirect chain.  First just for the final destination uri, and then again telling MCB that the load went through an insecure URI.  In those cases we might report twice.  What can we do about this?
If a resource is considered mixed because of aHadInsecureImageRedirect=true, skip it.  If the final destination was HTTP, it would have got caught on the first call to Mixed Content Blocker.  If the final destination was HTTPS, then there is nothing to report.  We won't be able to report on the actual insecure hop because we don't have data on it anymore.  You can add that check here before you call AccumulateMixedContentHSTS https://bugzilla.mozilla.org/attachment.cgi?id=8656023&action=diff#a/dom/security/nsMixedContentBlocker.cpp_sec5.

Almost r+, but I'd like to see that last change and hear that you are okay with the redirect double counting I described above.

Comment 20

[Richard Barnes [:rbarnes]]

I would propose we go ahead and land this with the double-counting issues.  We'll start getting data that give us a rough idea, and if we want to sharpen the picture, we can fix these in a follow-on.  

In any case, the failure case I really care about is when active mixed content is blocked.  For mixed active content, the only double-counting is when the user has opted out of blocking, which I'm willing to assume is a negligible fraction of the time.

Thanks for the analysis, though.  It will be good to have this analysis in the log to look at later.

Comment 21

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.4.patch

This patch just implements Tanvi's suggestion about images.

Comment 22

[Tanvi Vyas[:tanvi]]

Comment on attachment 8657314 [details] [diff] [review]
bug-1198572.4.patch

+  // Note that we count this for redirects as well as primary requests. This
+  // doesn't result in double-counting because we only count the last thing in
+  // the chain (i.e., where the blocking happened).

Change to:
Note that we count this for redirects as well as primary requests.  There are cases (particularly for passive content) where this could cause some duplication.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1198572#c19 for more details.

Did you upload the wrong file or forget to qref?  I don't see any image changes to disregard loads that have aHadInsecureImageRedirect=true.

Comment 23

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.5.patch

(In reply to Tanvi Vyas [:tanvi] from comment #22)
> Comment on attachment 8657314 [details] [diff] [review]
> bug-1198572.4.patch
>
> Did you upload the wrong file or forget to qref?  I don't see any image
> changes to disregard loads that have aHadInsecureImageRedirect=true.

Yeah, sorry.  qref'ed and edited comment.

Comment 24

[Tanvi Vyas[:tanvi]]

Comment on attachment 8658158 [details] [diff] [review]
bug-1198572.5.patch

+  // We do not count requests aHadInsecureImageRedirect=true, since these are
+  // just an artifact of the image caching system.
+  bool active = (classification == eMixedScript);
+  if (aHadInsecureImageRedirect) {

if (!aHadInsecureImageRedirect) {



+  "MIXED_CONTENT_HSTS": {
+    "expires_in_version": "never",
+    "kind": "enumerated",
+    "n_values": 10,
+    "description": "How often would blocked mixed content be allowed if HSTS upgrades were allowed? 1=display/no-HSTS, 2=display/HSTS, 3=active/no-HSTS, 4=active/HSTS"
+  },

description should use 0-3 instead of 1-4.

Is there any followup needed from comments 16 and 17?  Also, please request privacy review per comment 18.

Comment 25

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.6.patch

(In reply to Tanvi Vyas [:tanvi] from comment #24)
> Comment on attachment 8658158 [details] [diff] [review]
> bug-1198572.5.patch
> Is there any followup needed from comments 16 and 17?

Not right now.  We will want to look at this if/when we want to start treating HSTS-upgraded content as not-mixed.

Comment 26

[Vladan Djeric (:vladan)]

Comment on attachment 8658383 [details] [diff] [review]
bug-1198572.6.patch

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

::: dom/security/nsMixedContentBlocker.cpp
@@ +913,5 @@
> +  }
> +
> +  if (!aActive) {
> +    if (!hsts) {
> +      Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS, 0);

nit: i would make these into enums

::: toolkit/components/telemetry/Histograms.json
@@ +7318,5 @@
>      "kind": "enumerated",
>      "n_values": 3,
>      "description": "A simple counter of daily mixed-content unblock operations and top documents loaded"
>    },
> +  "MIXED_CONTENT_HSTS": {

add an alert_emails field

@@ +7319,5 @@
>      "n_values": 3,
>      "description": "A simple counter of daily mixed-content unblock operations and top documents loaded"
>    },
> +  "MIXED_CONTENT_HSTS": {
> +    "expires_in_version": "never",

do we really want this to never expire? this seems like the kind of thing that doesn't need to be constantly monitored.

unless people are going to be constantly checking up on this value, i propose we set expirationto 5 versions in the future, and then extend it if we need to

@@ +7322,5 @@
> +  "MIXED_CONTENT_HSTS": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "description": "How often would blocked mixed content be allowed if HSTS upgrades were allowed? 0=display/no-HSTS, 1=display/HSTS, 2=active/no-HSTS, 3=active/HSTS"

the description doesn't match the list of enum values?

Comment 27

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.7.patch

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #26)
> Comment on attachment 8658383 [details] [diff] [review]
> bug-1198572.6.patch
> 
> Review of attachment 8658383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +7319,5 @@
> >      "n_values": 3,
> >      "description": "A simple counter of daily mixed-content unblock operations and top documents loaded"
> >    },
> > +  "MIXED_CONTENT_HSTS": {
> > +    "expires_in_version": "never",
> 
> do we really want this to never expire? this seems like the kind of thing
> that doesn't need to be constantly monitored.
> 
> unless people are going to be constantly checking up on this value, i
> propose we set expirationto 5 versions in the future, and then extend it if
> we need to

This was discussed in c14 https://bugzilla.mozilla.org/show_bug.cgi?id=1198572#c14

Basically, this is something that will be useful in the long run, and in particular, it will be useful to have longitudinal data to gauge trends from.  So I think it's appropriate to keep it around.

Comment 28

[Richard Barnes [:rbarnes]]

Try run looking good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dc1317dcaa0

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10f7ba9106b6

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/10f7ba9106b6

Comment 31

[Richard Barnes [:rbarnes]]

Comment on attachment 8658776 [details] [diff] [review]
bug-1198572.7.patch

Approval Request Comment
[Feature/regressing bug #]: Telemetry for HSTS-upgradeable mixed content
[User impact if declined]: Delayed measurements
[Describe test coverage new/current, TreeHerder]: Manual functional testing, try, inbound
[Risks and why]: Low risk.  Includes some additional IPC, but all is async.
[String/UUID change made/needed]: None.

Comment 32

[Richard Barnes [:rbarnes]]

Attachment: bug-1198572.8.aurora.patch

Rebased on aurora

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8661571 [details] [diff] [review]
bug-1198572.8.aurora.patch

[Triage Comment]
I guess you want to uplift the rebased patch.

Anyway, taking it to help you.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a0ee85c4d56