1310955 - Too many HSTS priming requests are sent

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Review commit: https://reviewboard.mozilla.org/r/86622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86622/

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86622/diff/1-2/

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86622/diff/2-3/

Comment 5

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86622/diff/3-4/

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86622/diff/4-5/

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86622/diff/5-6/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d73b7dae598
Fix nsSiteSecurityService cache retrieval r=ckerschb,keeler

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d73b7dae598

Comment 11

[Alice0775 White]

[Tracking Requested - why for this release]: It need to fix Bug 1311807 on Aurora51.0a2

Could you uplift this to Aurora51.0a2?

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8802070 [details]
Bug 1310955 - Fix nsSiteSecurityService cache retrieval

Fix an issue related to HSTS priming. Take it in 51 aurora.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93fc874e1a2b

Comment 15

[Julien Cristau [:jcristau]]

Uplifted to aurora51, updating tracking flag