1424917 - Remove HSTS-Priming from codebase

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Christoph Kerschbaumer [:ckerschb]]

Eventually we should remove the code for HSTS Priming entirely, that would include Code within:

* nsMixedContentBlocker
* nsHttpBaseChannel (probably some base channel)
* LoadInfo
* ContentSecurityManager
* dom/security/tests/

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Remove HSTS Priming from Firefox

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8938095 [details] [diff] [review]
Remove HSTS Priming from Firefox

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

I went through the patches you've landed, but not to the last letter.  This looks ok, while leaving some of the sec-service API changes, that might be useful (if not even used already) on other places.

thanks!

::: dom/base/nsIDocument.h
@@ -3352,5 @@
>  
> -  // if nsMixedContentBlocker requires sending an HSTS priming request,
> -  // temporarily store that in the document so that it can be propogated to the
> -  // LoadInfo and eventually the HTTP Channel
> -  nsDataHashtable<nsURIHashKey, HSTSPrimingState> mHSTSPrimingURIList;

probably revert also https://bugzilla.mozilla.org/attachment.cgi?id=8795418&action=diff#a/dom/base/nsIDocument.h_sec2

::: dom/security/nsContentSecurityManager.cpp
@@ -516,5 @@
> -    rv = nsMixedContentBlocker::MarkLoadInfoForPriming(uri,
> -                                                       requestingContext,
> -                                                       aLoadInfo);
> -    return rv;
> -  }

also probably revert https://bugzilla.mozilla.org/attachment.cgi?id=8795418&action=diff#a/dom/security/nsContentSecurityManager.cpp_sec2

::: netwerk/base/nsNetUtil.cpp
@@ -2859,5 @@
>        }
>      } else {
>        Telemetry::Accumulate(Telemetry::HTTP_SCHEME_UPGRADE, 1);
>      }
> -  } else {

I think this 'else' should stay according https://reviewboard.mozilla.org/r/144334/diff/5#index_header

::: security/manager/ssl/nsISiteSecurityService.idl
@@ -311,5 @@
> -     */
> -    [noscript, must_use] void cacheNegativeHSTSResult(
> -      in nsIURI aURI, in unsigned long long aMaxAge,
> -      in const_OriginAttributesRef aOriginAttributes);
> -

should be removed too:

https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/security/manager/ssl/nsISiteSecurityService.idl#102

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ -128,5 @@
>      switch (source) {
>        case SourceUnknown:
>        case SourcePreload:
>        case SourceOrganic:
> -      case SourceHSTSPriming:

remove also:

https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/security/manager/ssl/nsSiteSecurityService.h#50

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Remove HSTS Priming from Firefox

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Updated from feedback.

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kate McKinley [:kmckinley, :Kate] from comment #4)
> Updated from feedback.
> 
> Treeherder:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=416e2727c3f6800a6e74d72731f769d1d1973ff4

test_anchor_ping.html looks pretty angry in that run :(

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8939956 [details] [diff] [review]
Remove HSTS Priming from Firefox

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

I compared all the changes from our initial patch where we landed priming [1]. It seems you got it all, r=me

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

::: dom/html/test/test_anchor_ping.html
@@ -50,2 @@
>      });
>    },

That test already caused problems when we landed Priming [see 1]. But the changes here really revert what we landed [2], not sure what's causing this problem, but we need to look into it. Maybe related to the recent referrer policy changes we landed (maybe worth consulting thomas).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1246540#c140
[2] https://hg.mozilla.org/mozilla-central/diff/380eebfd9d89/dom/html/test/test_anchor_ping.html

Comment 7

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Remove HSTS Priming from Firefox

rebased

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Remove HSTS Priming from Firefox

Changed the cross-origin anchor ping in dom/html/test/test_anchor_ping.html to use 127.0.0.1 (which is a Secure Context) instead of localhost. Otherwise the ping is blocked as mixed-content. Baku, can you take a look at that and let me know if that is the wrong fix?

new treeherder build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c4a3874a934e83cd8ccb0526a2b2ce06182a845

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8941267 [details] [diff] [review]
Remove HSTS Priming from Firefox

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

::: dom/security/nsMixedContentBlocker.cpp
@@ +881,3 @@
>    if (!aHadInsecureImageRedirect) {
>      if (XRE_IsParentProcess()) {
> +      AccumulateMixedContentHSTS(innerContentLocation, active,

same line here?

@@ +887,5 @@
>        mozilla::dom::ContentChild* cc = mozilla::dom::ContentChild::GetSingleton();
>        if (cc) {
>          mozilla::ipc::URIParams uri;
>          SerializeURI(innerContentLocation, uri);
> +        cc->SendAccumulateMixedContentHSTS(uri, active,

same line here?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2144,2 @@
>    OptionalURIParams redirectURI;
> +  nsresult rv;

unrelated change.

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Remove HSTS Priming from Firefox

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/logviewer.html#?job_id=155205944&repo=try looks like a related failure in that last Try run still, but maybe a pre-existing issue with the test?

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

I don't think it is related, I pushed another try run last night that looks clean:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c4a3874a934e83cd8ccb0526a2b2ce06182a845&selectedJob=155209955

Comment 13

[Kate McKinley [:kmckinley, :Kate]]

I attempted to reproduce both locally and with a loaner. Locally, I can't get it to fail at all. With the loaner, it always fails in step 2 of the --verify, but it fails with a timeout, not the indicated error.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e3aeaa3c3c
Remove support for HSTS Priming. r=mayhemer, r=ckerschb

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c5e3aeaa3c3c