Closed Bug 1424917 Opened 8 years ago Closed 8 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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: