Too many HSTS priming requests are sent

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kmckinley, Assigned: kmckinley)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51+ fixed, firefox52 fixed)

Details

(Whiteboard: [domsecurity-active] [hsts-priming])

Attachments

(1 attachment)

With HSTS priming, failures are being cached properly, but the fact that it is cached is not correctly reported.

SiteHSTSState is missing SecurityPropertyNegative

https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsSiteSecurityService.cpp?q=%2Bfunction%3A%22SiteHSTSState%3A%3ASiteHSTSState%28PRTime%2C+SecurityPropertyState%2C+bool%29%22&redirect_type=single#64
Depends on: 1305993
Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

https://reviewboard.mozilla.org/r/86622/#review85698

r=me for the nsSiteSecurityService.cpp change, but I don't think I'm the best reviewer for the test changes.
Attachment #8802070 - Flags: review?(dkeeler) → review+
Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

https://reviewboard.mozilla.org/r/86622/#review85822

r=me, thanks! As discussed over email, let's keep it simple here since the patch needs to be uplifted and in a follow up we can clean up the tests a little more.

::: dom/security/test/hsts/browser_hsts-priming_no-duplicates.js:22
(Diff revision 5)
> +
> +  for (let server of Object.keys(test_servers)) {
> +    yield execute_test(server, test_settings[which].mimetype);
> +  }
> +
> +  for (let server of Object.keys(test_servers)) {

nit: you could add a comment that running the tests again is required to trigger the right results.
Attachment #8802070 - Flags: review?(ckerschb) → review+
Keywords: checkin-needed
Whiteboard: [domsecurity-active]
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6d73b7dae598
Fix nsSiteSecurityService cache retrieval r=ckerschb,keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d73b7dae598
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1311807
[Tracking Requested - why for this release]: It need to fix Bug 1311807 on Aurora51.0a2

Could you uplift this to Aurora51.0a2?
Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Approval Request Comment
[Feature/regressing bug #]:
1246540
[User impact if declined]:
Browser will send HSTS priming with each mixed-content instead of using cached results
[Describe test coverage new/current, TreeHerder]:
A test was written to ensure that duplicate requests are not sent and is included in this patch.
[Risks and why]:
The risk of including this patch is very small as it is a one-line change to nsSiteSecurityService. It is difficult to say what the risk would be.
Not including this severely reduces performance (see 1311807 and 1308612 for examples).
[String/UUID change made/needed]:
None
Attachment #8802070 - Flags: approval-mozilla-aurora?
Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Fix an issue related to HSTS priming. Take it in 51 aurora.
Attachment #8802070 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Uplifted to aurora51, updating tracking flag
Whiteboard: [domsecurity-active] → [domsecurity-active] [hsts-priming]
You need to log in before you can comment on or make changes to this bug.