1246540 - HSTS Priming proof of concept

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Kate McKinley [:kmckinley, :Kate]]

Create a proof-of-concept for HSTS priming that happens when the CORS preflight takes place. Importantly, this prototype does not re-order HSTS and mixed-content blocking.

The priming request must only take place if the parent document is in a secure context and the request would result in mixed-content.

The HSTS cache must be checked prior to attempting priming. If there is an entry in the HSTS cache, use that.

The request will follow the format in [1].

An entry must be added to the HSTS cache. On failure, a cache entry will be generated with a reasonable timeout (24 hours?)

The prototype must be behind a pref.

Telemetry for tracking upgraded requests.

[1] https://mikewest.github.io/hsts-priming/#modifications-fetch-priming

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive mixed content

Comment 2

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content

Comment 3

[Tanvi Vyas[:tanvi]]

You will probably need a pushPrefEnv of the mixed content about:config prefs in your tests:
security.mixed_content.block_display_content
security.mixed_content.block_active_content

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content & CORS

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: hsts_priming.patch

Hey Kate, here is a WIP patch of how we could (should) implement HSTS priming. A few things are crucial:

1) We need to bail out of MixedContentBlocker if we find out that HSTS priming applies. This is crucial so we don't accidentaly update the mixed content UI.

2) Similar to upgrade-insecure-requests we should have a flag on the LoadInfo that indiciates that the requests needs to be upgraded to https before it hits the network. This is also important so HSTS priming works within e10s.

3) Unfortunately we don't have the channel available within MixedContentBlocker::ShouldLoad(), which is unforuntate, because we can't set that flag indicaiting the upgrade in the loadInfo directly. I think it's best if the loadingDocument holds a HashMap of all requests (channel-URIs) where HSTS priming applies so we can update those requests before they hit the network.

4) Similar to upgrade-insecure-reuqests we need to surpress CORS in case we upgrade the request (we definitely need quite so many tests to cover for all those edge cases).

5) At some point we have to set that flag in the loadInfo. Unforuntately we can't do that at construction time of the loadInfo, because the loadInfo does not know about the channel-URI. That's also unfortunate. I think as things stand right now we *always* call NS_NewInputStreamChannelInternal() which also attaches the loadInfo to the channel. This might be a good point to set the flag in the loadInfo.

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming WIP

Snapshot patch with the current state. A successful priming request results in an upgrade HTTP->HTTPS.

Comment 7

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content

Converted test to browser mochitest.

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Telemetry histogram definitions

Track how many times we call HSTS priming and the result of calling priming

Comment 9

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Telemetry histogram definitions

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Expose caching for nsSiteSecurityService and allow negative result to be cached

For HSTS priming, we need to know if the entry is in the cache or not, and allow storing a negative assertion.

Comment 11

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Return out of case statement so that this actually works and doesn't throw an error.

Trying to use SpecialPowers.clearSTSData was throwing an error due to lack of return here.

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

Mixed Content should understand whether it is allowed to send a priming request, and if the request is mixed content, mark the channel for priming.

Comment 13

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsIDocument needs to cache the locations we want to send HSTS priming requests to

In order to transfer the information to the channel (where priming takes place), we need to mark the document in nsMixedContentBlocker::ShouldLoad for the URI.

Comment 14

[Kate McKinley [:kmckinley, :Kate]]

Attachment: The LoadInfo is the origin of whether a nsHttpChannel needs priming.

Comment 15

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS priming listener proxy and callbacks

Classes implementing the ListenerProxy and callback definitions used by nsHttpChannel

Comment 16

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

Block HSTS priming from working on TYPE_IMAGE for now. To be addressed in #1269815.

Comment 17

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Core HTTP Channel changes to support HSTS priming

Handle sending an HSTS priming request and success or failure

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS priming needs to propagate priming information between parent and child

Comment 19

[Kate McKinley [:kmckinley, :Kate]]

Attachment: e10s patch for HSTS priming

Comment 20

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Expose caching for nsSiteSecurityService and allow negative result to be cached

Update to include consumer

Comment 21

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Core HTTP Channel changes to support HSTS priming

Include NullHttpChannel

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8751935 [details] [diff] [review]
The LoadInfo is the origin of whether a nsHttpChannel needs priming.

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

first of all, please submit patches with 8 line context.

I'm not owner of load info, please ask :sworkment or tanvi to find a better reviewer.

::: netwerk/base/LoadInfo.h
@@ +141,5 @@
>    bool                             mForcePreflight;
>    bool                             mIsPreflight;
> +  bool                             mForcePriming;
> +  bool                             mIsPriming;
> +  bool                             mMixedContentWouldBlock;

could this be turned to a bit field?

::: netwerk/base/nsILoadInfo.idl
@@ +532,5 @@
> +  /**
> +   * Mark this LoadInfo as needing HSTS Priming
> +   */
> +  [noscript, notxpcom, nostdcall]
> +  void setHSTSPriming(in boolean wouldBlock);

please describe the argument as well

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8751922 [details] [diff] [review]
Return out of case statement so that this actually works and doesn't throw an error.

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

thanks Kate!

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8751924 [details] [diff] [review]
Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

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

Looks already pretty good to me. Let's wait for Tanvi's reviews and then you can flag me again. Thanks for working on this!

::: dom/security/nsContentSecurityManager.cpp
@@ +12,4 @@
>                    nsIContentSecurityManager,
>                    nsIChannelEventSink)
>  
> +

nit: please make sure there are no changes to empty lines here and elsewhere.

@@ +373,5 @@
> +  bool isHttp = false;
> +  aURI->SchemeIs("http", &isHttp);
> +  if (isHttp) {
> +    nsAutoCString spec;
> +    aURI->GetSpec(spec);

Probably a relict from some logging, please remove 'spec'.

@@ +381,5 @@
> +      // set it on the loadInfo
> +      aLoadInfo->SetHSTSPriming(docShell->GetDocument()->GetMixedContentWouldBlock(aURI));
> +    }
> +  }
> +

In general, I would be more comfortable having this code in the loadInfo constructor. Or is there any advantage of having that code within DoContentSecurityChecks?

::: dom/security/nsContentSecurityManager.h
@@ +31,3 @@
>  
>    static nsresult doContentSecurityCheck(nsIChannel* aChannel,
>                                           nsCOMPtr<nsIStreamListener>& aInAndOutListener);

No change here, right?

::: dom/security/nsMixedContentBlocker.cpp
@@ +370,5 @@
>    return rv;
>  }
>  
> +void
> +nsMixedContentBlocker::ClassifyContentType(uint32_t aContentType, MixedContentTypes& aClassification)

Nit: wouldn't it be better to return the MixedContentTYpe instead of returning void and having an outgoing argument?

@@ +434,5 @@
>    switch (aContentType) {
>      // The top-level document cannot be mixed content by definition
>      case TYPE_DOCUMENT:
> +      aClassification = eTopLevelDocument;
> +      break;

since we are changing that I would prefer early returns instead of a switch statement, but I leave this up to you.

@@ +505,5 @@
> +  // scripts.  Let's remember if we have seen a worker type, and reset it to the
> +  // external type in all cases right now.
> +  bool isWorkerType = aContentType == nsIContentPolicy::TYPE_INTERNAL_WORKER ||
> +                      aContentType == nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER ||
> +                      aContentType == nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER;

Why not write a helper function nsContentUtils::IsWorkerType()?

@@ +517,5 @@
> +
> +  if (classification == eTopLevelDocument || classification == eWebSocket) {
> +    *aDecision = ACCEPT;
> +    return NS_OK;
> +  }

Now that I think about it, you could even bail early for TYPE_DOCUMENT and TYPE_WEBSOCKET before calling ClassifyContentType, then you save yourself all that special handling (e.g. you woulnd't even have to add ETopLevelDocument and eWebSocket to the enum). I would prefer that.

@@ +818,5 @@
> +    bool hsts = false;
> +    bool cached = false;
> +    nsCOMPtr<nsISiteSecurityService> sss = do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aContentLocation, 0, &cached, &hsts);

longer that 80 char line, please reduce.

@@ +829,5 @@
> +    }
> +
> +    if (!cached && sSendHSTSPriming) {
> +      // add this URI as a priming location
> +      // TODO Should the be the innermost URI? really care most about host

I am not entirely certain, probably Tanvi knows. If in doubt, I would rather use the innerMostURI (innerContentLocation) though.

@@ +832,5 @@
> +      // add this URI as a priming location
> +      // TODO Should the be the innermost URI? really care most about host
> +
> +      doHSTSPriming = true;
> +      docShell->GetDocument()->AddHSTSPrimingLocation(aContentLocation);

Would it make sense to call
> docShell->GetDocument()->SetMixedContentWouldBlock(aContentLocation);
here as well since it's to only location you set doHSTSPriming to true?
Or would it even make the most sense to just set the mixedContentWouldBlock flag within AddHSTSPrimingLocation, then you could potentially remove the functionSetMixedContentWouldBlock completely.

@@ +835,5 @@
> +      doHSTSPriming = true;
> +      docShell->GetDocument()->AddHSTSPrimingLocation(aContentLocation);
> +      *aDecision = ACCEPT;
> +    }
> +  }

Can we please encapsulate all that special HSTS handling in a separate (static) function? Thanks

Comment 25

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

Comment on attachment 8751940 [details] [diff] [review]
nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed


Ok, have to read other patches to understand what the new methods on nIDocument do.

>@@ -9735,6 +9735,24 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>     return NS_ERROR_CONTENT_BLOCKED;
>   }
> 
>+  // If HSTS priming was set by nsMixedContentBlocker::ShouldLoad, and we
>+  // would block due to mixed content, go ahead and block here. If we try to
>+  // proceed with priming, we will error out later on.
>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(context);
>+  NS_ENSURE_TRUE(docShell, NS_OK);
>+  bool needsPriming = false;
>+  bool wouldBlock = false;
>+  if (docShell) {
>+    needsPriming = docShell->GetDocument()->GetHSTSPriming(aURI);
>+    wouldBlock = docShell->GetDocument()->GetMixedContentWouldBlock(aURI);
Even if unlikely, but GetDocument() may return null, so add a null check.

>+    nsAutoCString spec;
>+    aURI->GetSpec(spec);
'spec' isn't used for anything.

>+    if (needsPriming && wouldBlock) {
>+      docShell->GetDocument()->ClearHSTSPrimingLocation(aURI);
Why do we call ClearHSTSPrimingLocation here? Doesn't that mean that in case of two load using same url the latter succeeds even if it shouldn't.
Or do I misunderstand the other patch, https://bug1246540.bmoattachments.org/attachment.cgi?id=8751930
Please explain and fix if needed (and fix the other nits too) and ask review for the new patch.

Comment 26

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8751946 [details] [diff] [review]
HSTS priming needs to propagate priming information between parent and child

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

IPC changes look reasonable.

::: dom/ipc/ContentParent.cpp
@@ +4336,5 @@
>    if (!ourURI) {
>      return false;
>    }
> +  bool result = false;
> +  nsresult rv = sss->IsSecureURI(type, ourURI, flags, isSecureURI, &result);

This part looks like it belongs to the other patch with the change to the XPIDL for this method.  (Also, this variable could use a better name than “result” given that it's unused.)

::: dom/ipc/PContent.ipdl
@@ +802,4 @@
>      sync IsSecureURI(uint32_t type, URIParams uri, uint32_t flags)
>          returns (bool isSecureURI);
>  
> +    async AccumulateMixedContentHSTS(URIParams uri, bool active, bool hasHSTSPriming);

The matching change to the SendAccumulateMixedContentHSTS() call is currently in attachment 8751924 [details] [diff] [review]; they should be in the same patch when they're landed.

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8751947 [details] [diff] [review]
e10s patch for HSTS priming

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

Please resubmit the patch with an 8 line context.  Also please write down an overall description of what the patches are doing.  To do a proper review I need to know the whole picture, sorry.

Comment 28

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

Comment on attachment 8751953 [details] [diff] [review]
Expose caching for nsSiteSecurityService and allow negative result to be cached

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

This looks good. Just a few comments.

::: security/manager/ssl/nsISiteSecurityService.idl
@@ +115,4 @@
>       * @param aHost the hostname (punycode) to query for state.
>       * @param aFlags  options for this request as defined in nsISocketProvider:
>       *                  NO_PERMANENT_STORAGE
> +     * @param aCached true if we have an entry for this host, false otherwise

Maybe something more like "true if we have cached information regarding whether or not the host is HSTS and false otherwise"? (also might be good to make it clear that this only applies to HSTS)

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ +316,4 @@
>      return RemoveState(aType, aSourceURI, flags);
>    }
>  
> +  MOZ_ASSERT((aHSTSState != SecurityPropertySet || aHSTSState != SecurityPropertyNegative), "HSTS State must be SecurityPropertySet or SecurityPropertyNegative");

style nit: keep lines under 80 characters

Also, I think this assertion doesn't quite implement what it says it does - shouldn't the '!='s be '=='?

@@ +340,5 @@
>  
>  NS_IMETHODIMP
> +nsSiteSecurityService::CacheNegativeHSTSResult(nsIURI* aSourceURI, uint64_t aMaxAge)
> +{
> +  return SetHSTSState(nsISiteSecurityService::HEADER_HSTS, aSourceURI, aMaxAge, false, 0, SecurityPropertyNegative);

style nit: same here

@@ +882,3 @@
>  {
>     // Child processes are not allowed direct access to this.
>     if (!XRE_IsParentProcess() && aType != nsISiteSecurityService::HEADER_HSTS) {

Let's add an assertion that if aResult is non-null, aType is HSTS.

@@ +937,3 @@
>  {
>     // Child processes are not allowed direct access to this.
>     if (!XRE_IsParentProcess() && aType != nsISiteSecurityService::HEADER_HSTS) {

Same assertion here.

@@ +1004,5 @@
>    if (siteState.mHSTSState != SecurityPropertyUnset) {
>      SSSLOG(("Found entry for %s", host.get()));
>      bool expired = siteState.IsExpired(aType);
> +    if (!expired) {
> +      if (aCached) {

It seems like we shouldn't set aCached to true in the case where siteState.mHSTSState is SecurityPropertyKnockout, right?

@@ +1062,5 @@
>      if (siteState.mHSTSState != SecurityPropertyUnset) {
>        SSSLOG(("Found entry for %s", subdomain));
>        bool expired = siteState.IsExpired(aType);
> +      if (!expired) {
> +        if (aCached) {

Same here.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8751910 [details] [diff] [review]
Test for expected HSTS priming results for passive/active mixed content

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

Kate, I had a first look at those tests and in general they are fine. I got two questions however:
* Why are you not using plain mochitests? I don't think there is anything you can't test using a plain mochitest, or is there?
* There is a lot of duplicate code within file_priming-top-active.html and file_priming-top-passive.html as well as within file_priming.sjs and file_testserver.sjs.

I know it's more work for you refactoring those tests, but I think it's time wisely spent given this is a new security feature.
Looking at the changest of this bug I think we also need a test that covers redirects as well as a test that clearly demonstrates the integration with CORS, but it's totally fine to have separate tests for the two cases.

::: dom/security/test/hsts/browser_priming_active-mixed.js
@@ +1,3 @@
> +/*
> + * Description of the test:
> + *   Check that HSTS priming occurs correctly with active mixed conten

nit: s/conten/content

By the way, can you elaborate on the description? It seems complicated and a short description might be useful in case someone has to update the test in the future.

@@ +13,5 @@
> +  }
> +}
> +
> +var tests = {
> +  noheader: {

why is noheader not using single quotes while 'reqject-upgrade' for example uses quotes?

::: dom/security/test/hsts/browser_priming_passive-mixed.js
@@ +1,3 @@
> +/*
> + * Description of the test:
> + *   Check that HSTS priming occurs correctly with mixed display conten

same here, please add a little longer description of the test.

@@ +11,5 @@
> +  }
> +}
> +
> +// TODO make some more descriptive domains
> +// TODO redirects

please remove TODOs or implement them.

@@ +43,5 @@
> +var priming = {};
> +
> +function checkFinished() {
> +  if (Object.keys(finished).length == Object.keys(tests).length
> +       && Object.keys(priming).length == priming_count) {

nit: flip statement around and use early returns please

::: dom/security/test/hsts/file_priming.sjs
@@ +23,5 @@
> +  var query = {};
> +  request.queryString.split('&').forEach(function (val) {
> +    var [name, value] = val.split('=');
> +    query[name] = unescape(value);
> +  });

You can include:
Components.utils.importGlobalProperties(["URLSearchParams"]);
and then use:
const query = new URLSearchParams(request.queryString);

@@ +33,5 @@
> +  response.setHeader("Cache-Control", "no-cache", false);
> +
> +  // XXX
> +  // since we kill the path and query, this has to be on / ... >.<
> +  // temporarily keeping full URI

what does that comment mean? if not applicable anymore, please remove.

@@ +46,5 @@
> +    } else if (expected == 'reject-upgrade') {
> +      response.setHeader("Strict-Transport-Security", "max-age=0", false);
> +    }
> +    response.write('');
> +  } else {

nit: use early return and skip the *else*.

Comment 30

[Tanvi Vyas[:tanvi]]

Comment on attachment 8751924 [details] [diff] [review]
Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

>diff --git a/dom/security/nsContentSecurityManager.cpp b/dom/security/nsContentSecurityManager.cpp
>@@ -364,9 +365,24 @@ DoContentSecurityChecks(nsIURI* aURI, nsILoadInfo* aLoadInfo)
>                                           nsContentUtils::GetContentPolicy(),
>                                           nsContentUtils::GetSecurityManager());
>   NS_ENSURE_SUCCESS(rv, rv);
>+
>   if (NS_CP_REJECTED(shouldLoad)) {
>     return NS_ERROR_CONTENT_BLOCKED;
>   }
>+
>+  bool isHttp = false;
>+  aURI->SchemeIs("http", &isHttp);
Check the innermostURI - NS_GetInnermostURI()

>+  if (isHttp) {
>+    nsAutoCString spec;
>+    aURI->GetSpec(spec);
>+    // If the DocShell was marked for HSTS priming, propagate that to the LoadInfo
>+    nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(requestingContext);
>+    if (docShell && docShell->GetDocument()->GetHSTSPriming(aURI)) {
>+      // set it on the loadInfo
>+      aLoadInfo->SetHSTSPriming(docShell->GetDocument()->GetMixedContentWouldBlock(aURI));
>+    }
>+  }
>+
>   return NS_OK;
> }
> 
>@@ -481,7 +497,7 @@ nsContentSecurityManager::AsyncOnChannelRedirect(nsIChannel* aOldChannel,
>       rv = nsContentUtils::GetSecurityManager()->
>         CheckLoadURIWithPrincipal(oldPrincipal, newOriginalURI, flags);
>   }
>-  NS_ENSURE_SUCCESS(rv, rv);  
>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>   aCb->OnRedirectVerifyCallback(NS_OK);
>   return NS_OK;



>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>@@ -359,40 +370,9 @@ nsMixedContentBlocker::ShouldLoad(uint32_t aContentType,
>+void
>+nsMixedContentBlocker::ClassifyContentType(uint32_t aContentType, MixedContentTypes& aClassification)
> {

>@@ -454,22 +434,21 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>   switch (aContentType) {
>     // The top-level document cannot be mixed content by definition
>     case TYPE_DOCUMENT:
>-      *aDecision = ACCEPT;
>-      return NS_OK;
>+      aClassification = eTopLevelDocument;
>+      break;
>     // Creating insecure websocket connections in a secure page is blocked already
>     // in the websocket constructor. We don't need to check the blocking here
>     // and we don't want to un-block
>     case TYPE_WEBSOCKET:
>-      *aDecision = ACCEPT;
>-      return NS_OK;
>-
>+      aClassification = eWebSocket;
>+      break;
> 
>     // Static display content is considered moderate risk for mixed content so
>     // these will be blocked according to the mixed display preference
>     case TYPE_IMAGE:
>     case TYPE_MEDIA:
>     case TYPE_OBJECT_SUBREQUEST:
>-      classification = eMixedDisplay;
>+      aClassification = eMixedDisplay;
>       break;
> 
...
>+  // Assume active (high risk) content and blocked by default
>+  MixedContentTypes classification = eMixedScript;
>+  ClassifyContentType(aContentType, classification);

Since this classification a helper function now, we have to explicitly set "classification = eMixedScript;" in the helper for active content, so that the caller doesn't have to have knowledge of the default/strictest setting before calling the function.


>@@ -783,6 +803,41 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>   }
>   nsresult stateRV = securityUI->GetState(&state);
> 
>+  bool doHSTSPriming = false;
>+  // pick up the HSTS Priming from document, unless it is an image, object, or
>+  // stylesheet, in which case ignore HSTS and allow/block as before
>+  // See Bugs: 1269815, 1269814, 1269850
>+  if (isHttpScheme &&
>+      !(aContentType == TYPE_IMAGE ||
>+        aContentType == TYPE_IMAGESET ||
>+        aContentType == TYPE_OBJECT ||
>+        aContentType == TYPE_STYLESHEET ||
>+        aContentType == TYPE_DOCUMENT
>+        )
You can remove TYPE_DOCUMENT from here since you already returned early in the TYPE_DOCUMENT case

>+      ) {
>+    bool hsts = false;
>+    bool cached = false;
>+    nsCOMPtr<nsISiteSecurityService> sss = do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aContentLocation, 0, &cached, &hsts);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (hsts && sUseHSTS) {
>+      // assume we will be upgraded later
>+      *aDecision = ACCEPT;
>+      return NS_OK;
>+    }
>+
>+    if (!cached && sSendHSTSPriming) {
>+      // add this URI as a priming location
>+      // TODO Should the be the innermost URI? really care most about host
>+
I don't think it really matters, since you will pull the host out.  It depends what "AddHSTSPrimingLocation" does.  I agree with Chris that if in doubt, use the innerMost.

>+      doHSTSPriming = true;
>+      docShell->GetDocument()->AddHSTSPrimingLocation(aContentLocation);
>+      *aDecision = ACCEPT;
>+    }
>+  }

Comment 31

[Tanvi Vyas[:tanvi]]

Comment on attachment 8751924 [details] [diff] [review]
Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

>@@ -847,7 +902,12 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>          }
>       }
>     } else {
>-      *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      if (doHSTSPriming) {
>+        docShell->GetDocument()->SetMixedContentWouldBlock(aContentLocation);
>+        *aDecision = nsIContentPolicy::ACCEPT;
>+      } else {
>+        *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      }

Do we have a mixed content would warn state?  What happens for passive not-blocked content that changes the security UI?

Comment 32

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Update and consolidate tests for HSTS Priming

Updated the HSTS priming tests to include all the tests in a single file. Each test sets up and executes a test against the three servers and checks: 1) was the load blocked? 2) did we receive HSTS priming response as expected? 3) was the actual load conducted over an insecure or secure channel.

Comment 33

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Telemetry histogram definitions for HSTS priming

These two histograms tell us how HSTS priming looks in the wild.

MIXED_CONTENT_HSTS_PRIMING tells how often HSTS would be used and how often HSTS priming would be sent. We expect this to be a small percentage, and over time the number of HSTS connections would increase. This overlaps with MIXED_CONTENT_HSTS.

MIXED_CONTENT_HSTS_PRIMING_RESULT is fired every time we see a priming result and gives us the result of priming as well as whether the connection was upgraded and accepted or blocked.

Comment 34

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8755953 [details] [diff] [review]
Update and consolidate tests for HSTS Priming

HSTS priming tests are browser mochitests so that we have access to the http-on-examine-response event. Even if the load is upgraded to HTTPS, that is not reflected in the page, so we have to look at the response to determine if the load actually took place over http or https. In e10s, the http-on-* events are no longer available from a child process, which is where the code runs in a regular mochitest.

Comment 35

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

Address previous comments.

* realized ClassifyContentType was not being called anywhere else anymore due to other refactoring and reverted that method.
* switch to using innermost URI
* general cleanup

Not done:
* Setting priming flags doesn't make sense in the LoadInfo constructor since the LoadInfo is built before we call into DoSecurityChecks.
* The decision about whether we would block or not is made at a different time than the decision to prime. The current assumption is that the load will not block if we are unable to upgrade it, and if we decide later that it will block, then we mark the URI as blocking. If we use only one method, then we have to remember to call it in all places with different arguments.
* Via :rbarnes, the consensus sounds like HSTS priming will go to the same URI, just HTTP->HTTPS, so we won't only store the host, but whole URI. If there are multiple URIs needing priming in a document, only the first one should send priming, the remaining URIs should pick up the cached HSTS state.
* Putting UI update into a separate bug for follow-up. Currently, the UI will be updated and console logging will happen even if HSTS priming results in an upgrade. See 1275402

Comment 36

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content

* Cleanup test server to use URLSearchParams
* better name for confusing test case
* Clarification

Comment 37

[Kate McKinley [:kmckinley, :Kate]]

Attachment: The LoadInfo is the origin of whether a nsHttpChannel needs priming.

The LoadInfo stores information about the state of HSTS priming.

* It transfers the need for HSTS priming from the document to the HttpChannel
* In e10s (see e10s patch), the LoadInfo carries the status of priming between the parent and child processes

Comment 38

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS priming listener proxy and callbacks

The HSTS priming listener proxy handles the additional logic needed to send and process HSTS priming requests.

Comment 39

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Core HTTP Channel changes to support HSTS priming

Comment 40

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Expose caching for nsSiteSecurityService and allow negative result to be cached

Address comments from dkeeler, carry over r+

Comment 41

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

Address comments. This patch prevents HSTS priming from being used when called from nsDocShell::InternalLoad as these loads will fail if we do not have the correct security state. This will be addressed in a follow-up bug.

Comment 42

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsIDocument needs to cache the locations we want to send HSTS priming requests to

When nsMixedContentBlocker::ShouldLoad checks whether a load needs HSTS priming, it does not have access to the LoadInfo or HttpChannel, but it does have a parent nsIDocument. The LoadInfo picks up whether to send a priming request and whether mixed-content would have blocked the load from its parent nsIDocument.

Comment 43

[Kate McKinley [:kmckinley, :Kate]]

Attachment: e10s patch for HSTS priming

These changes mirror the changes in nsHttpChannel to allow HSTS priming to work in e10s

Comment 44

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

Comment on attachment 8756524 [details] [diff] [review]
nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(context);
>+  NS_ENSURE_TRUE(docShell, NS_OK);
>+  bool needsPriming = false;
>+  bool wouldBlock = false;
>+  if (docShell) {
>+    nsIDocument* document = docShell->GetDocument();
>+    NS_ENSURE_TRUE(document, NS_OK);
>+
>+    needsPriming = document->GetHSTSPriming(aURI);
>+    wouldBlock = document->GetMixedContentWouldBlock(aURI);
>+    if (needsPriming && wouldBlock) {
>+      // HSTS Priming currently disabled for InternalLoad, so we need to clear
>+      // the location that was added by nsMixedContentBlocker::ShouldLoad
>+      // Bug 1269815 will address images loaded via InternalLoad
>+      document->ClearHSTSPrimingLocation(aURI);
Why do we need to clear the state here?
I think I'm just not understanding something about the setup.
What happens if one first tries to load something and 
document->GetHSTSPriming(aURI); return true, and this method fails because of that, and right after that
load is initiated again and because of document->ClearHSTSPrimingLocation(aURI) call
document->GetHSTSPriming(aURI) return false so the load succeeds.

Comment 45

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

Comment on attachment 8756524 [details] [diff] [review]
nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

ok, thanks for explaining this on IRC.

But I'd add just one method to nsIDocument which returns enum value telling whether there should be no priming, or allow or disallowed.
But that is about some other patch in this bug.

Comment 46

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

changes for nsIDocument method refactor

Comment 47

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

refactor to accommodate nsIDocument changes

Comment 48

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsIDocument needs to cache the locations we want to send HSTS priming requests to

reduce number of addtional methods on nsIDocument to make the code easier to read

Comment 49

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

Comment on attachment 8756637 [details] [diff] [review]
nsIDocument needs to cache the locations we want to send HSTS priming requests to

>+// Enum for HSTS priming states
>+enum class HSTSPrimingState {
>+  eNO_HSTS_PRIMING = 0,
>+  eSEND_HSTS_PRIMING_WOULD_ALLOW = 1,
>+  eSEND_HSTS_PRIMING_WOULD_BLOCK = 2

aren't these a bit long names.
Would eHSTS_PRIMING_ALLOWED and eHSTS_PRIMING_BLOCKED work here?

>   /**
>+   * Check to see if a subresource we want to load requires HSTS priming
>+   * to be done.
>+   */
>+  HSTSPrimingState GetHSTSPrimingForURI(nsIURI* aContentLocation) const
>+  {
>+    if (mHSTSPrimingURIList.Contains(aContentLocation)) {
>+      if (mHSTSPrimingURIList.Get(aContentLocation)) {
>+        return HSTSPrimingState::eSEND_HSTS_PRIMING_WOULD_BLOCK;
>+      }
>+      return HSTSPrimingState::eSEND_HSTS_PRIMING_WOULD_ALLOW;
>+    }
>+    return HSTSPrimingState::eNO_HSTS_PRIMING;
>+  }
>+
>+  /**
>+   * Add a subresource to the HSTS priming list. If this URI is
>+   * not in the HSTS cache, it will trigger an HSTS priming request
>+   * when we try to load it.
>+   */
>+  void AddHSTSPrimingLocation(nsIURI* aContentLocation, bool aWouldBlock)
>+  {
>+    mHSTSPrimingURIList.Put(aContentLocation, aWouldBlock);
You should store enum values.
Then you could in GetHSTSPrimingForURI do something like
HSTSPrimingState state;
if (mHSTSPrimingURIList.Get(aContentLocation, &state)) {
 return state;
}
return eNO_HSTS_PRIMING;

no magical bool -> enum value conversions.

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8756634 [details] [diff] [review]
Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

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

Clearing review mostly because TYPE_OBJECT does not yet use AsyncOPen2() and hence will not consult nsContentSecurityManager resulting in a bypass of HSTS priming (see detailed description underneath). Do we know how often we would need HSTS priming for TYPE_OBJECT? If it's important we need a different strategy.

::: dom/security/nsContentSecurityManager.cpp
@@ +368,5 @@
>    if (NS_CP_REJECTED(shouldLoad)) {
>      return NS_ERROR_CONTENT_BLOCKED;
>    }
> +
> +  bool isHttp = false;

Please add a comment on top explaining the flow. e.g. you could mention that within nsMixedContentBlocker we bail early and then here we have to set the flag in the loadInfo ...

::: dom/security/nsMixedContentBlocker.cpp
@@ +825,5 @@
> +      doHSTSPriming = true;
> +      document->AddHSTSPrimingLocation(innerContentLocation, false);
> +        *aDecision = ACCEPT;
> +    }
> +  }

Kate, as requested in my previous review, can we please encapsulate that priming code in a separate static function to bring it in somewhat understandable format? Please add comments along the lines so people understand what's going on.

Finally I have the big picture in my head. So you call contentPolicies within nsContentSecurityManager and then set the flag on the loadInfo. One problem I see is that we haven't converted TYPE_OBJECT to use ASyncOpen2() yet [1] (see Bug 1188642). So the nsContentSecurityMnager will not be consulted for TYPE_OBJECT. Unfortunately it will still take some time till TYPE_OBJECT will use asyncOpen2() - how big of a problem is that?

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

::: dom/security/nsMixedContentBlocker.h
@@ +58,5 @@
>                               const nsACString& aMimeGuess,
>                               nsISupports* aExtra,
>                               nsIPrincipal* aRequestPrincipal,
>                               int16_t* aDecision);
> +  static void AccumulateMixedContentHSTS(nsIURI* aURI, bool aActive, bool hasHSTSPriming);

s/hasHSTSPriming/aHasHSTSPriming

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8755957 [details] [diff] [review]
Telemetry histogram definitions for HSTS priming

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

Looks good to me, but someone who knows more about Telemetry should review those bits.

Comment 52

[Allison Naaktgeboren :ally]

Comment on attachment 8755957 [details] [diff] [review]
Telemetry histogram definitions for HSTS priming

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

p=ally for data collection review. 

Thank you for remembering to add the bug number.
The number of buckets cannot be increased after the probe lands, but it looks like this is well accounted for already.

Please do not land the patch without tanvi's review. The rest of this bug and the other patches are way over my head. :)

Comment 53

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsIDocument needs to cache the locations we want to send HSTS priming requests to

Better enum value names and no more magical bool <-> enum conversions

Comment 54

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

Comment 55

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

Comment on attachment 8758834 [details] [diff] [review]
nsIDocument needs to cache the locations we want to send HSTS priming requests to

>+// Enum for HSTS priming states
>+enum class HSTSPrimingState {
>+  eNO_HSTS_PRIMING = 0,
>+  eSEND_HSTS_PRIMING_WOULD_ALLOW = 1,
>+  eSEND_HSTS_PRIMING_WOULD_BLOCK = 2
I think the naming of the last two entries is a bit odd and long.
My suggestion from comment 49 wouldn't work here?


>+   * Check to see if a subresource we want to load requires HSTS priming
>+   * to be done.
>+   */
>+  HSTSPrimingState GetHSTSPrimingForURI(nsIURI* aContentLocation) const
>+  {
>+    if (mHSTSPrimingURIList.Contains(aContentLocation)) {
>+      if (mHSTSPrimingURIList.Get(aContentLocation)) {
Why you need both Contains and Get calls?
This all would be simpler if you'd store HSTSPrimingState in the hashtable and not boolean.

>+  void AddHSTSPrimingLocation(nsIURI* aContentLocation, bool aWouldBlock)
So that latter param should have type HSTSPrimingState.
And if we don't want to allow eNO_HSTS_PRIMING here, assert about it, or
if we want to use eNO_HSTS_PRIMING as a way to unset the flag, then just remove the entry from hashtable when
eNO_HSTS_PRIMING is passed.
And I think the method name should start with Set* and not with Add*

>+  nsDataHashtable<nsURIHashKey, bool> mHSTSPrimingURIList;
So this wouldn't be nsDataHashtable<nsURIHashKey, bool>, but
nsDataHashtable<nsURIHashKey, HSTSPrimingState>
(or if that doesn't work, nsDataHashtable<nsURIHashKey, uint32_t> and convert that value when needed.)

Comment 56

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8758834 [details] [diff] [review]
nsIDocument needs to cache the locations we want to send HSTS priming requests to

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

Clearing review flag till you have addressed smaugs concerns.

Comment 57

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsIDocument needs to cache the locations we want to send HSTS priming requests to

* use enum instead of bool
* shorter names for enum values

Comment 58

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

Comment on attachment 8759770 [details] [diff] [review]
nsIDocument needs to cache the locations we want to send HSTS priming requests to

>+// Enum for HSTS priming states
>+enum class HSTSPrimingState {
>+  eNO_HSTS_PRIMING = 0,
>+  eHSTS_PRIMING_MCB_ALLOW = 1,
>+  eHSTS_PRIMING_MCB_BLOCK = 2
>+};
It is unclear to me why _MCB is needed in the names. Does it make the names easier to understand? If so, how, please explain?
If not, drop _MCB.

>+  HSTSPrimingState GetHSTSPrimingForURI(nsIURI* aContentLocation) const
>+  {
>+    HSTSPrimingState state;
>+    if (mHSTSPrimingURIList.Get(aContentLocation, &state)) {
>+        return state;
nit, 2 spaces for indentation


>+  /**
>+   * Add a subresource to the HSTS priming list. If this URI is
>+   * not in the HSTS cache, it will trigger an HSTS priming request
>+   * when we try to load it.
>+   */
>+  void AddHSTSPrimingLocation(nsIURI* aContentLocation, HSTSPrimingState state)
Nit, param name should be aState, not state

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8759770 [details] [diff] [review]
nsIDocument needs to cache the locations we want to send HSTS priming requests to

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

::: dom/base/nsIDocument.h
@@ +355,5 @@
>    /**
> +   * Check to see if a subresource we want to load requires HSTS priming
> +   * to be done.
> +   */
> +  HSTSPrimingState GetHSTSPrimingForURI(nsIURI* aContentLocation) const

Nit: Potentially better to name this GetHSTSPrimingStateForLocation

@@ +2887,5 @@
>    bool mBlockAllMixedContentPreloads;
>    bool mUpgradeInsecureRequests;
>    bool mUpgradeInsecurePreloads;
>  
> +  nsDataHashtable<nsURIHashKey, HSTSPrimingState> mHSTSPrimingURIList;

Please add a small description of why that is needed as a member and what it's used for. Something like:
Since nsMixedContentBlocker::ShouldLoad() only receives the URI of the channel to be loaded but not the channel itself, we have to pass priming state from ShouldLoad() to the channel's loadInfo by using this detour and storing the state on the document temporarily before the channel is actually created and we can pass the priming state on to the loadinfo.

Comment 60

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8756459 [details] [diff] [review]
Test for expected HSTS priming results for passive/active mixed content

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

Hey Kate, tests are really hard to follow at the moment, maybe we can simplify them; hopefully my comments will help to make things clearer, please also add more comments explaining what should happen and why. Since HSTS priming is a new, complicated and fragile logic, I wanna make sure we are testing the right things.

(Also clearing the review request for Tanvi for now.)

::: dom/security/test/hsts/browser_hsts-priming_main.js
@@ +17,5 @@
> +'use strict';
> +
> +var TOP_URI = "https://example.com/browser/dom/security/test/hsts/";
> +
> +var DEBUG = true;

nit: please set to false before landing;

@@ +26,5 @@
> +}
> +
> +var tests = {
> +  'no-ssl': {
> +    file: 'file_priming.js',

it seems the file is always 'file_priming.js'; I think you can just append it directly when starting the test instead of seeting it in every test object.

@@ +70,5 @@
> +    block_active: false,
> +    block_display: false,
> +    use_hsts: true,
> +    send_hsts_priming: true,
> +    file: 'file_priming.js',

same here, file is not variable, right? it's always the same.

@@ +114,5 @@
> +  observe: function (subject, topic, data) {
> +    switch (topic) {
> +      case 'console-api-log-event':
> +        return Observer.console_api_log_event(subject, topic, data);
> +        break;

no need for a break; underneath a return;

@@ +117,5 @@
> +        return Observer.console_api_log_event(subject, topic, data);
> +        break;
> +      case 'http-on-examine-response':
> +        return Observer.http_on_examine_response(subject, topic, data);
> +        break;

same here.

@@ +148,5 @@
> +        }
> +        checkFinished();
> +      }
> +    } catch (e) {
> +      log('error: ' + e + ":\n"+e.stack);

please also add an ok(false, ...) in the catch-block to make sure the test fails in that case.

@@ +154,5 @@
> +  },
> +  http_on_examine_response: function (subject, topic, data) {
> +    log("Got http-on-examine-response event");
> +    try {
> +      let test = null;

nit: please rename to curTest to make it easier to follow.

@@ +162,5 @@
> +        if (re.test(channel.URI.asciiSpec)) {
> +          test = tests[item];
> +          break;
> +        }
> +      }

I am curious, the tests run in sequence, no? Why not just keep a static counter around and use it as an index into the tests array? Wouldn't that be way simpler?

@@ +207,5 @@
> +}
> +
> +function clear_sts_data() {
> +  for (let test in tests) {
> +    var origin = 'http://'+tests[test].host;

nit: no need for the tmp variable, just inline into cleanUPSTSData("http://" + ...)

@@ +216,5 @@
> +function do_cleanup() {
> +  clear_sts_data();
> +
> +  Services.obs.removeObserver(Observer, "console-api-log-event");
> +  Services.obs.removeObserver(Observer, "http-on-examine-response");

Just to make sure, are all the prefs you are modifying reset to it's original value before the test ran? Otherwise you are jeopardizing our testsuite state.

@@ +247,5 @@
> +  let uri = base_uri +
> +    "?file=" + escape(file) +
> +    "&host=" + escape(host) +
> +    "&id=" + escape(test_id) +
> +    "&mimetype=" + escape(mimetype);

no need for the tmp uri, just do:
return base_uri + ...

::: dom/security/test/hsts/file_priming-top-active.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

file_primging-top-active.html as well as file_priming_top-passive.html introduce a lot of duplicate code, maybe we can simplify that as well.

::: dom/security/test/hsts/file_testserver.sjs
@@ +46,5 @@
> +    response.write('');
> +    return;
> +  }
> +
> +  if (file) {

query the file right above the if check, no need to to it at the top.

@@ +47,5 @@
> +    return;
> +  }
> +
> +  if (file) {
> +    response.setHeader("Content-Type", mimetype, false);

query the mimetype only right before this line within the |if (file)| branch.

@@ +48,5 @@
> +  }
> +
> +  if (file) {
> +    response.setHeader("Content-Type", mimetype, false);
> +    response.write(loadFromFile(unescape(file)));

return;

@@ +49,5 @@
> +
> +  if (file) {
> +    response.setHeader("Content-Type", mimetype, false);
> +    response.write(loadFromFile(unescape(file)));
> +  } else {

now you can remove that else.

Comment 61

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content & CORS

Comment 62

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsIDocument needs to cache the locations we want to send HSTS priming requests to

Updated based on comments

Comment 63

[Kate McKinley [:kmckinley, :Kate]]

Attachment: nsDocShell needs to block the load in InternalLoad until follow-up bugs are addressed

Comment 64

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

Comment 65

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

Comment 66

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8760911 [details] [diff] [review]
Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

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

Kate, I would like to see it one more time. Mostly because of the wrong assumptions regarding TYPE_IMAGE, TYPE_IMAGESET and TYPE_STYLESHEET. Please answer my question and flag me again.

::: dom/security/nsContentSecurityManager.cpp
@@ +372,5 @@
> +
> +  if (nsMixedContentBlocker::sSendHSTSPriming) {
> +    rv = nsMixedContentBlocker::MarkLoadInfoForPriming(requestingContext,
> +                                                       aLoadInfo,
> +                                                       aURI);

return nsMixedContentBlocker::MarkLoadInfoForPriming.

btw, I like that abstraction of having a static function within nsMixedContentBlocker.

::: dom/security/nsMixedContentBlocker.cpp
@@ +802,5 @@
> +      !(aContentType == TYPE_IMAGE ||
> +        aContentType == TYPE_IMAGESET ||
> +        aContentType == TYPE_OBJECT ||
> +        aContentType == TYPE_STYLESHEET
> +        )

Kate, this is important. ImageLoader was converted to use AsyncOpen2(), StyleLoader was converted to use AsyncOpen2(), so the only thing currently not using asyncOpen2() is type_object.

So, I would suppose you can remove the special case handling for everything but TYPE_OBJECT, right?

@@ +807,5 @@
> +      ) {
> +    bool hsts = false;
> +    bool cached = false;
> +    nsCOMPtr<nsISiteSecurityService> sss =
> +                          do_GetService(NS_SSSERVICE_CONTRACTID, &rv);

Nit: only 2 spaces of indentation are enogh

@@ +824,5 @@
> +      // add this URI as a priming location
> +      doHSTSPriming = true;
> +      document->AddHSTSPrimingLocation(innerContentLocation,
> +                                       HSTSPrimingState::eHSTS_PRIMING_ALLOW);
> +        *aDecision = ACCEPT;

nit: indentation

@@ +894,5 @@
>        }
>      } else {
> +      if (doHSTSPriming) {
> +        document->AddHSTSPrimingLocation(aContentLocation,
> +                                     HSTSPrimingState::eHSTS_PRIMING_BLOCK);

nit: indentation

@@ +945,5 @@
>      } else {
>        //User has not overriden the pref by Disabling protection. Reject the request and update the security state.
> +      if (doHSTSPriming) {
> +        document->AddHSTSPrimingLocation(aContentLocation,
> +                                     HSTSPrimingState::eHSTS_PRIMING_BLOCK);

nit: indentation

@@ +1107,5 @@
> +  }
> +
> +  bool isHttp = false;
> +  innerURI->SchemeIs("http", &isHttp);
> +  if (isHttp) {

if (!isHttp) {
  // there is nothing to do
  return NS_OK;
}

@@ +1113,5 @@
> +    nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
> +    if (!docShell) {
> +      return NS_OK;
> +    }
> +    nsIDocument* document = docShell->GetDocument();

docShell->GetDocument() never returns null, so you can simplify to
HSTSPrimingState status =  docshell->GetDocument()->GetHSTSPrimingStateForLocation(innerURI);

::: dom/security/nsMixedContentBlocker.h
@@ +62,5 @@
>                               nsIPrincipal* aRequestPrincipal,
>                               int16_t* aDecision);
> +  static void AccumulateMixedContentHSTS(nsIURI* aURI,
> +                                         bool aActive,
> +                                         bool hasHSTSPriming);

nit: bool aHasHSTSPriming

@@ +65,5 @@
> +                                         bool aActive,
> +                                         bool hasHSTSPriming);
> +  static nsresult MarkLoadInfoForPriming(nsISupports* aRequestingContext,
> +                                         nsILoadInfo* aLoadInfo,
> +                                         nsIURI* aURI);

Sort in the order they are consumed:
MarkLoadInfoForPriming(nsIURI* aURI,
                       nsISupports* aRequestingContext,
                       nsILoadInfo* aLoadInfo);

And add a comment explaining the different arguments, e.g.
/*
 * @param aURI ...
 * ...
 */

Comment 67

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8760905 [details] [diff] [review]
Test for expected HSTS priming results for passive/active mixed content & CORS

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

::: dom/security/test/hsts/browser_hsts-priming_main.js
@@ +95,5 @@
> +    block_display: false,
> +    use_hsts: true,
> +    send_hsts_priming: true,
> +    file: 'file_1x1.png',
> +    type: 'img',

Kate, how did that test ever succeed with the wrong assumption within nsMixedContentBlocker?
Within nsMixedContentBlocker you would only add the URL of the image load to the hashmap in the document if not type image, right? But then you would never set the priming bit within the LoadInfo and hence never send a priming request, right?

Comment 68

[Christoph Kerschbaumer [:ckerschb]]

Kate, please answer my question within comment 67!

Comment 69

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8760905 [details] [diff] [review]
Test for expected HSTS priming results for passive/active mixed content & CORS

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

As discussed over IRC, I'll review the updated version!

Comment 70

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content

Based on previous comments, TYPE_IMAGE and TYPE_STYLESHEET work now, so enable those tests.

Comment 71

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

Updated based on comments. Only blocking TYPE_OBJECT now since images and stylesheets use AsyncOpen2().

Comment 72

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to support HSTS priming

Combine all Necko patches into one for easier reviewing.

Comment 73

[Kate McKinley [:kmckinley, :Kate]]

Attachment: IPC changes needed for HSTS priming

Comment 74

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8761867 [details] [diff] [review]
Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

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

Thanks Kate for incorporating all those change requests - looks great now, r=me

Comment 75

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8761866 [details] [diff] [review]
Test for expected HSTS priming results for passive/active mixed content

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

Hey Kate, there are still too many issues so those tests can be r+ed. Please address my concerns and nits and flag me again. In general, I strongly encourage you to add plain mochitests which provide the better testing environment for what's needed to be tested here.

::: dom/security/test/hsts/browser.ini
@@ +3,5 @@
> +  file_priming-top.html
> +  file_testserver.sjs
> +  file_1x1.png
> +  file_priming.js
> +  file_stylesheet.css

is that file needed? if so, please add it to the changeset.

@@ +4,5 @@
> +  file_testserver.sjs
> +  file_1x1.png
> +  file_priming.js
> +  file_stylesheet.css
> +  file_iframe.html

is that still needed? I don't see that file anywhere.

::: dom/security/test/hsts/browser_hsts-priming_main.js
@@ +17,5 @@
> +'use strict';
> +
> +var TOP_URI = "https://example.com/browser/dom/security/test/hsts/file_priming-top.html";
> +
> +var DEBUG = true;

please set to false before landing.

@@ +47,5 @@
> +};
> +var priming_count = 2;
> +
> +var finished = {};
> +var priming = {};

please add comments what priming_count, finished and priming is used for.

@@ +128,5 @@
> +    block_display: false,
> +    use_hsts: true,
> +    send_hsts_priming: true,
> +    file: 'file_stylesheet.css',
> +    type: 'link',

it seems you should be able to infer the type from the file ending (.css, .png, .js). Can't we remove type?

@@ +151,5 @@
> +  observe: function (subject, topic, data) {
> +    switch (topic) {
> +      case 'console-api-log-event':
> +        return Observer.console_api_log_event(subject, topic, data);
> +        break;

no need to break after a return;

@@ +154,5 @@
> +        return Observer.console_api_log_event(subject, topic, data);
> +        break;
> +      case 'http-on-examine-response':
> +        return Observer.http_on_examine_response(subject, topic, data);
> +        break;

no need to break after a return;

@@ +156,5 @@
> +      case 'http-on-examine-response':
> +        return Observer.http_on_examine_response(subject, topic, data);
> +        break;
> +    }
> +    return Observer.null_func(subject, topic, data);

I am just curious: Is there any advantage of returning Observer.null_func over just throwing right here?

@@ +165,5 @@
> +  // When a load is blocked which results in an error event within a page, the
> +  // test logs to the console.
> +  console_api_log_event: function (subject, topic, data) {
> +    var message = subject.wrappedJSObject.arguments[0];
> +    var re = RegExp(/^HSTS_PRIMING: Blocked ([-\w]+).*$/);

Please add a comment what the regular expression is doing.

@@ +183,5 @@
> +      checkFinished();
> +    }
> +  },
> +  // When we see a response come back, peek at the response and make sure the
> +  // it is secure or insecure as needed. Addtionally, watch the response for

this sentence reads weired; I suppose you want to remove the 'the'.

@@ +186,5 @@
> +  // When we see a response come back, peek at the response and make sure the
> +  // it is secure or insecure as needed. Addtionally, watch the response for
> +  // priming requests.
> +  http_on_examine_response: function (subject, topic, data) {
> +    let curTest =null;

Nit: space after =;

@@ +247,5 @@
> +
> +  SpecialPowers.popPrefEnv();
> +}
> +
> +function setup(which) {

please rename to something like SetupPrefTestEnvironment

@@ +248,5 @@
> +  SpecialPowers.popPrefEnv();
> +}
> +
> +function setup(which) {
> +  waitForExplicitFinish();

What are the side effects of calling waitForExplicitFinish() multiple times? Can we place it somewhere where it's only called once?

@@ +264,5 @@
> +
> +  Services.obs.addObserver(Observer, "console-api-log-event", false);
> +  Services.obs.addObserver(Observer, "http-on-examine-response", false);
> +
> +  registerCleanupFunction(do_cleanup);

same with adding the observers and also registering the cleanup function, can we do that only once?

@@ +310,5 @@
> +//jscs:disable
> +add_task(function*() {
> +  //jscs:enable
> +  which_test = "allow_active";
> +  setup(which_test);

you are assigning to the global which_test above, I suppose there is no need to pass is as an argument in addtion, right?

@@ +363,5 @@
> +
> +  for (let test of Object.keys(test_servers)) {
> +    yield execute_test(test, "text/css");
> +  }
> +});

All this 'add tasks' is still a whole lot of duplicate code, all you need is to change the testEnvironment, right? Can you please refactor and simplify?

::: dom/security/test/hsts/file_priming-top.html
@@ +14,5 @@
> +/*
> + * Description of the test:
> + * Load an iframe in a secure context which attempts to load insecure
> + * resources. If the resource responds to HSTS priming, a cache entry should
> + * be generated (chrome test?). If it does not, the load should continue

what does 'chrome test' mean? please explain or remove.

@@ +51,5 @@
> +    + "?file=" +escape(args.file)
> +    + "&primer=" + escape(args.id)
> +    + "&mimetype=" + escape(args.mimetype)
> +    ;
> +  if (args.mimetype == 'text/css') {

thanks - the refactoring really cleared things up here eliminating all the duplicate code.

Comment 76

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content

Cleaned up and tightened up tests according to comments. Biggest change here is that instead of multiple add_task() calls, there is just one, and we loop over each of the tests, then each of the test servers.

Comment 77

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Make nsMixedContentBlocker and nsContentSecurityManager aware of HSTS Priming

Comment 78

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8762230 [details] [diff] [review]
Test for expected HSTS priming results for passive/active mixed content

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

ok, thanks kate, r=me, but again, I strongly encourage you to add some plain mochitests in addition to those tests!

::: dom/security/test/hsts/browser_hsts-priming_main.js
@@ +17,5 @@
> +'use strict';
> +
> +var TOP_URI = "https://example.com/browser/dom/security/test/hsts/file_priming-top.html";
> +
> +var DEBUG = true;

please don't forget to set to false before landing.

Comment 79

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8761868 [details] [diff] [review]
HttpChannel changes to support HSTS priming

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

Hey Kate, here is a first round of reviews. At this point I haven't looked closely at the logic, but I have two questions:
1) Why do we need the same variables (mRequireHSTSPriming, mMixedContentWouldBlock) within the httpbasechannel and the loadInfo? Wouldn't it be enough to keep that logic in the loadinfo?
2) Is priming something that should be exposed on nsIHttpChannel.idl or would it rather make sense to keep that within nsIHttpChannelInternal.idl?

::: netwerk/base/nsNetUtil.cpp
@@ +1317,5 @@
> +      nsCOMPtr<nsIURI> uri;
> +      aChannel->GetURI(getter_AddRefs(uri));
> +      nsAutoCString spec;
> +      uri->GetSpec(spec);
> +  }

I suppose some leftovers from a debug statement?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +420,5 @@
> +  bool requireHSTSPriming = false;;
> +  if (NS_SUCCEEDED(mLoadInfo->GetIsPriming(&isPriming)) && !isPriming) {
> +    rv = mLoadInfo->GetForcePriming(&requireHSTSPriming);
> +    if (NS_FAILED(rv)) {
> +      LOG(("::::HSTS Couldn't get force HSTS Priming from LoadInfo (%s)", mSpec.get()));

nit: 80 char line limit.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +527,5 @@
>    nsTArray<nsCString>               mUnsafeHeaders;
>  
> +  uint32_t                          mRequireHSTSPriming : 1;
> +  uint32_t                          mIsHSTSPrimingDone : 1;
> +  uint32_t                          mMixedContentWouldBlock : 1;

any reason those are uints and not booleans?

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.cpp
@@ +29,5 @@
> +void
> +nsHSTSPrimerListenerProxy::Shutdown()
> +{
> +  return; // NO-OP
> +}

I don't see shutdown to be called anywhere? do you even need that?

@@ +33,5 @@
> +}
> +
> +nsHSTSPrimerListenerProxy::nsHSTSPrimerListenerProxy(nsIStreamListener* aOuter,
> +                                         nsIPrincipal* aRequestingPrincipal,
> +                                         bool aWithCredentials)

nit: indendation

@@ +46,5 @@
> +}
> +
> +nsresult
> +nsHSTSPrimerListenerProxy::Init(nsIChannel* aChannel)
> +{

just asking: can aChannel be null here?

@@ +57,5 @@
> +    mRequestingPrincipal = nullptr;
> +    mOuterNotificationCallbacks = nullptr;
> +  }
> +#ifdef DEBUG
> +  mInited = true;

just asking, should mInited also be true if UpdateChannel failed?

@@ +130,5 @@
> +  rv = http->GetResponseHeader(stsHeader, hstsHeader);
> +  if (NS_FAILED(rv)) {
> +    // cache negative result
> +    return rv;
> +  }

nit: you can use NS_ENSURE_SUCCESS(rv, rv)

@@ +153,5 @@
> +    if (NS_FAILED(rv)) {
> +      nsAutoString consoleErrorCategory;
> +      nsAutoString consoleErrorTag;
> +      //nsHttpChannel::GetSTSConsoleErrorTag(failureResult, consoleErrorTag);
> +      //consoleErrorCategory = NS_LITERAL_STRING("Invalid HSTS Headers");

I suppose some leftovers from debugging.

@@ +166,5 @@
> +
> +NS_IMETHODIMP
> +nsHSTSPrimerListenerProxy::OnStopRequest(nsIRequest* aRequest,
> +                                   nsISupports* aContext,
> +                                   nsresult aStatusCode)

nit: indentation

@@ +171,5 @@
> +{
> +  MOZ_ASSERT(mInited, "nsHSTSPrimerListenerProxy has not been initialized properly");
> +  nsresult rv = mOuterListener->OnStopRequest(aRequest, aContext, aStatusCode);
> +  mOuterListener = nullptr;
> +  mOuterNotificationCallbacks = nullptr;

nit: reshuffle to
mOuterListener = nullptr;
mOuterNotificationCallbacks = nullptr;
return mOuterListener->OnStopRequest(aRequest, aContext, aStatusCode);

@@ +180,5 @@
> +nsHSTSPrimerListenerProxy::OnDataAvailable(nsIRequest* aRequest,
> +                                     nsISupports* aContext,
> +                                     nsIInputStream* aInputStream,
> +                                     uint64_t aOffset,
> +                                     uint32_t aCount)

nit: indendation

@@ +192,5 @@
> +  /*
> +  if (!mRequestApproved) {
> +    return NS_ERROR_DOM_BAD_URI;
> +  }
> +  */

is that a valid todo? If so, then please do, otherwise please remove comment.

@@ +230,5 @@
> +NS_IMETHODIMP
> +nsHSTSPrimerListenerProxy::AsyncOnChannelRedirect(nsIChannel *aOldChannel,
> +                                            nsIChannel *aNewChannel,
> +                                            uint32_t aFlags,
> +                                            nsIAsyncVerifyRedirectCallback *aCb)

nit: indendation

@@ +356,5 @@
> +    rv = http->GetLoadFlags(&flags);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    flags |= nsIRequest::LOAD_ANONYMOUS;
> +    rv = http->SetLoadFlags(flags);

nit: http->SetLoadFlags(flags | LOAD_ANONYMOUS);

@@ +417,5 @@
> +public:
> +  nsHSTSPrimerListener(nsIPrincipal* aReferrerPrincipal,
> +                       nsIHstsPrimingCallback* aCallback)
> +   : mReferrerPrincipal(aReferrerPrincipal)
> +     , mCallback(aCallback)

nit: indendation

@@ +473,5 @@
> +    // Everything worked, fire off the actual request.
> +    mCallback->OnPrimingSucceeded();
> +  } else {
> +    mCallback->OnPrimingFailed(rv);
> +  }

nit: if (NS_FAILED(rv) {
       ...
       return rv;
     }
     mCallback->OnPrimingSucceeded();
     return NS_OK;

@@ +499,5 @@
> +  if (!NS_IsInternalSameURIRedirect(aOldChannel, aNewChannel, aFlags) &&
> +      !NS_IsHSTSUpgradeRedirect(aOldChannel, aNewChannel, aFlags))
> +    return NS_ERROR_DOM_BAD_URI;
> +
> +  */

is this a valid todo, then please do, otherwise please remove comment

@@ +513,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  //NS_ENSURE_SUCCESS(status, status);
> +  if (NS_FAILED(status)) {
> +    return status;
> +  }

please uncomment NS_ENSURE_SUCCESS(status, status) and remove the NS_FAILED case.

@@ +533,5 @@
> +  bool cached;
> +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }

NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE) or just NS_ENSURE_SUCCES(rv, rv)

@@ +541,5 @@
> +
> +  rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri, 0, &cached, &hsts);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }

same here

@@ +548,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (!cached) {
> +    rv = sss->CacheNegativeHSTSResult(uri, 24*60*60);

please add a comment what the hardcoded value is doing.

@@ +562,5 @@
> +nsHSTSPrimerListener::OnDataAvailable(nsIRequest *aRequest,
> +                                         nsISupports *ctxt,
> +                                         nsIInputStream *inStr,
> +                                         uint64_t sourceOffset,
> +                                         uint32_t count)

nit: indendation

@@ +589,5 @@
> +  // XXX spec says we need to touch / instead of the URI we have. For now we need to
> +  // straighten out the test or fix the spec, so leave the full URI
> +  rv = uri->SetPath(NS_LITERAL_CSTRING(""));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  */

please remove the comment or add a leading * to every line, it's very confusing to have a mixture of * and //

@@ +605,5 @@
> +  bool cached;
> +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }

NS_ENSURE_SUCCESS(rv, rv)

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.h
@@ +18,5 @@
> +#include "nsIAsyncVerifyRedirectCallback.h"
> +#include "nsIThreadRetargetableStreamListener.h"
> +#include "mozilla/Attributes.h"
> +
> +class nsIURI;

forward declaration and inclusion of nsIURI? please remove one.

@@ +52,5 @@
> +
> +priming succeded (got sts header back with upgrade)
> +priming failed, block
> +priming failed, accept
> + */

please keep the leading * at the beginning of every line in the comment.

@@ +113,5 @@
> +  // This can get changed during redirects, unlike mRequestingPrincipal.
> +  nsCOMPtr<nsIInterfaceRequestor> mOuterNotificationCallbacks;
> +  nsCOMPtr<nsINetworkInterceptController> mInterceptController;
> +  bool mWithCredentials;
> +  //bool mRequestApproved;

if not needed, please remove.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +360,5 @@
>                                  shouldUpgrade);
>      NS_ENSURE_SUCCESS(rv, rv);
>      if (shouldUpgrade) {
> +        nsAutoCString spec;
> +        mURI->GetSpec(spec);

another leftover from a debugging statement.

@@ +438,5 @@
> +        }
> +
> +        if (mRequireHSTSPriming) {
> +            nsresult rv = nsHSTSPrimerListenerProxy::StartHSTSPriming(this, this,
> +                    getter_AddRefs(mPrimingChannel));

nit: I suppose you can reuse rv here

@@ +441,5 @@
> +            nsresult rv = nsHSTSPrimerListenerProxy::StartHSTSPriming(this, this,
> +                    getter_AddRefs(mPrimingChannel));
> +            if (mPrimingChannel) {
> +                return rv;
> +            }

move the error case before the if (mPrimingChannel) and return NS_OK to be more explicit.

@@ +2258,5 @@
>      PushRedirectAsyncFunc(
>          &nsHttpChannel::ContinueAsyncRedirectChannelToURI);
>      rv = gHttpHandler->AsyncOnChannelRedirect(this, newChannel, flags);
>  
> +

nit: what was changed here? probably just a space, please revert.

@@ +5367,5 @@
>                  nsContentUtils::IsSystemPrincipal(mLoadInfo->LoadingPrincipal())),
>                 "security flags in loadInfo but asyncOpen2() not called");
>  
> +    nsAutoCString spec;
> +    mURI->GetSpec(spec);

nit: is that needed? probably just used for you development debugging, right?

@@ +7663,5 @@
> +NS_IMETHODIMP
> +nsHttpChannel::OnPrimingSucceeded()
> +{
> +    MOZ_ASSERT(mRequireHSTSPriming, "Why did priming happen?");
> +    mIsHSTSPrimingDone = 1;

mIsHSTSPrimingDone = true;

@@ +7673,5 @@
> +        return AsyncCall(&nsHttpChannel::HandleAsyncRedirectChannelToHttps);
> +    }
> +
> +    bool wouldBlock;
> +    mLoadInfo->GetMixedContentWouldBlock(&wouldBlock);

it's not guaranteed that mLoadInfo is not null, please add check for that throughout the file.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +2243,5 @@
>      if (loadContext && loadContext->UsePrivateBrowsing())
>          flags |= nsISocketProvider::NO_PERMANENT_STORAGE;
>      nsCOMPtr<nsIURI> clone;
>      if (NS_SUCCEEDED(sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS,
> +                                      aURI, flags, nullptr, &isStsHost)) && isStsHost) {

nit: 80 chars line limit

::: netwerk/protocol/http/nsIHstsPrimingCallback.h
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I don't see that file being used anywhere. if it's not used, please remove.

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +1003,5 @@
> +NS_IMETHODIMP
> +nsViewSourceChannel::SetRequireHSTSPriming(bool aWouldBlock)
> +{
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +}

Even though I think it's nice of you to clean up the whitespaces within that file, it probably shouldn't happen within such a big changeset.

Comment 82

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Test for expected HSTS priming results for passive/active mixed content

Comment 83

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to support HSTS priming

* LoadInfo is the source of truth for HSTS priming
* cleanup / other comments

Comment 84

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8763227 [details] [diff] [review]
HttpChannel changes to support HSTS priming

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

Clearing for Jason as well. I still haven't looked closely at the logical stuff yet. Let's incorporate my suggestions and I'll have another look.

::: netwerk/base/LoadInfo.cpp
@@ +781,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +LoadInfo::SetHSTSPriming(bool wouldBlock)

nit: please prefix with 'a' - aWouldBlock

::: netwerk/base/LoadInfo.h
@@ +146,5 @@
> +
> +  uint32_t                         mForcePriming : 1;
> +  uint32_t                         mIsPriming : 1;
> +  uint32_t                         mMixedContentWouldBlock : 1;
> +  uint32_t                         mPrimingDone : 1;

Should we consider making this flags and store the whole information within one int instead of allocating 4 uints?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +110,5 @@
>    , mTransferSize(0)
>    , mDecodedBodySize(0)
>    , mEncodedBodySize(0)
>    , mRequireCORSPreflight(false)
> +  , mRequireHSTSPriming(false)

I don't see that information being queried anywhere? Is that still needed or just a remainder from a previous version?

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +314,5 @@
>      void EnsureUploadStreamIsCloneableComplete(nsresult aStatus);
>  
> +    bool GetRequireHSTSPriming();
> +    bool GetMixedContentWouldBlock();
> +    bool GetIsHSTSPrimingChannel();

I don't see that method being called anywhere.

@@ +315,5 @@
>  
> +    bool GetRequireHSTSPriming();
> +    bool GetMixedContentWouldBlock();
> +    bool GetIsHSTSPrimingChannel();
> +    bool GetHSTSPrimingDone();

It seems those methods are only called internally from httpchannel, hence it doesn't make sense to me to keep them around; you can just access the information directly within mLoadInfo.

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.cpp
@@ +17,5 @@
> +                  nsIInterfaceRequestor, nsIThreadRetargetableStreamListener)
> +
> +nsHSTSPrimerListenerProxy::nsHSTSPrimerListenerProxy(nsIStreamListener* aOuter,
> +                                         nsIPrincipal* aRequestingPrincipal,
> +                                         bool aWithCredentials)

nit: indendation

@@ +30,5 @@
> +}
> +
> +nsresult
> +nsHSTSPrimerListenerProxy::Init(nsIChannel* aChannel)
> +{

can aChannel be null? Please add a check if so.

@@ +69,5 @@
> +  if (!http) {
> +    // cache negative result
> +    // log we were blocked & report no HTTPS
> +    return NS_ERROR_DOM_BAD_URI;
> +  }

nit: NS_ENSURE_TRUE(http, NS_ERROR_DOM_BAD_URI);

@@ +72,5 @@
> +    return NS_ERROR_DOM_BAD_URI;
> +  }
> +
> +  nsCOMPtr<nsIURI> requestURI;
> +  rv = http->GetURI(getter_AddRefs(requestURI));

might the requestURI ever be null here?

@@ +92,5 @@
> +    return NS_OK;
> +  }
> +
> +  uint32_t flags =
> +      NS_UsePrivateBrowsing(http) ? nsISocketProvider::NO_PERMANENT_STORAGE : 0;

nit: please use the exact flag rather than 0 - is it 'PROXY_RESOLVES_HOST'? If we ever update PROXY_RESOLVES_HOST to be not null but 1, then you still have that hardcoded value within here.

@@ +110,5 @@
> +
> +  nsAutoCString stsHeader("Strict-Transport-Security");
> +  // Check the HSTS Priming Header
> +  nsAutoCString hstsHeader;
> +  rv = http->GetResponseHeader(stsHeader, hstsHeader);

simplify to: http->GetResponseHeader(NS_LITERAL_CSTRING("Strict-Transport-Security"),...)

@@ +129,5 @@
> +
> +  rv = sss->ProcessHeader(nsISiteSecurityService::HEADER_HSTS,
> +      requestURI, hstsHeader.get(), sslStatus, flags,
> +      nullptr, nullptr, &failureResult);
> +  NS_ENSURE_SUCCESS(rv, rv);

nit: if that is the last operation within the method you can just do:
return sss->ProcessHeader(...);

@@ +228,5 @@
> +      return NS_ERROR_DOM_BAD_URI;
> +    }
> +
> +    // TODO if redirect is HTTPS->HTTP, then we know it is not going to work,
> +    // so update the cache and cancel

if that is a valid todo, then please do, otherwise remove comment.

@@ +286,5 @@
> +    rv = nsContentUtils::GetSecurityManager()->
> +      CheckLoadURIWithPrincipal(mRequestingPrincipal, originalURI,
> +                                nsIScriptSecurityManager::STANDARD);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

Mhm, both of those checks should be performed within the contentSecurityManager when calling asyncOpen2() - what are the cases where that doesn't happen?

@@ +299,5 @@
> +
> +  // Make cookie-less if needed. We don't need to do anything here if the
> +  // channel was opened with AsyncOpen2, since then AsyncOpen2 will take
> +  // care of the cookie policy for us.
> +  if (!loadInfo || !loadInfo->GetEnforceSecurity()) {

nit: query the loadInfo right above that check

@@ +385,5 @@
> +{
> +  // Only internal redirects allowed for now.
> +  if (!NS_IsInternalSameURIRedirect(aOldChannel, aNewChannel, aFlags) &&
> +      !NS_IsHSTSUpgradeRedirect(aOldChannel, aNewChannel, aFlags))
> +    return NS_ERROR_DOM_BAD_URI;

nit: please use curly braces around the body of the if statement.

@@ +432,5 @@
> +
> +  if (!cached) {
> +    /* Cache a negative HSTS result for 1 day */
> +    rv = sss->CacheNegativeHSTSResult(uri, 24*60*60);
> +    NS_ENSURE_SUCCESS(rv, rv);

nit:

if (!cached) {
  ...
  return sss->CacheNegativeHSTSResult(uri, 24*60*60);
}

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.h
@@ +9,5 @@
> +
> +#include "nsIStreamListener.h"
> +#include "nsIInterfaceRequestor.h"
> +#include "nsCOMPtr.h"
> +#include "nsString.h"

do you need nsString to be included here?

@@ +10,5 @@
> +#include "nsIStreamListener.h"
> +#include "nsIInterfaceRequestor.h"
> +#include "nsCOMPtr.h"
> +#include "nsString.h"
> +#include "nsIURI.h"

same here, do you need that inclusion?

@@ +11,5 @@
> +#include "nsIInterfaceRequestor.h"
> +#include "nsCOMPtr.h"
> +#include "nsString.h"
> +#include "nsIURI.h"
> +#include "nsTArray.h"

and here as well.

@@ +12,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsString.h"
> +#include "nsIURI.h"
> +#include "nsTArray.h"
> +#include "nsIInterfaceRequestor.h"

you already include that file 4 lines above.

@@ +14,5 @@
> +#include "nsIURI.h"
> +#include "nsTArray.h"
> +#include "nsIInterfaceRequestor.h"
> +#include "nsIChannelEventSink.h"
> +#include "nsIAsyncVerifyRedirectCallback.h"

i don't see any usage of that inclusion.

@@ +16,5 @@
> +#include "nsIInterfaceRequestor.h"
> +#include "nsIChannelEventSink.h"
> +#include "nsIAsyncVerifyRedirectCallback.h"
> +#include "nsIThreadRetargetableStreamListener.h"
> +#include "mozilla/Attributes.h"

nit: please sort all you inclusions in alphabetical order.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +429,5 @@
> +                    HSTSPrimingResult::eHSTS_PRIMING_ALREADY_UPGRADED);
> +            mLoadInfo->ClearHSTSPriming();
> +        }
> +
> +        if (GetRequireHSTSPriming()) {

Important question, can GetRequireHSTSPriming() change state between the if here and the if 10 lines above? If not, then it would probably make sense to simlify the whole block.

@@ +7655,5 @@
> +nsHttpChannel::OnPrimingSucceeded()
> +{
> +    MOZ_ASSERT(mLoadInfo, "LoadInfo must not be null for HSTS priming");
> +    MOZ_ASSERT(GetRequireHSTSPriming(), "Why did priming happen?");
> +    mLoadInfo->SetPrimingDone(true);

you sure you always have mLoadInfo? In some cases you do a null check and in other you don't, please do it everywhere.

remember, MOZ_ASSERT() becomes a no-op for release builds and addons make use of httpchannel not necessarily having a loadinfo attached.

@@ +7678,5 @@
> +nsHttpChannel::OnPrimingFailed(nsresult aError)
> +{
> +    MOZ_ASSERT(mLoadInfo, "LoadInfo must not be null for HSTS priming");
> +    MOZ_ASSERT(GetRequireHSTSPriming(), "Why did priming happen?");
> +    mLoadInfo->SetPrimingDone(true);

same here, probably it just makes sense to bail out of that function early if there is no loadInfo.

Comment 85

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to support HSTS priming

Comment 86

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to support HSTS priming

> ::: netwerk/base/LoadInfo.h
> @@ +146,5 @@
> > +
> > +  uint32_t                         mForcePriming : 1;
> > +  uint32_t                         mIsPriming : 1;
> > +  uint32_t                         mMixedContentWouldBlock : 1;
> > +  uint32_t                         mPrimingDone : 1;
> 
> Should we consider making this flags and store the whole information within
> one int instead of allocating 4 uints?

it doesn't allocate 4 uints, it is a bitfield, and follows existing practice in these files
http://en.cppreference.com/w/cpp/language/bit_field


> @@ +315,5 @@
> >  
> > +    bool GetRequireHSTSPriming();
> > +    bool GetMixedContentWouldBlock();
> > +    bool GetIsHSTSPrimingChannel();
> > +    bool GetHSTSPrimingDone();
> 
> It seems those methods are only called internally from httpchannel, hence it
> doesn't make sense to me to keep them around; you can just access the
> information directly within mLoadInfo.

Moving these to protected. There is still null guarding that is needed since mLoadInfo may be null.

> @@ +92,5 @@
> > +    return NS_OK;
> > +  }
> > +
> > +  uint32_t flags =
> > +      NS_UsePrivateBrowsing(http) ? nsISocketProvider::NO_PERMANENT_STORAGE : 0;
> 
> nit: please use the exact flag rather than 0 - is it 'PROXY_RESOLVES_HOST'?
> If we ever update PROXY_RESOLVES_HOST to be not null but 1, then you still
> have that hardcoded value within here.

0 is no flag, PROXY_RESOLVES_HOST is 1. There is no named value for no flag.

> 
> @@ +110,5 @@
> > +
> > +  nsAutoCString stsHeader("Strict-Transport-Security");
> > +  // Check the HSTS Priming Header
> > +  nsAutoCString hstsHeader;
> > +  rv = http->GetResponseHeader(stsHeader, hstsHeader);
> 
> simplify to:
> http->GetResponseHeader(NS_LITERAL_CSTRING("Strict-Transport-Security"),...)
> 
> @@ +129,5 @@
> > +
> > +  rv = sss->ProcessHeader(nsISiteSecurityService::HEADER_HSTS,
> > +      requestURI, hstsHeader.get(), sslStatus, flags,
> > +      nullptr, nullptr, &failureResult);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> nit: if that is the last operation within the method you can just do:
> return sss->ProcessHeader(...);
> 
> @@ +228,5 @@
> > +      return NS_ERROR_DOM_BAD_URI;
> > +    }
> > +
> > +    // TODO if redirect is HTTPS->HTTP, then we know it is not going to work,
> > +    // so update the cache and cancel
> 
> if that is a valid todo, then please do, otherwise remove comment.
> 
> @@ +286,5 @@
> > +    rv = nsContentUtils::GetSecurityManager()->
> > +      CheckLoadURIWithPrincipal(mRequestingPrincipal, originalURI,
> > +                                nsIScriptSecurityManager::STANDARD);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +  }
> 
> Mhm, both of those checks should be performed within the
> contentSecurityManager when calling asyncOpen2() - what are the cases where
> that doesn't happen?

UpdateChannel is called after a redirect, so these checks are happening on the redirected URI.

Comment 87

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8764992 [details] [diff] [review]
HttpChannel changes to support HSTS priming

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

I think at this point it's up to a Necko peer - thanks Kate for working on this!

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +361,5 @@
>    // for a possible synthesized response instead.
>    bool ShouldIntercept(nsIURI* aURI = nullptr);
>  
> +
> +  // Methods for 

nit: I think you haven't finished that comment, right?

@@ +365,5 @@
> +  // Methods for 
> +  bool GetRequireHSTSPriming();
> +  bool GetMixedContentWouldBlock();
> +  bool GetIsHSTSPrimingChannel();
> +  bool GetHSTSPrimingDone();

nit: if you wanna keep them as internal methods, please remove the leading 'Get' since they usually indicate class interfaces for quering information.

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.cpp
@@ +18,5 @@
> +
> +nsHSTSPrimerListenerProxy::nsHSTSPrimerListenerProxy(nsIStreamListener* aOuter,
> +                                                     nsIPrincipal* aRequestingPrincipal)
> +  : mOuterListener(aOuter),
> +  mRequestingPrincipal(aRequestingPrincipal)

nit: move the comma from the end of the line to the beginning of the next line:
  : mOuterListener(aOuter)
  , mRequestingPrincipal(aRequestingPrincipal)

@@ +68,5 @@
> +    return NS_OK;
> +  }
> +
> +  uint32_t flags =
> +      NS_UsePrivateBrowsing(http) ? nsISocketProvider::NO_PERMANENT_STORAGE : 0;

nit: move the flags declaration closer to usage, or even inline within sss->ProcessHeader()

@@ +101,5 @@
> +  uint32_t failureResult;
> +
> +  return sss->ProcessHeader(nsISiteSecurityService::HEADER_HSTS,
> +      requestURI, hstsHeader.get(), sslStatus, flags,
> +      nullptr, nullptr, &failureResult);

nit: indendation

@@ +327,5 @@
> +public:
> +  nsHSTSPrimerListener(nsIPrincipal* aReferrerPrincipal,
> +                       nsIHstsPrimingCallback* aCallback)
> +   : mReferrerPrincipal(aReferrerPrincipal)
> +     , mCallback(aCallback)

nit: align colon and comma
: mReferrerPrincipal(aReferrerPrincipal)
, mCallback(aCallback)

@@ +374,5 @@
> +
> +NS_IMETHODIMP
> +nsHSTSPrimerListener::OnStopRequest(nsIRequest *aRequest,
> +                                       nsISupports *aContext,
> +                                       nsresult aStatus)

nit: indendation

@@ +384,5 @@
> +NS_IMETHODIMP
> +nsHSTSPrimerListener::AsyncOnChannelRedirect(nsIChannel *aOldChannel,
> +                                                nsIChannel *aNewChannel,
> +                                                uint32_t aFlags,
> +                                                nsIAsyncVerifyRedirectCallback *callback)

nit: indendation

@@ +389,5 @@
> +{
> +  // Only internal redirects allowed for now.
> +  if (!NS_IsInternalSameURIRedirect(aOldChannel, aNewChannel, aFlags) &&
> +      !NS_IsHSTSUpgradeRedirect(aOldChannel, aNewChannel, aFlags))
> +    return NS_ERROR_DOM_BAD_URI;

nit: please add curly braces around the body of the if-block

Comment 88

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to implement HSTS priming

Comment 89

[Honza Bambas (:mayhemer)]

Comment on attachment 8766046 [details] [diff] [review]
HttpChannel changes to implement HSTS priming

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

r- for general lack of comments on new classes/methods/members/properties/attributes and no description of the patch.  Please add both.  Also it would be helpful to have a complete patch for reference (all patches folded together).

::: netwerk/base/LoadInfo.h
@@ +76,5 @@
>    // separate request. I.e. not for a redirect or an inner channel, but
>    // when a separate request is made with the same security properties.
>    already_AddRefed<nsILoadInfo> CloneForNewRequest() const;
> +  // like CloneForNewRequest(), and adds anything needed for HSTS Priming
> +  already_AddRefed<nsILoadInfo> CloneForPriming() const;

CloneForHSTSPriming please.

@@ +81,4 @@
>  
>    void SetIsPreflight();
>  
> +  void SetIsPriming();

SetIsHSTSPriming please

@@ +156,5 @@
> +
> +  uint32_t                         mForcePriming : 1;
> +  uint32_t                         mIsPriming : 1;
> +  uint32_t                         mMixedContentWouldBlock : 1;
> +  uint32_t                         mPrimingDone : 1;

you can also use bool : 1

are these changing during the LoadInfo object lifetime?  (playing with making them bool const : 1)

::: netwerk/base/nsILoadInfo.idl
@@ +540,5 @@
>  
>    /**
> +   * When this request would be mixed-content and we do not have an
> +   * entry in the HSTS cache, we send an HSTS priming request to
> +   * determine if it is ok to upgrade the request to HTTPS.

this comment explains why these new attributes are here.  but it says nothing about their values and effects.  add reasonably detailed comments to each of them please.

and in general, just using "priming" is misleading (to general).  it should always be connection of "HSTS priming" to make it clear.

@@ +554,5 @@
> +   * @param wouldBlock Carry the decision of Mixed Content Blocking to be
> +   * applied when HSTS priming is complete.
> +   */
> +  [noscript, notxpcom, nostdcall]
> +  void setHSTSPriming(in boolean wouldBlock);

a bit better name than wouldBlock may be mixedContentWouldBlock (to make it more clear)

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.cpp
@@ +365,5 @@
> +  if (NS_FAILED(rv)) {
> +    return mCallback->OnPrimingFailed(rv);
> +  }
> +
> +  return mCallback->OnPrimingSucceeded();

why are you passing error from the callback to result of OnStartRequest?

also, if you call on a callback, please swap to a local nsCOMPtr<> first.  avoids any potential reentrance issues.

@@ +501,5 @@
> +    // there is a non-expired entry in the cache
> +    // decide on block/no block here
> +    Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS_PRIMING_RESULT,
> +                          HSTSPrimingResult::eHSTS_PRIMING_CACHED_NO_UPGRADE);
> +    return aCallback->OnPrimingFailed(NS_ERROR_DOM_BAD_URI);

how can you be sure we don't loop?  the callback impl (if it is nsHttpChannel) calls again ContinueConnect() that calls this method.  I don't see anything that would be done here to prevent reenter.

I think best would be to make the callbacks (OnSuccess/OnFailure) just continue past the priming request (split the ContineConnect method) to make sure it can never loop (call back on this code).

@@ +532,5 @@
> +  rv = NS_NewChannelInternal(getter_AddRefs(primingChannel),
> +                             uri,
> +                             loadInfo,
> +                             loadGroup,
> +                             nullptr,   // aCallbacks

please note it's set later

@@ +537,5 @@
> +                             loadFlags);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Set method and headers
> +  nsCOMPtr<nsIHttpChannel> preHttp = do_QueryInterface(primingChannel);

please try to find some better name than "preHttp"

@@ +538,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Set method and headers
> +  nsCOMPtr<nsIHttpChannel> preHttp = do_QueryInterface(primingChannel);
> +  NS_ASSERTION(preHttp, "Failed to QI to nsIHttpChannel!");

Hmm.. I think you should make this a graceful failure and if you want, add MOZ_ASSERT(false).  NS_ASSERTION doesn't do much these days and you crash in release when the QI fails..

@@ +540,5 @@
> +  // Set method and headers
> +  nsCOMPtr<nsIHttpChannel> preHttp = do_QueryInterface(primingChannel);
> +  NS_ASSERTION(preHttp, "Failed to QI to nsIHttpChannel!");
> +
> +  rv = preHttp->SetRequestMethod(NS_LITERAL_CSTRING("HEAD"));

it's known that HEAD is sometimes not well implemented by servers, but let's see what happens...

::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.h
@@ +50,5 @@
> +  eHSTS_PRIMING_FAILED_BLOCK      = 6,
> +  eHSTS_PRIMING_FAILED_ACCEPT     = 7
> +};
> +
> +class nsHSTSPrimerListenerProxy final : public nsIStreamListener,

new classes have to be added to the mozilla namespace (and probably some subnamspace according module it lies at - here ::net) and not have the "ns" prefix.  

I understand you want to use the static methods (it's convenient), but something tells me this should be in /security where the HSTS service is.  but up to you, I'm ok with having it here when only user is http channel.

@@ +92,5 @@
> +  // The principal that originally kicked off the request
> +  nsCOMPtr<nsIPrincipal> mRequestingPrincipal;
> +  nsCOMPtr<nsIInterfaceRequestor> mOuterNotificationCallbacks;
> +  nsCOMPtr<nsINetworkInterceptController> mInterceptController;
> +};

no comments, no review...

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +418,5 @@
> +    // If we need to perform HSTS priming, do it now
> +    if (GetRequireHSTSPriming()
> +            && !GetHSTSPrimingDone()
> +            && !GetIsHSTSPrimingChannel()
> +            && mInterceptCache != INTERCEPTED) {

jdm should review this
weird indention, should be:

if (X() &&
    Y() &&
    Z()) {
   ...
}

@@ +428,5 @@
> +            rv = nsHSTSPrimerListenerProxy::StartHSTSPriming(this, this,
> +                    getter_AddRefs(mPrimingChannel));
> +            if (NS_FAILED(rv)) {
> +                CloseCacheEntry(true);
> +                AsyncAbort(rv);

why async?  you can just return the failure here.

@@ +438,5 @@
> +            }
> +        }
> +
> +        Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS_PRIMING_RESULT,
> +                HSTSPrimingResult::eHSTS_PRIMING_ALREADY_UPGRADED);

note for me: when I fully understand the patch, check this tele is correct.

@@ +453,5 @@
>          MOZ_ASSERT(!mPreflightChannel);
>          nsresult rv =
>              nsCORSListenerProxy::StartCORSPreflight(this, this,
> +                    mUnsafeHeaders,
> +                    getter_AddRefs(mPreflightChannel));

don't change the indention here?

@@ +2652,5 @@
>  {
>      bool hasContentEncoding =
>          mCachedResponseHead->HasHeader(nsHttp::Content_Encoding);
>  
> +    nsAutoCString etag;

no whitespace changes please, we fix them only when there is a change close to it it.  this just makes the patch harder to read and potentially backout.

@@ +7697,5 @@
> +    nsresult rv = sss->CacheNegativeHSTSResult(mURI, 24*60*60);
> +    NS_WARN_IF(NS_FAILED(rv));
> +
> +    if (wouldBlock) {
> +        CloseCacheEntry(true);

CloseCacheEntry(false);

@@ +7703,5 @@
> +        return NS_OK;
> +    }
> +
> +    // we can continue the load and the UI has been updated as mixed content
> +    return ContinueConnect();

and where the result error goes?  how is it handled?  I think you must asyncabort when it fails.

same applies to the success callback.

::: netwerk/protocol/http/nsIHstsPrimingCallback.h
@@ +20,5 @@
> +public:
> +  NS_DECLARE_STATIC_IID_ACCESSOR(nsIHstsPrimingCallback);
> +  NS_IMETHOD OnPrimingSucceeded() = 0;
> +  NS_IMETHOD OnPrimingFailed(nsresult aError) = 0;
> +};

total lack of comments, for me a reason to r-.  please add detailed comments to this whole interface (how it's used, who usually implements it and, if needed, what is its lifetime etc..) and to each attribute/method etc.

Comment 90

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Full HSTS priming patch

Comment 91

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to implement HSTS priming

Cleanup based on review comments.

Comment 92

[Honza Bambas (:mayhemer)]

(In reply to Kate McKinley [:kmckinley] from comment #91)
> Created attachment 8767304 [details] [diff] [review]
> HttpChannel changes to implement HSTS priming
> 
> Cleanup based on review comments.

And description (or reference to it) of the patch?  It would speed up the review and help understanding what/how/why you are doing.  Thanks.

Comment 93

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Honza Bambas (:mayhemer) from comment #89)
> Comment on attachment 8766046 [details] [diff] [review]
> HttpChannel changes to implement HSTS priming
> 
> Review of attachment 8766046 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +156,5 @@
> > +
> > +  uint32_t                         mForcePriming : 1;
> > +  uint32_t                         mIsPriming : 1;
> > +  uint32_t                         mMixedContentWouldBlock : 1;
> > +  uint32_t                         mPrimingDone : 1;
> 
> you can also use bool : 1
> 
> are these changing during the LoadInfo object lifetime?  (playing with
> making them bool const : 1)

They do change. They are first set in nsMixedContentBlocker::ShouldLoad after the LoadInfo is created.

> ::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.cpp
> @@ +365,5 @@
> > +  if (NS_FAILED(rv)) {
> > +    return mCallback->OnPrimingFailed(rv);
> > +  }
> > +
> > +  return mCallback->OnPrimingSucceeded();
> 
> why are you passing error from the callback to result of OnStartRequest?
> 
> also, if you call on a callback, please swap to a local nsCOMPtr<> first. 
> avoids any potential reentrance issues.

> @@ +540,5 @@
> > +  // Set method and headers
> > +  nsCOMPtr<nsIHttpChannel> preHttp = do_QueryInterface(primingChannel);
> > +  NS_ASSERTION(preHttp, "Failed to QI to nsIHttpChannel!");
> > +
> > +  rv = preHttp->SetRequestMethod(NS_LITERAL_CSTRING("HEAD"));
> 
> it's known that HEAD is sometimes not well implemented by servers, but let's
> see what happens...

we are going to experiment with using a GET instead of HEAD next

> 
> ::: netwerk/protocol/http/nsHSTSPrimerListenerProxy.h
> @@ +50,5 @@
> > +  eHSTS_PRIMING_FAILED_BLOCK      = 6,
> > +  eHSTS_PRIMING_FAILED_ACCEPT     = 7
> > +};
> > +
> > +class nsHSTSPrimerListenerProxy final : public nsIStreamListener,
> 
> new classes have to be added to the mozilla namespace (and probably some
> subnamspace according module it lies at - here ::net) and not have the "ns"
> prefix.  
> 
> I understand you want to use the static methods (it's convenient), but
> something tells me this should be in /security where the HSTS service is. 
> but up to you, I'm ok with having it here when only user is http channel.
> 

> 
> @@ +7703,5 @@
> > +        return NS_OK;
> > +    }
> > +
> > +    // we can continue the load and the UI has been updated as mixed content
> > +    return ContinueConnect();
> 
> and where the result error goes?  how is it handled?  I think you must
> asyncabort when it fails.
> 
> same applies to the success callback.

The result error is reported in telemetry, but an error in the request doesn't necessarily mean that we will block the load. For example, an image may not be available over HTTPS at all, but the user doesn't want to block mixed display content, so they want the HTTP load to continue.

Comment 94

[Josh Matthews [:jdm]]

Comment on attachment 8767304 [details] [diff] [review]
HttpChannel changes to implement HSTS priming

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +425,5 @@
> +
> +        if (requireHSTSPriming &&
> +            !isHSTSPrimingDone &&
> +            nsMixedContentBlocker::sSendHSTSPriming &&
> +            mInterceptCache != INTERCEPTED) {

It should not make a difference, but for clarity we should use `mInterceptCache == DO_NOT_INTERCEPT` here.

Comment 95

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #92)
> (In reply to Kate McKinley [:kmckinley] from comment #91)
> > Created attachment 8767304 [details] [diff] [review]
> > HttpChannel changes to implement HSTS priming
> > 
> > Cleanup based on review comments.
> 
> And description (or reference to it) of the patch?  It would speed up the
> review and help understanding what/how/why you are doing.  Thanks.

^^^

Comment 96

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to implement HSTS priming

HSTS priming does two things. First it swaps the order of mixed-content blocking and HSTS, so if a load would be mixed content and domain is in the HSTS cache, the load will be upgraded to HTTPS automatically. This means that some loads which would be blocked by mixed-content will instead be loaded as HTTPS. Second, if the domain is not in the HSTS cache, HSTS priming sends a HEAD request to the server over HTTPS, and if the server returns the Strict-Transport-Security header, the result is cached and the load is upgraded to HTTPS. If there is no STS header present, or if the site is not available over HTTPS, or any other problem, then that result is cached for one day so that we don't try again.

HSTS priming requires AsyncOpen2, so only loads that happen with AsyncOpen2 and have a LoadInfo will be affected. In the channel, the LoadInfo is the source of truth for whether or not to perform HSTS priming and whether mixed-content would be blocked if it can not be upgraded.

If the priming request fails, the load may continue if the load would not have been blocked by mixed-content blocker, for example, by default mixed display content is allowed, so an image may continue to load on the original HTTP URI.

Comment 97

[Honza Bambas (:mayhemer)]

Comment on attachment 8770757 [details] [diff] [review]
HttpChannel changes to implement HSTS priming

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

have you submitted an incomplete/draft/unrefreshed patch?

I can't even give f+, I'm just more and more confused here.  lack of comments, some code seems to be unused.  description doesn't match what I can see in the patch.

also, I'm missing any tests for this functionality.

I'm sorry...

::: netwerk/base/nsILoadInfo.idl
@@ +554,5 @@
> +   * Carry the decision whether this load would be blocked by mixed content so
> +   * that if HSTS priming fails, the correct decision can be made.
> +   */
> +  [noscript, infallible] readonly attribute boolean mixedContentWouldBlock;
> +  /*

**

@@ +555,5 @@
> +   * that if HSTS priming fails, the correct decision can be made.
> +   */
> +  [noscript, infallible] readonly attribute boolean mixedContentWouldBlock;
> +  /*
> +   * Set to true when HSTS priming is completed.

what exactly the "completed" here means?

::: netwerk/protocol/http/HSTSPrimerListenerProxy.cpp
@@ +19,5 @@
> +
> +using namespace mozilla;
> +
> +//////////////////////////////////////////////////////////////////////////
> +// HSTSPrimerListenerProxy

and what is this class?

@@ +29,5 @@
> +HSTSPrimerListenerProxy::HSTSPrimerListenerProxy(nsIStreamListener* aOuter,
> +                                                 nsIPrincipal* aRequestingPrincipal)
> +  : mOuterListener(aOuter)
> +  , mRequestingPrincipal(aRequestingPrincipal)
> +{

I don't see this class being constructed anywhere...

@@ +44,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +HSTSPrimerListenerProxy::CheckRequestApproved(nsIRequest* aRequest)

according what the method seems to do (actually reads the STS header and updated the STS status according ir + some other prechecks) I don't think the name CheckRequestApproved is good.

@@ +47,5 @@
> +nsresult
> +HSTSPrimerListenerProxy::CheckRequestApproved(nsIRequest* aRequest)
> +{
> +  // Check if the request failed
> +  LOG((":::::HSTS HSTSPrimerListenerProxy::CheckRequestApproved"));

"HSTSPrimerListenerProxy::CheckRequestApproved [this=%p]", this

@@ +54,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(status, status);
> +
> +  // Test that things worked on a HTTP level
> +  nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aRequest);

should be httpChannel

@@ +62,5 @@
> +  rv = http->GetURI(getter_AddRefs(requestURI));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(requestURI, NS_ERROR_DOM_BAD_URI);
> +
> +  // If this request was 

unfinished comment

@@ +148,5 @@
> +HSTSPrimerListenerProxy::SetInterceptController(nsINetworkInterceptController* aInterceptController)
> +{
> +  mInterceptController = aInterceptController;
> +}
> +*/

remove this

@@ +164,5 @@
> +  /*
> +  if (aIID.Equals(NS_GET_IID(nsINetworkInterceptController)) &&
> +      mInterceptController) {
> +    nsCOMPtr<nsINetworkInterceptController> copy(mInterceptController);
> +    *aResult = copy.forget().take();

maybe use the same for returning |this| in the nsIChannelEventSink case?

@@ +168,5 @@
> +    *aResult = copy.forget().take();
> +
> +    return NS_OK;
> +  }
> +  */

but I can see this is commented out, remove?

@@ +206,5 @@
> +  } else {
> +    // A real, external redirect, so check the URL.
> +    rv = CheckRequestApproved(aOldChannel);
> +    if (NS_FAILED(rv)) {
> +      // not approved or problem on the old channel

can you please explain more in detail why you are doing this?

@@ +208,5 @@
> +    rv = CheckRequestApproved(aOldChannel);
> +    if (NS_FAILED(rv)) {
> +      // not approved or problem on the old channel
> +      aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
> +      return NS_ERROR_DOM_BAD_URI;

why NS_ERROR_DOM_BAD_URI?

@@ +213,5 @@
> +    }
> +
> +    // if redirect is not to HTTPS, then we know it is not going to work
> +    nsCOMPtr<nsIURI> newURI;
> +    rv = NS_GetFinalChannelURI(aNewChannel, getter_AddRefs(newURI));

why exactly are you using NS_GetFinalChannelURI?  GetURI on the new channel would do.

@@ +223,5 @@
> +    bool isHttps = false;
> +    rv = newURI->SchemeIs("https", &isHttps);
> +    if (NS_FAILED(rv) || !isHttps) {
> +      nsCOMPtr<nsIURI> oldURI;
> +      rv = NS_GetFinalChannelURI(aOldChannel, getter_AddRefs(oldURI));

here GetURI would do as well... or do you want the original URI here?  it's so confusing, mainly because of missing comments about what you are doing here..

@@ +233,5 @@
> +      aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI);
> +      return NS_ERROR_DOM_BAD_URI;
> +    }
> +
> +    rv = UpdateChannel(aNewChannel);

update to what? different url? or what?  it actually just adds USTS header, so it more sounds like PrepareChannel or something to me.  a better name for this method would be nice too.

@@ +259,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (nsCOMPtr<nsIThreadRetargetableStreamListener> retargetableListener =
> +        do_QueryInterface(mOuterListener)) {

put the whole assignment expression to one more ( ) or split it to its own line.

@@ +320,5 @@
> +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService(NS_SSSERVICE_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  /* Don't try again for about a day */
> +  rv = sss->CacheNegativeHSTSResult(aURI, 24*60*60);

nit: 24 * 60 * 60

@@ +322,5 @@
> +
> +  /* Don't try again for about a day */
> +  rv = sss->CacheNegativeHSTSResult(aURI, 24*60*60);
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("HSTSPrimerListenerProxy::AsyncOnChannelRedirect: "

why do you refer AsyncOnChannelRedirect here?

@@ +361,5 @@
> +  void AddResultToCache(nsIRequest* aRequest);
> +
> +  nsCOMPtr<nsIPrincipal> mReferrerPrincipal;
> +  nsCOMPtr<nsIHstsPrimingCallback> mCallback;
> +};

comments on methods/members please...

@@ +439,5 @@
> +  }
> +
> +  // check to see if the HSTS cache was updated
> +  bool hsts;
> +  bool cached;

please declare before first use

@@ +478,5 @@
> +  uint32_t totalRead;
> +  return inStr->ReadSegments(NS_DiscardSegment, nullptr, count, &totalRead);
> +}
> +
> +nsresult

add |// static| comment before this method (actually, before any static method definition)

@@ +481,5 @@
> +
> +nsresult
> +HSTSPrimerListenerProxy::StartHSTSPriming(nsIChannel* aRequestChannel,
> +                                            nsIHstsPrimingCallback* aCallback,
> +                                            nsIChannel** aPrimingChannel)

nit: indent

@@ +489,5 @@
> +  nsCOMPtr<nsIURI> finalChannelURI;
> +  nsresult rv = NS_GetFinalChannelURI(aRequestChannel, getter_AddRefs(finalChannelURI));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // strip out any path, query, or fragment from the URI

where is it that happening?

::: netwerk/protocol/http/HSTSPrimerListenerProxy.h
@@ +39,5 @@
> +  // even though we succeeded, preserve prior order of mixed-content and hsts
> +  // and allow the load as http
> +  eHSTS_PRIMING_SUCCEEDED_HTTP    = 5,
> +  eHSTS_PRIMING_FAILED_BLOCK      = 6,
> +  eHSTS_PRIMING_FAILED_ACCEPT     = 7

please add comments to all of these when they happen

@@ +45,5 @@
> +
> +class HSTSPrimerListenerProxy final : public nsIStreamListener,
> +                                        public nsIInterfaceRequestor,
> +                                        public nsIChannelEventSink,
> +                                        public nsIThreadRetargetableStreamListener

nit: indent

because of complete lack of comments on this class I don't understand how it's used and why it implements stream listener and others.

@@ +84,5 @@
> +   */
> +  nsresult UpdateChannel(nsIChannel* aChannel);
> +  /**
> +   * Given a request, a request is approved if the request happens over HTTPS
> +   * and contains a valid STS header with a max-age greater than 0

sorry, this really is not a good comment.  what is "a request" and what is "the request"?  what does the method actually do?

@@ +89,5 @@
> +   */
> +  nsresult CheckRequestApproved(nsIRequest* aRequest);
> +
> +  // The nsIStreamListener to propagate events to
> +  nsCOMPtr<nsIStreamListener> mOuterListener;

and why is it here?

::: netwerk/protocol/http/HttpBaseChannel.h
@@ -247,5 @@
>  
>    // nsIResumableChannel
>    NS_IMETHOD GetEntityID(nsACString& aEntityID) override;
>  
> -

no ws only changes please

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +420,5 @@
> +        bool requireHSTSPriming = false;
> +        mLoadInfo->GetForceHSTSPriming(&requireHSTSPriming);
> +
> +        bool isHSTSPrimingDone = false;
> +        mLoadInfo->GetIsHSTSPrimingDone(&isHSTSPrimingDone);

isn't there a getter that can be used as:

bool isHSTSPrimingDone = mLoadInfo->GetIsHSTSPrimingDone();

?

@@ +425,5 @@
> +
> +        if (requireHSTSPriming &&
> +            !isHSTSPrimingDone &&
> +            nsMixedContentBlocker::sSendHSTSPriming &&
> +            mInterceptCache != INTERCEPTED) {

according comment 94 change this to:

mInterceptCache == DO_NOT_INTERCEPT

@@ +434,5 @@
> +                rv = HSTSPrimerListenerProxy::StartHSTSPriming(this, this,
> +                        getter_AddRefs(mPrimingChannel));
> +
> +                if (NS_FAILED(rv)) {
> +                    CloseCacheEntry(true);

just leave the arg at false

@@ +7664,5 @@
> +nsHttpChannel::OnHSTSPrimingSucceeded()
> +{
> +    mLoadInfo->SetIsHSTSPrimingDone(true);
> +    mPrimingChannel = nullptr;
> +    LOG((":::::HSTS Priming succeeded: %s", mSpec.get()));

LOG(("nsHttpChannel::OnHSTSPrimingSucceeded [this=%p]", this));

and similar to on-failed callback please.

@@ +7671,5 @@
> +        // redirect the channel to HTTPS if we are allowed to use the result
> +        LOG((":::::HSTS Priming succeeded: redirecting to HTTPS %s", mSpec.get()));
> +        Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS_PRIMING_RESULT,
> +                              HSTSPrimingResult::eHSTS_PRIMING_SUCCEEDED);
> +        return AsyncCall(&nsHttpChannel::HandleAsyncRedirectChannelToHttps);

please add a note the OnHSTSPrimingSucceeded() callback can invoke synchronously from HSTSPrimerListenerProxy::StartHSTSPriming.  This must also be documented in the IDL!

@@ +7675,5 @@
> +        return AsyncCall(&nsHttpChannel::HandleAsyncRedirectChannelToHttps);
> +    }
> +
> +    bool wouldBlock = true;
> +    mLoadInfo->GetMixedContentWouldBlock(&wouldBlock);

According your description of the patch, you claim you first try HSTS and then go to mixed content blocker (that you have swapped the checks).

but here it seems like the check has already been done and here we enforce its resolution.  that seems odd, I would expect that mixed content blocker will do check later after this point and potentially block the channel.

can you explain?

@@ +7685,5 @@
> +        Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS_PRIMING_RESULT,
> +                              HSTSPrimingResult::eHSTS_PRIMING_SUCCEEDED_BLOCK);
> +        CloseCacheEntry(false);
> +        AsyncAbort(NS_ERROR_CONTENT_BLOCKED);
> +        return NS_ERROR_CONTENT_BLOCKED;

return AsyncAbort(NS_ERROR_CONTENT_BLOCKED);

or just.

return NS_OK


the result goes nowhere anyway.

@@ +7691,5 @@
> +
> +    LOG((":::::HSTS Priming succeeded, loading insecure: %s", mSpec.get()));
> +    Telemetry::Accumulate(Telemetry::MIXED_CONTENT_HSTS_PRIMING_RESULT,
> +                          HSTSPrimingResult::eHSTS_PRIMING_SUCCEEDED_HTTP);
> +    return ContinueConnect();

if ContinueConnect() fails, you have to AsyncAbort.

@@ +7701,5 @@
> +    mLoadInfo->SetIsHSTSPrimingDone(true);
> +    mPrimingChannel = nullptr;
> +
> +    bool wouldBlock = true;
> +    mLoadInfo->GetMixedContentWouldBlock(&wouldBlock);

use infallible getter?

@@ +7704,5 @@
> +    bool wouldBlock = true;
> +    mLoadInfo->GetMixedContentWouldBlock(&wouldBlock);
> +
> +    LOG((":::::HSTS Priming Failed %s, %s the load", mSpec.get(),
> +                (wouldBlock) ? "blocking" : "allowing"));

update the log here as well.

@@ +7719,5 @@
> +    // If we would block, go ahead and abort with the error provided
> +    if (wouldBlock) {
> +        CloseCacheEntry(false);
> +        AsyncAbort(aError);
> +        return aError;

same here

@@ +7723,5 @@
> +        return aError;
> +    }
> +
> +    // we can continue the load and the UI has been updated as mixed content
> +    return ContinueConnect();

same here.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +168,5 @@
>      NS_IMETHOD OnPreflightSucceeded() override;
>      NS_IMETHOD OnPreflightFailed(nsresult aError) override;
> +    // nsIHstsPrimingCallback
> +    NS_IMETHOD OnHSTSPrimingSucceeded() override;
> +    NS_IMETHOD OnHSTSPrimingFailed(nsresult aError) override;

just use NS_DECL_NSIHSTSPRIMINGCALLBACK ?

@@ +206,5 @@
>      }
>  
>      nsresult OpenCacheEntry(bool usingSSL);
>      nsresult ContinueConnect();
> +    nsresult TryHSTSPriming();

comment what these do, when called etc..

@@ +555,5 @@
>      uint32_t                          mStronglyFramed : 1;
>  
>      nsCOMPtr<nsIChannel>              mPreflightChannel;
>  
> +    nsCOMPtr<nsIChannel>              mPrimingChannel;

comment what this is

Comment 98

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HttpChannel changes to implement HSTS priming

* updated simple comments from comment 97
* removed dead code (HSTSPrimerListenerProxy)
* move nsIHstsPrimingCallback to an idl
* stop double counting results if there is a cached result in HSTSPrimerListener::StartHSTSPriming.
* removed the isHSTSPrimingDone flags on the LoadInfo since this is now handled by splitting it out of ContinueConnect

Addressing remaining questions in comment 97.

> also, I'm missing any tests for this functionality.

Tests for HSTS priming are in attachment 8763116 [details] [diff] [review].

> @@ +7675,5 @@
> > +        return AsyncCall(&nsHttpChannel::HandleAsyncRedirectChannelToHttps);
> > +    }
> > +
> > +    bool wouldBlock = true;
> > +    mLoadInfo->GetMixedContentWouldBlock(&wouldBlock);
> 
> According your description of the patch, you claim you first try HSTS and
> then go to mixed content blocker (that you have swapped the checks).
> 
> but here it seems like the check has already been done and here we enforce
> its resolution.  that seems odd, I would expect that mixed content blocker
> will do check later after this point and potentially block the channel.
> 
> can you explain?

Mixed-content blocking is kicked off by nsContentSecurityManager::doContentSecurityCheck very early on in AsyncOpen2, so nsMixedContentBlocker marks the LoadInfo as needing priming, as well as what would happen if the request can't be upgraded to HTTPS. If priming fails, or if the user pref security.mixed_content.use_hsts is false, the mixed-content blocker decision should be honored.

Comment 99

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Full HSTS priming proof-of-concept patch

Bug 1246540 HSTS Priming Proof of Concept

HSTS priming changes the order of mixed-content blocking and HSTS
upgrades, and adds a priming request to check if a mixed-content load is
accesible over HTTPS and the server supports upgrading via the
Strict-Transport-Security header.

Every call site that uses AsyncOpen2 passes through the mixed-content
blocker, and has a LoadInfo. If the mixed-content blocker marks the load as
needing HSTS priming, nsHttpChannel will build and send an HSTS priming
request on the same URI with the scheme upgraded to HTTPS. If the server
allows the upgrade, then channel performs an internal redirect to the HTTPS URI,
otherwise use the result of mixed-content blocker to allow or block the
load.

nsISiteSecurityService adds an optional boolean out parameter to
determine if the HSTS state is already cached for negative assertions.
If the host has been probed within the previous 24 hours, no HSTS
priming check will be sent.

Comment 100

[Honza Bambas (:mayhemer)]

Comment on attachment 8773518 [details] [diff] [review]
HttpChannel changes to implement HSTS priming

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

r+ with the comments fixed and questions answered.

Looks much nicer and cleaner now!  Thanks, also for the comments.

The complete patch doesn't apply cleanly on m-c, so I could not test the patch manually.  Hope your test checks all the corner cases.

::: netwerk/protocol/http/HSTSPrimerListener.cpp
@@ +53,5 @@
> +HSTSPrimingListener::OnStopRequest(nsIRequest *aRequest,
> +                                   nsISupports *aContext,
> +                                   nsresult aStatus)
> +{
> +  mCallback = nullptr;

It's guarantied you always get OnStartRequest and it also seems like there is no way to assign mCallback again.  Hence I don't think you need to nullify it here again.

@@ +151,5 @@
> +  rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri, 0, &cached, &hsts);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (hsts) {
> +    // already saw this host and will upgrade if alllowed by preferences

alllowed

@@ +153,5 @@
> +
> +  if (hsts) {
> +    // already saw this host and will upgrade if alllowed by preferences
> +    aCallback->OnHSTSPrimingSucceeded(true);
> +    return NS_OK;

return what the callback is returning here as well as in the on-failed case

@@ +172,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsILoadInfo> loadInfo = static_cast<mozilla::LoadInfo*>
> +    (originalLoadInfo.get())->CloneForNewRequest();

isn't there some nicer way that would avoid static_cast?

@@ +185,5 @@
> +
> +  // Priming requests should never be intercepted by service workers and
> +  // are always anonymous.
> +  loadFlags |= nsIChannel::LOAD_BYPASS_SERVICE_WORKER |
> +               nsIRequest::LOAD_ANONYMOUS;

You should pass through (leave set) only few load flags, mainly cache related: INHIBIT_CACHING, INHIBIT_PERSISTENT_CACHING, LOAD_BYPASS_CACHE, LOAD_FROM_CACHE, VALIDATE_ALWAYS.

It would be good to carry all the class flags: GetClassFlags on the aRequest channel and then SetClassFlags them on the priming channel.  It will somewhat help performance.

@@ +200,5 @@
> +
> +  // Set method and headers
> +  nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(primingChannel);
> +  if (!httpChannel) {
> +    NS_WARNING("HSTSPrimingListener: Failed to QI to nsIHttpChannel!");

I'd almost use NS_ERROR here

@@ +218,5 @@
> +  // Set up listener which will start the original channel
> +  RefPtr<HSTSPrimingListener> primingListener =
> +    new HSTSPrimingListener(aCallback);
> +
> +  rv = primingChannel->SetNotificationCallbacks(primingListener);

why are you doing this actually?  I don't see any purpose.

::: netwerk/protocol/http/HSTSPrimerListener.h
@@ +75,5 @@
> +   * HSTS header and we can upgrade. If the request failed, if no header was
> +   * found, or the site does not permit HSTS upgrades, return
> +   * NS_ERROR_CONTENT_BLOCKED. The load may still proceed of mixed-content would
> +   * not otherwise block the load.
> +   */

a misplaced comment?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +421,5 @@
> +        if (requireHSTSPriming &&
> +            nsMixedContentBlocker::sSendHSTSPriming &&
> +            mInterceptCache == DO_NOT_INTERCEPT) {
> +            // The load requires HSTS priming, we are allowed to send HSTS
> +            // priming requests, and this is not an intercepted request.

nice comment, but everybody can deduce that from the condition already.  if you want to add a comment, then rather say why the condition is built the way it is.

@@ +436,5 @@
> +                }
> +
> +                NS_ENSURE_TRUE(mPrimingChannel, NS_ERROR_DOM_BAD_URI);
> +                return NS_OK;
> +            } else {

don't do else after return

::: netwerk/protocol/http/nsHttpChannel.h
@@ +205,5 @@
>  
>      nsresult OpenCacheEntry(bool usingSSL);
>      nsresult ContinueConnect();
>  
> +    // If the load is mixed-content, build and send an HSTS priming request. 

ws

@@ +555,5 @@
>      uint32_t                          mStronglyFramed : 1;
>  
>      nsCOMPtr<nsIChannel>              mPreflightChannel;
>  
> +    // HSTS priming creates a new channel to handle the priming request

and why are we keeping a reference to it?

honestly, as I'm seeing the code, you don't use this member at all.

::: netwerk/protocol/http/nsIHstsPrimingCallback.idl
@@ +9,5 @@
> +/**
> + * HSTS priming attempts to prevent mixed-content by looking for the
> + * Strict-Transport-Security header as a signal from the server that it is
> + * safe to upgrade HTTP to HTTPS.
> + * 

nit: trailing white space, more times in this file.

Comment 101

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Honza Bambas (:mayhemer) from comment #100)
> Comment on attachment 8773518 [details] [diff] [review]
> HttpChannel changes to implement HSTS priming
> 
> Review of attachment 8773518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the comments fixed and questions answered.
> 
> Looks much nicer and cleaner now!  Thanks, also for the comments.
> 
> The complete patch doesn't apply cleanly on m-c, so I could not test the
> patch manually.  Hope your test checks all the corner cases.
> 
> ::: netwerk/protocol/http/HSTSPrimerListener.cpp
> @@ +53,5 @@
> > +HSTSPrimingListener::OnStopRequest(nsIRequest *aRequest,
> > +                                   nsISupports *aContext,
> > +                                   nsresult aStatus)
> > +{
> > +  mCallback = nullptr;
> 
> It's guarantied you always get OnStartRequest and it also seems like there
> is no way to assign mCallback again.  Hence I don't think you need to
> nullify it here again.

removed

> @@ +172,5 @@
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  nsCOMPtr<nsILoadInfo> loadInfo = static_cast<mozilla::LoadInfo*>
> > +    (originalLoadInfo.get())->CloneForNewRequest();
> 
> isn't there some nicer way that would avoid static_cast?

Currently CloneForNewRequest is not in the IDL. The same pattern is used by nsCORSListenerProxy and ExtensionProtocolHandler.

I've been working with mwobensmith to make sure the tests have good coverage.

Comment 102

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Addressed comment 100 and unified patch. Carrying over feedback on all individual patches.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08c576ffc550

Comment 103

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/640247e978ba
HSTS Priming Proof of Concept. r=honzab

Comment 104

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32509782&repo=mozilla-inbound

Comment 105

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ce447d842c
Backed out changeset 640247e978ba for bustage

Comment 106

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Update patch to fix compiler error.

Comment 107

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Bump patch due to feedback from tanvi

Comment 108

[Tanvi Vyas[:tanvi]]

Comment on attachment 8774429 [details] [diff] [review]
HSTS Priming PoC

Great work Kate!  A couple of nits, which I think you've already fixed...

>diff --git a/dom/security/nsMixedContentBlocker.cpp b/dom/security/nsMixedContentBlocker.cpp
>@@ -859,17 +898,23 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>       } else {
>         // User has overriden the pref and the root is not https;
>         // mixed display content was allowed on an https subframe.
>         if (NS_SUCCEEDED(stateRV)) {
>           eventSink->OnSecurityChange(aRequestingContext, (state | nsIWebProgressListener::STATE_LOADED_MIXED_DISPLAY_CONTENT));
>         }
>       }
>     } else {
>-      *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      if (doHSTSPriming) {
>+        document->AddHSTSPrimingLocation(aContentLocation,
User innerContentLocation here and below.

>+            HSTSPrimingState::eHSTS_PRIMING_BLOCK);
>+        *aDecision = nsIContentPolicy::ACCEPT;
>+      } else {
>+        *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      }
>       LogMixedContentMessage(classification, aContentLocation, rootDoc, eBlocked);
>       if (!rootDoc->GetHasMixedDisplayContentBlocked() && NS_SUCCEEDED(stateRV)) {
>         rootDoc->SetHasMixedDisplayContentBlocked(true);
>         eventSink->OnSecurityChange(aRequestingContext, (state | nsIWebProgressListener::STATE_BLOCKED_MIXED_DISPLAY_CONTENT));
>       }
>     }
>     return NS_OK;
> 
>@@ -905,32 +950,37 @@ nsMixedContentBlocker::ShouldLoad(bool aHadInsecureImageRedirect,
>         // mixed active content was allowed on an https subframe.
>         if (NS_SUCCEEDED(stateRV)) {
>           eventSink->OnSecurityChange(aRequestingContext, (state | nsIWebProgressListener::STATE_LOADED_MIXED_ACTIVE_CONTENT));
>         }
>         return NS_OK;
>       }
>     } else {
>       //User has not overriden the pref by Disabling protection. Reject the request and update the security state.
>-      *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      if (doHSTSPriming) {
>+        document->AddHSTSPrimingLocation(aContentLocation,
innerContentLocation

>+            HSTSPrimingState::eHSTS_PRIMING_BLOCK);
>+        *aDecision = nsIContentPolicy::ACCEPT;
>+      } else {
>+        *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+      }
>       LogMixedContentMessage(classification, aContentLocation, rootDoc, eBlocked);
>       // See if the pref will change here. If it will, only then do we need to call OnSecurityChange() to update the UI.
>       if (rootDoc->GetHasMixedActiveContentBlocked()) {
>         return NS_OK;
>       }
>       rootDoc->SetHasMixedActiveContentBlocked(true);
> 
>       // The user has not overriden the pref, so make sure they still have an option by calling eventSink
>       // which will invoke the doorhanger
>       if (NS_SUCCEEDED(stateRV)) {
>          eventSink->OnSecurityChange(aRequestingContext, (state | nsIWebProgressListener::STATE_BLOCKED_MIXED_ACTIVE_CONTENT));
>       }
>-      return NS_OK;
>     }
>-
>+    return NS_OK;
Leave this return where it was before.

>   } else {
>     // The content is not blocked by the mixed content prefs.
> 
>     // Log a message that we are loading mixed content.
>     LogMixedContentMessage(classification, aContentLocation, rootDoc, eUserOverride);
> 
>     // Fire the event from a script runner as it is unsafe to run script
>     // from within ShouldLoad

Comment 109

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1825b8fa636a
HSTS Priming Proof of Concept. r=ckerschb,r=mayhemer,r=jld,r=smaug,r=dkeeler,r=jmaher,p=ally

Comment 110

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5ba1af68d0
Backed out changeset 1825b8fa636a for bustage

Comment 111

[Carsten Book [:Tomcat]]

backed out for windows bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32600563&repo=mozilla-inbound

Comment 112

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Builds on Win32. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0800fdf085

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e39be85498
HSTS Priming Proof of Concept. r=ckerschb, r=mayhemer, r=jld, r=smaug, r=dkeeler, r=jmaher, p=ally

Comment 114

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2155e6ea7a
Backed out changeset d7e39be85498 for Mochitest failures

Comment 115

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc198cd46ff
HSTS Priming Proof of Concept

Comment 116

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a977d25ead
Backed out changeset 8dc198cd46ff for Mochitest failures

Comment 117

[Carsten Book [:Tomcat]]

since this bounced now a few times can we maybe get a full try before trying to checkin again

Comment 118

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #117)
> since this bounced now a few times can we maybe get a full try before trying
> to checkin again

Comment 119

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Fixed the crashing bug & web-platform-tests issue.
Rebased to current

Full try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=868658c64887

Investigating failing tests:
Windows 7 VM opt web-platform-tests 3 - no unexpected results, but log parsing fails
Windows 7 VM debug web-platform-tests 6 - no unexpected results, log parsing failed

Reftest jobs, GTest jobs - work in all other cases, but have crashes, so retriggered jobs.

Carsten,

Comment 120

[Kate McKinley [:kmckinley, :Kate]]

retriggered jobs all succeeded, marking for checkin

Comment 121

[Ryan VanderMeulen [:RyanVM]]

I see a ton of failing mochitests in that Try push, a la:
https://treeherder.mozilla.org/logviewer.html#?job_id=24734937&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=24734940&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=24734948&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=24734959&repo=try

Comment 122

[Honza Bambas (:mayhemer)]

Kate, please make sure you check all the tests on a try run carefully before you set the checkin-needed keyword.  Our sheriffs have a lot of other more important work to do than checking other people's try runs carefully.  That is a full responsibility of the patch author - here - you.

The patch is complicated, it had a complicated review process already.  With such patches r+ doesn't usually mean it's the end of the work.

If you need any help, just let us know.  Figuring out failing tests may not always be easy.

Comment 123

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Changes from previous:

* The test for bug 704320 uses shared state, and the priming request caused the shared state to be set. Have the server return a 418 instead of updating shared state, and ignore 418 in the client. (reviewed with ckerschb)

* nsMixedContentBlocker::AsyncOnChannelRedirect was setting HSTS priming, but it wasn't being set on the redirected channel. Propagate the values from HttpChannelChild to HttpChannelParent in Redirect2Verify and attach them to the new channel, if the channel has a LoadInfo.

* browser/base/content/test/general/browser_mcb_redirect.js had 4 tests which fail whether or not the patch is applied due to timeouts. Reason was image/test/mochitest/blue.png was 404 due to other changes. Added it to browser/base/content/test/general/browser.ini. The other failing tests are fixed by the nsMixedContentBlocker changes.


* In HSTSPrimingListener::StartHSTSPriming, if the LoadInfo doesn't have a security flag, consider priming failed. This avoids a MOZ_ASSERT in nsContentSecurityManager::doContentSecurityCheck where we validate the flags on the LoadInfo.

Latest Treeherder https://treeherder.mozilla.org/#/jobs?repo=try&revision=06456b4a91cd
Results look good, except for the log parsing errors on Windows7.

mayhemer, can you please review the HttpChannelParent/Child changes?
ckerschb, can you please look at the nsMixedContentBlocker change? It is a small change in AsyncOnChannelRedirect to pass the priming through the redirect.

Comment 124

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kate McKinley [:kmckinley] from comment #123)
> ckerschb, can you please look at the nsMixedContentBlocker change? It is a

Kate, it seems like a lot of things changed (tests and code). I can certainly help review those bits, but it's too time consuming, cumbersome and error prone to look through the whole patch again (which you have to agree is quite massive). Also, this is a new feature so I am hesitate to accept those new changes without being able to exactly see what changed.

Any chance you provide an interdiff of the changes you describe in comment 123?

Comment 125

[Honza Bambas (:mayhemer)]

Comment on attachment 8781662 [details] [diff] [review]
HSTS Priming PoC

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

if you insist on such a long commit message (not very useful when there is always a reference to the bug) then remove the "[PATCH]" and "New Binary File" bits.  maybe check how commit messages are usually formatted in our tree
next time simply ask for review (not need info)
the changes look good, but I'd like to see an interdiff too, if possible

thanks for the effort!

this is a complicated patch that will need care a long after we land it...  I anticipate problems, mainly after this gets to the release channel.

I forgot one important thing: please add your new preferences (security.mixed_content.*) to all.js (modules/libpref/init/all.js)

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1592,5 @@
>      newHttpChannel->SetOriginalURI(mOriginalURI);
> +
> +    nsCOMPtr<nsILoadInfo> newLoadInfo;
> +    nsresult rv = newHttpChannel->GetLoadInfo(getter_AddRefs(newLoadInfo));
> +    if (NS_SUCCEEDED(rv) && newLoadInfo) {

please declare rv at the top of the method (only exception from the 'declare on first use' rule)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +673,5 @@
>          newInternalChannel->SetCorsPreflightParameters(args.unsafeHeaders());
>        }
> +
> +      nsCOMPtr<nsILoadInfo> newLoadInfo;
> +      nsresult rv = newHttpChannel->GetLoadInfo(getter_AddRefs(newLoadInfo));

again, declare rv at the top of the method.

@@ +675,5 @@
> +
> +      nsCOMPtr<nsILoadInfo> newLoadInfo;
> +      nsresult rv = newHttpChannel->GetLoadInfo(getter_AddRefs(newLoadInfo));
> +      if (NS_SUCCEEDED(rv) && newLoadInfo) {
> +        if (aForceHSTSPriming) {

maybe change the whole code (schematically):

if (aForceHSTSPriming) {
  newChannel->GetLoadInfo
  if (SUCCEDED && loadInfo) {
    loadInfo->SetHSTSPrim(aMixed...);
  }
}

because you don't need newLoadInfo when !aForceHSTS.

Comment 126

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

* Updated with comments from mayhemer
* Added permissions sets to mixed-content web-platform tests to work around the log parsing bugs.

Comment 127

[Kate McKinley [:kmckinley, :Kate]]

Attachment: interdiff to prior approved patches

Interdiff as requested.

This also has a set of changes to the web-platform tests to disable HSTS priming, not because it causes the tests to fail, but because on Win7 the HEAD request causes the HTTP server to emit characters that crashes the parser. Bug 1295722 addresses that, and when that is complete, the prefs can be removed. (Bug 1296384  to remove).

Last treeherder https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcc3d167ca81&selectedJob=25903084

Comment 128

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8782575 [details] [diff] [review]
interdiff to prior approved patches

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

Oh, in the end that was a smaller change from what I expected when looking at the initial ni? (r?) request. Anyway, it's good that we have an interdiff. Changes look good. r=me on the mixed content bits.

::: dom/security/nsMixedContentBlocker.cpp
@@ +1129,5 @@
>  {
> +  bool sendPriming = false;
> +  bool mixedContentWouldBlock = false;
> +  nsresult rv = GetHSTSPrimingFromRequestingContext(aURI, aRequestingContext,
> +      sendPriming, mixedContentWouldBlock);

nit: indendation

@@ +1144,5 @@
> +nsresult
> +nsMixedContentBlocker::GetHSTSPrimingFromRequestingContext(nsIURI* aURI,
> +    nsISupports* aRequestingContext,
> +    bool& aSendPrimingRequest,
> +    bool& aMixedContentWouldBlock)

nit: indendation

::: dom/security/nsMixedContentBlocker.h
@@ +86,5 @@
> +   */
> +  static nsresult GetHSTSPrimingFromRequestingContext(nsIURI* aURI,
> +                                         nsISupports* aRequestingContext,
> +                                         bool& aSendPrimingRequest,
> +                                         bool& aMixedContentWouldBlock);

nit: indendation

::: image/imgLoader.cpp
@@ +657,5 @@
>  
> +  bool sendPriming = false;
> +  bool mixedContentWouldBlock = false;
> +  rv = nsMixedContentBlocker::GetHSTSPrimingFromRequestingContext(contentLocation,
> +      aLoadingContext, sendPriming, mixedContentWouldBlock);

nit: indendation

::: netwerk/protocol/http/HSTSPrimerListener.cpp
@@ +38,5 @@
>  {
>    nsresult rv = CheckHSTSPrimingRequestStatus(aRequest);
>    nsCOMPtr<nsIHstsPrimingCallback> callback(mCallback);
>    mCallback = nullptr;
> +  

nit: remove whitespace

Comment 129

[Honza Bambas (:mayhemer)]

Comment on attachment 8782575 [details] [diff] [review]
interdiff to prior approved patches

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

r- because try run still has few unexpected failures of tests apparently related to this patch.

::: browser/base/content/test/general/browser_mcb_redirect.js
@@ +307,3 @@
>    Services.prefs.setBoolPref(PREF_ACTIVE, true);
>    Services.prefs.setBoolPref(PREF_DISPLAY, true);
> +  Services.prefs.setBoolPref(PREF_SEND_PRIMING, true);

this is a bit odd.. in all.js send_hsts_priming is true.  so there should be no need to set it to true here.  

although, I understand this is a kind of a test that is directly influenced with this preference, so that you want to make sure the prefs are set a way to make this test work.  Better than branch this test to respect any value of this preference, probably.

::: dom/security/nsMixedContentBlocker.cpp
@@ +344,1 @@
>      rv = nsMixedContentBlocker::MarkLoadInfoForPriming(newUri,

add a comment why you are doing this (no use for passed loadInfo)


nit: the style is more like (things that relates are in a single "monolith"):

    nsCOMPtr<nsILoadInfo> newLoadInfo;
    rv = aNewChannel->GetLoadInfo(getter_AddRefs(newLoadInfo));
    NS_ENSURE_SUCCESS(rv, rv);

    rv = nsMixedContentBlocker::MarkLoadInfoForPriming(newUri,

@@ +1128,5 @@
>                                                nsILoadInfo* aLoadInfo)
>  {
> +  bool sendPriming = false;
> +  bool mixedContentWouldBlock = false;
> +  nsresult rv = GetHSTSPrimingFromRequestingContext(aURI, aRequestingContext,

small nit: please declare rv as the very first variable of the method.

@@ +1145,5 @@
> +nsMixedContentBlocker::GetHSTSPrimingFromRequestingContext(nsIURI* aURI,
> +    nsISupports* aRequestingContext,
> +    bool& aSendPrimingRequest,
> +    bool& aMixedContentWouldBlock)
> +{

set the out args to default values first.  mozilla style is to have a fully defined result when you return NS_OK, so that the caller doesn't set the default values first.

@@ +1165,5 @@
>    nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>    if (!docShell) {
>      return NS_OK;
>    }
> +  nsCOMPtr<nsIDocument> document(docShell->GetDocument());

I don't think you need it, documents are usually worked with only on a single thread, so it can't go away and you save addref/release.

::: dom/security/nsMixedContentBlocker.h
@@ +76,5 @@
>                                           nsISupports* aRequestingContext,
>                                           nsILoadInfo* aLoadInfo);
>  
> +  /* When the imgLoader has a cache entry, and that entry may result in a
> +   * priming request, don't use the cache entry.

can you please make this comment more descriptive somehow?  I want to understand what the method exactly does, but this doesn't explain it very much.  I don't find the vague reference to imgLoader a good comment.

@@ +81,5 @@
> +   *
> +   * @param aURI The URI associated with the load
> +   * @param aRequestingContext the requesting context passed to ShouldLoad
> +   * @param aSendPrimingRequest true if priming is required on the channel
> +   * @param aMixedContentWouldBlock true if mixed content would block

I assume the last two args are OUT args, can you please emphasize it in the comment?

@@ +86,5 @@
> +   */
> +  static nsresult GetHSTSPrimingFromRequestingContext(nsIURI* aURI,
> +                                         nsISupports* aRequestingContext,
> +                                         bool& aSendPrimingRequest,
> +                                         bool& aMixedContentWouldBlock);

Mozilla style is to return by pointer, not reference (I know, I like this one more too, but it's not up to me...)

::: image/imgLoader.cpp
@@ +657,5 @@
>  
> +  bool sendPriming = false;
> +  bool mixedContentWouldBlock = false;
> +  rv = nsMixedContentBlocker::GetHSTSPrimingFromRequestingContext(contentLocation,
> +      aLoadingContext, sendPriming, mixedContentWouldBlock);

I'd go with indention like:

rv = nsMixedContentBlocker::GetHSTSPrimingFromRequestingContext(
  contentLocation, aLoadingContext, sendPriming, mixedContentWouldBlock);

::: modules/libpref/init/all.js
@@ +5512,5 @@
>  #else
>  pref("dom.html_fragment_serialisation.appendLF", false);
>  #endif
> +
> +// HSTS Priming (Bug 1275402)

don't refer the bug number

::: netwerk/protocol/http/HSTSPrimerListener.cpp
@@ +34,4 @@
>  
>  NS_IMETHODIMP
>  HSTSPrimingListener::OnStartRequest(nsIRequest *aRequest,
>                                     nsISupports *aContext)

nit: indent

@@ +39,5 @@
>    nsresult rv = CheckHSTSPrimingRequestStatus(aRequest);
>    nsCOMPtr<nsIHstsPrimingCallback> callback(mCallback);
>    mCallback = nullptr;
> +  
> +  Unused << aRequest->Cancel(NS_OK);

add a comment why you are doing this.

@@ +168,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo(dont_AddRef(static_cast<mozilla::LoadInfo*>
> +    (originalLoadInfo.get())->CloneForNewRequest()));

What was wrong with the original code?

CloneForNewRequest returns already_AddRefed<nsILoadInfo>, no need for dont_AddRef (actually, I'm quite surprised this even builds)

@@ +243,1 @@
>    

remove the trailing white space.

@@ +253,5 @@
>    // Start priming
>    rv = primingChannel->AsyncOpen2(primingListener);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  uri = nullptr;

you really don't need to do this.

Comment 130

[Kate McKinley [:kmckinley, :Kate]]

Attachment: interdiff to prior approved patches

Addressed feedback.

Comment 131

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Updated to match interdiff.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeb4030b2353

Comment 132

[Honza Bambas (:mayhemer)]

Maybe before you ask for review, double check your try run.

Can you provide just an interdiff from https://bugzilla.mozilla.org/attachment.cgi?id=8782575&action=edit ?

There still are failures:

Linux debug M(8)

Traceback (most recent call last):
ReferenceError: SpecialPowers is not defined
Assertion failure: mCacheInputStream, at /builds/slave/try-lx-d-000000000000000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:4662
TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/mochitest/mixedcontent/test_dynDelayedUnsecurePicture.html | application terminated with exit code 11
PROCESS-CRASH | security/manager/ssl/tests/mochitest/mixedcontent/test_dynDelayedUnsecurePicture.html | application crashed [@ mozilla::net::nsHttpChannel::ReadFromCache]
TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks! 


Linux debug M-e10s(bc3)

291 INFO TEST-UNEXPECTED-FAIL | dom/security/test/hsts/browser_hsts-priming_main.js | Test timed out -
293 INFO TEST-UNEXPECTED-FAIL | dom/security/test/hsts/browser_hsts-priming_main.js | Found a tab after previous test timed out: https://example.com/browser/dom/security/test/hsts/file_priming-top.html?host=test1.example.com&id=prime-hsts&type=img -

Comment 133

[Honza Bambas (:mayhemer)]

(I didn't check the whole try run!)

Comment 134

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Diff to prior

This patch fixes three things:

1) A small problem with the image cache - only disallow load from the cache if we would block the load.
2) setting mCallback to nullptr seems to be needed in some cases to allow the HSTSPrimerListener and the nsHttpChannel nsCOMPtrs to release the objects.
3) NS_WARN_IF return value not used, so change to NS_ERROR

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb2d93760d7

Comment 135

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Updated patch to current head

Comment 136

[:Cykesiopka]

Comment on attachment 8792143 [details] [diff] [review]
HSTS Priming PoC

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

Forgive the drive by review - I was skimming the PSM changes and noticed some minor issues.

::: netwerk/protocol/http/nsIHstsPrimingCallback.idl
@@ +41,5 @@
> +   *
> +   * May be invoked synchronously if HSTS priming has already been performed
> +   * for the host.
> +   *
> +   * param aError The error which caused this failure, or NS_ERROR_CONTENT_BLOCKED

Nit: @param ...

::: security/manager/ssl/nsISiteSecurityService.idl
@@ +183,5 @@
> +    /**
> +     * Mark a host as declining to provide a given security state so that features
> +     * such as HSTS priming will not flood a server with requests.
> +     *
> +     * @param aHost the hostname (punycode) that this applies to

This doesn't match the actual parameter.

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ +323,5 @@
>      return RemoveState(aType, aSourceURI, flags);
>    }
>  
> +  MOZ_ASSERT((aHSTSState == SecurityPropertySet
> +              || aHSTSState == SecurityPropertyNegative),

Nit: PSM style typically puts operators like || at the end of lines like so:
> MOZ_ASSERT(aHSTSState == SecurityPropertySet ||
>            aHSTSState == SecurityPropertyNegative)

::: security/manager/ssl/nsSiteSecurityService.h
@@ +34,5 @@
>  enum SecurityPropertyState {
>    SecurityPropertyUnset = 0,
>    SecurityPropertySet = 1,
> +  SecurityPropertyKnockout = 2,
> +  SecurityPropertyNegative = 3

Consider adding a trailing comma here if you feel like it.

Comment 137

[Honza Bambas (:mayhemer)]

Comment on attachment 8792142 [details] [diff] [review]
Diff to prior

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

Thanks!

try is green, go ahead :)

::: netwerk/protocol/http/HSTSPrimerListener.cpp
@@ +52,5 @@
>  HSTSPrimingListener::OnStopRequest(nsIRequest *aRequest,
>                                     nsISupports *aContext,
>                                     nsresult aStatus)
>  {
> +  mCallback = nullptr;

please add a comment why you are releasing the callback that late.

Comment 138

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Carried over r+ from previous

Comment 139

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

rebased patch

Comment 140

[Ryan VanderMeulen [:RyanVM]]

Those test_anchor_ping.html timeouts in your Try push look real to me. You're hitting them on nearly every platform and the OrangeFactor history for that bug shows no recent occurrences in production anywhere.

https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1289013&endday=2016-09-23&startday=2016-09-01&tree=trunk

Note that per the logs, it's timing out after:
1582 INFO -- running diff_origin_secure_referrer

Comment 141

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC

Disable priming for the ping tests as the servers the test creates can't handle the priming HEAD request.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da817608deb9

Comment 142

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/380eebfd9d89
HSTS Priming Proof of Concept. r=ckerschb, r=mayhemer, r=jld, r=smaug, r=dkeeler, r=jmaher, p=ally

Comment 143

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/380eebfd9d89

Comment 144

[Kate McKinley [:kmckinley, :Kate]]

Attachment: HSTS Priming PoC for Aurora 51

Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bc46a9f5587

Comment 145

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8798315 [details] [diff] [review]
HSTS Priming PoC for Aurora 51

Approval Request Comment
[Feature/regressing bug #]: Feature

[User impact if declined]: None

[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bc46a9f5587

I have worked with mwobensmith to make sure we have adequate automated coverage. More exploratory testing would be helpful as not many sites are expected to be able to take advantage of HSTS priming today.

New tests in dom/security/test/hsts cover the feature. All mixed-content tests throughout the code exercise the priming code unless specifically disabled. 

web-compat tests: pref'd to false because changing order of HSTS and mixed-content is not currently supported
dom/html/test/test_anchor_ping.html - server with promises hangs on HEAD request.
webconsole/test/browser_webconsole_block_mixedcontent_securityerrors.js - python BasicHttpServer throws an exception when given a HEAD request, even though the documentation suggests otherwise.

[Risks and why]: 
Users may perceive formerly blocked mixed-content loading, which may cause confusion.
HSTS priming may cause a noticeable delay in loading some pages the first time a user visits them, if the priming request times out.
The priming request may not be handled well, or may cause additional load on servers that do not support it well. If this is discovered, priming requests can be turned off.
This is a large and complex patch, and even though existing tests cover much of this functionality, new areas which rely on the current mixed-content behavior could be discovered.
In all cases, setting both preferences to false will prevent HSTS priming from occurring.

[String/UUID change made/needed]: No l10n changes. New UUID in nsIHstsPrimingCallback.idl, but no other changes.

HSTS priming is controlled by 2 preferences, security.mixed_content.use_hsts and security.mixed_content.send_hsts_priming. When security.mixed_content.use_hsts is false, mixed-content blocking happens before HSTS, as specified in the current Fetch standard. When security.mixed_content.send_hsts_priming is true, a priming request is sent to determine if the server supports HTTPS. security.mixed_content.use_hsts is set to false in release so that it will observe standard behavior in release, but users can turn it on if they like. If both are set to false, the feature will be effectively turned off and no additional requests will be sent.

I am the Engineering owner, so any problems would come directly to me.

The w3 public-webappsec list has been informed of the feature, but there isn't a marketing or comms push around it. There are no user-visible UI changes.

Comment 146

[Gerry Chang [:gchang]]

Comment on attachment 8798315 [details] [diff] [review]
HSTS Priming PoC for Aurora 51

Although this is a POC of new feature, we still want to get a picture about how it works in real world. Plus, the new tests are added and it also has prefs. security.mixed_content.use_hsts pref will be off in release which should not change the current behaviors. So, if the feature causes any unexpected behaviors or performance issues, we can always turn it off. Thus, let's take it in 51 aurora.

Comment 147

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7d42cef7968

Comment 148

[Mike Hommey [:glandium]]

(In reply to Carsten Book [:Tomcat] from comment #147)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/d7d42cef7968

FWIW, this changeset added .rej files to the tree.