Closed
Bug 1424917
Opened 8 years ago
Closed 8 years ago
Remove HSTS-Priming from codebase
Categories
(Core :: DOM: Security, enhancement, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: ckerschb, Assigned: kmckinley)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 4 obsolete files)
178.37 KB,
patch
|
kmckinley
:
review+
|
Details | Diff | Splinter Review |
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/
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → kmckinley
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8938095 -
Flags: review?(honzab.moz)
Attachment #8938095 -
Flags: review?(ckerschb)
![]() |
||
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8938095 -
Attachment is obsolete: true
Attachment #8938095 -
Flags: review?(ckerschb)
Attachment #8939956 -
Flags: review?(ckerschb)
Assignee | ||
Comment 4•8 years ago
|
||
Updated from feedback.
Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=416e2727c3f6800a6e74d72731f769d1d1973ff4
Comment 5•8 years ago
|
||
(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 :(
Reporter | ||
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
rebased
Attachment #8939956 -
Attachment is obsolete: true
Attachment #8940823 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8941267 -
Attachment is obsolete: true
Attachment #8941549 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•