Closed Bug 1424917 Opened 2 years ago Closed 2 years ago

Remove HSTS-Priming from codebase

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ckerschb, Assigned: kmckinley)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 4 obsolete files)

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/
Depends on: 1374453
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Assignee: nobody → kmckinley
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Blocks: 1425968
Attached patch Remove HSTS Priming from Firefox (obsolete) — Splinter Review
Attachment #8938095 - Flags: review?(honzab.moz)
Attachment #8938095 - Flags: review?(ckerschb)
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
Attachment #8938095 - Flags: review?(honzab.moz) → review+
Attached patch Remove HSTS Priming from Firefox (obsolete) — Splinter Review
Attachment #8938095 - Attachment is obsolete: true
Attachment #8938095 - Flags: review?(ckerschb)
Attachment #8939956 - Flags: review?(ckerschb)
(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 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
Attachment #8939956 - Flags: review?(ckerschb) → review+
Attached patch Remove HSTS Priming from Firefox (obsolete) — Splinter Review
rebased
Attachment #8939956 - Attachment is obsolete: true
Attachment #8940823 - Flags: review+
Attached patch Remove HSTS Priming from Firefox (obsolete) — Splinter Review
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
Attachment #8940823 - Attachment is obsolete: true
Attachment #8941267 - Flags: review+
Attachment #8941267 - Flags: feedback?(amarchesini)
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.
Attachment #8941267 - Flags: feedback?(amarchesini) → feedback+
Attachment #8941267 - Attachment is obsolete: true
Attachment #8941549 - Flags: review+
Keywords: checkin-needed
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?
Flags: needinfo?(kmckinley)
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
Flags: needinfo?(kate+bugzilla)
Keywords: checkin-needed
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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e3aeaa3c3c
Remove support for HSTS Priming. r=mayhemer, r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5e3aeaa3c3c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.