1322044 - HSTS priming fail to load CSS and scripts for the first time

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
Open <https://www.ssllabs.com/ssltest/viewMyClient.html>.

Actual result:
Only Images and XMLHttpRequest rows are "Yes". Moreover, the location bar will have a lock with yellow triangle icon.

Expected result:
Images, CSS, Scripts, and XMLHttpRequest rows should be "Yes". The location bar should have a green lock icon.

Note: this problem is reproducible only for the first load. You will have to create a fresh profile or remove everything using the "Clear All History" dialog.

Comment 1

[Daniel Veditz [:dveditz]]

Kate: please take a look here.

Comment 2

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Screenshot showing Mixed-Content Handling table

As far as I can tell, this is working as intended. I looked at this page in Release (49), Developer Edition (52 with HSTS priming), my Nightly with 1313595 patched, and in Chrome Beta and they all show the same thing. I've attached the screenshot from my Nightly, but they all appear the same. In all cases, the UI shows as mixed content.

The server at https://plaintext.ssllabs.com returns inconsistent headers. If the t= query parameter is not passed, it returns a Strict-Transport-Security header with a max-age of 31536000. If the t= query parameter is passed, no STS header is returned. If I go to https://plaintext.ssllabs.com without the t= parameter and get the STS header, then I get the behavior described whereImages, Scripts, and XMLHttpRequest are all "Yes" and a green lock. This is as expected. If I never visit https://plaintext.ssllabs.com without the t= query parameter, as the page seems to do, I never get the Strict-Transport-Security header, and consistently get the Yes only on images and the mixed-content lock icon.

Even if https://plaintext.ssllabs.com consistently returned Strict-Transport-Security headers, the first load would incorrectly show the mixed-content lock icon due to bug 1275402.

In addition, the Mixed Content Handling box appears to be a faulty test because the stylesheet does get loaded over HTTPS instead of HTTP. The frame appears to be blocked by CSP and/or X-Frame-Options. There appears to be an HTTPS WebSocket connection to www.ssllabs.com, but I'm not sure that is the right WebSocket.

Please let me know if I am missing something!

Comment 3

[Masatoshi Kimura [:emk]]

It is not what I'm seeing. (Is bug 1313595 important?) I don't think other browsers or other Firefox channels result matter because only Nightly supports HSTS priming at the moment.

If Developer Tools does not tell a lie, I got the following plaintext.ssllabs.com accesses and responsees on the first load:
https://plaintext.ssllabs.com/plaintext/script.js?t=1482271781875&_=1482271781809
Strict-Transport-Security:"max-age=31536000"
https://plaintext.ssllabs.com/plaintext/style1.css?t=1482271781875
Strict-Transport-Security:"max-age=31536000"
https://plaintext.ssllabs.com/plaintext/1x1-transparent.png?t=1482271781875
Strict-Transport-Security:"max-age=31536000"
https://plaintext.ssllabs.com/plaintext/xhr.txt?t=1482271781875
Strict-Transport-Security:"max-age=31536000"

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: screenshot

Comment 5

[Kate McKinley [:kmckinley, :Kate]]

Digging into this a bit further, it looks like the reason is that the browser sees an STS header from the parent domain, ssllabs.com, which does not have includeSubDomains. When nsSiteSecurityService walks up the tree, it sees that plaintext.ssllabs.com does not have HSTS, then tries to resolve ssllabs.com. Since ssllabs.com exists and does not include subdomains, we return false for HSTS and true for cached.

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

https://reviewboard.mozilla.org/r/100354/#review101060

::: security/manager/ssl/nsSiteSecurityService.cpp:1040
(Diff revision 1)
>        if (siteState.mHSTSState == SecurityPropertySet) {
>          *aResult = aRequireIncludeSubdomains ? siteState.mHSTSIncludeSubdomains
>                                               : true;
> +        // Only set cached if this includes subdomains
> +        if (aCached && aRequireIncludeSubdomains) {
> +          if (!siteState.mHSTSIncludeSubdomains) {

I can't make the call whether this is the right thing to do or not, probably keeler is the better person to ask. Looks reasonable though.
NIT: instead of having the additional 'if' you could do:

if (aCached &&
    aRequireIncludeSubdomains &&
    !siteState.mHSTSIncludeSubdomains) {

Comment 8

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

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

https://reviewboard.mozilla.org/r/100354/#review100880

LGTM with comments addressed.

::: dom/security/test/hsts/browser_hsts-priming_include-subdomains.js:27
(Diff revision 1)
> +
> +  let which = "block_active";
> +
> +  SetupPrefTestEnvironment(which);
> +
> +  yield execute_test("top-level", test_settings[which].mimetype);

As far as I can tell, this second argument is unused in execute_test, which is a little strange.

::: dom/security/test/hsts/browser_hsts-priming_include-subdomains.js:31
(Diff revision 1)
> +
> +  yield execute_test("top-level", test_settings[which].mimetype);
> +
> +  yield execute_test("prime-hsts", test_settings[which].mimetype);
> +
> +  ok("prime-hsts" in test_settings[which_test].priming, "Saw a priming request on the subdomain when the domain does not includeSubDomains");

nit: long line
Also, since `which_test` is indirectly set, it's not immediately clear what it's supposed to be here. Maybe just use `which`?

::: security/manager/ssl/nsSiteSecurityService.cpp:1038
(Diff revision 1)
>          *aCached = true;
>        }
>        if (siteState.mHSTSState == SecurityPropertySet) {
>          *aResult = aRequireIncludeSubdomains ? siteState.mHSTSIncludeSubdomains
>                                               : true;
> +        // Only set cached if this includes subdomains

Let's actually do this all at once where we set *aCached starting on line 1032:

    if (aCached) {
      *aCached = aRequireIncludeSubdomains ? siteState.mHSTSIncludeSubdomains
                                           : true;
    }

Comment 9

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 11

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 13

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 14

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8820976 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 15

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 16

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking... is this still an issue or did the recent aurora uplift fix it in bug 1328460 ?

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Virtual, what aurora build are you using?

Comment 19

[Kate McKinley [:kmckinley, :Kate]]

This is not landed, and is a different issue than bug 1328460. The patch works, but is very leaky for some reason I haven't tracked down yet, so it's not complete.

Comment 20

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@ Liz Henry (:lizzard) - I'm Nightly user and I can still reproduce part of the issue.

Comment 21

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 22

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8854113 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8854113 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

https://reviewboard.mozilla.org/r/126086/#review129028

Please answer my question and flag me for review again.

::: security/manager/ssl/nsSiteSecurityService.cpp:1302
(Diff revision 2)
>      bool expired = dynamicState->IsExpired(nsISiteSecurityService::HEADER_HSTS);
>      if (!expired) {
>        if (dynamicState->mHSTSState == SecurityPropertySet) {
> -        *aResult = aRequireIncludeSubdomains ? dynamicState->mHSTSIncludeSubdomains
> +        *aResult = aRequireIncludeSubdomains ?
> +                                        dynamicState->mHSTSIncludeSubdomains
> -                                             : true;
> +                                        : true;

just curious: why did you change those lines? they did not go over 80 chars, did they? Please change back to not pollute history logs unecessarily.

::: security/manager/ssl/nsSiteSecurityService.cpp:1308
(Diff revision 2)
> +        if (aCached) {
> +          // Only set cached if this includes subdomains
> +          *aResult = aRequireIncludeSubdomains ?
> +                                          dynamicState->mHSTSIncludeSubdomains
> +                                          : true;
> +        }

that doesn't seem right. In all the other cases you set *aChached within the aCache branch, but within this branch you assign to *aResult. Btw, you already assign to *aResult the same value right before entering the if (aCached) branch.

Please clarify.

Comment 24

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8854113 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8854113 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

https://reviewboard.mozilla.org/r/126086/#review129032

That's better, thanks.

Comment 26

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

Comment on attachment 8854113 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

https://reviewboard.mozilla.org/r/126086/#review129062

Looks good to me with comments addressed.

::: dom/security/test/hsts/browser_hsts-priming_include-subdomains.js:1
(Diff revision 3)
> +/*

Does this need a license block and other boilerplate?

::: dom/security/test/hsts/browser_hsts-priming_include-subdomains.js:32
(Diff revision 3)
> +  yield execute_test("top-level", test_settings[which].mimetype);
> +
> +  yield execute_test("prime-hsts", test_settings[which].mimetype);
> +
> +  ok("prime-hsts" in test_settings[which].priming,
> +    "HSTS priming on a subdomain when top-level does not includeSubDomains");

nit: indent one more space

::: dom/security/test/hsts/head.js:434
(Diff revision 3)
>  async function execute_test(test, mimetype) {
>    var src = build_test_uri(TOP_URI, test_servers[test].host,
>        test, test_settings[which_test].type,
>        test_settings[which_test].timeout);
>  
> -  let tab = openTab(src);
> +  await BrowserTestUtils.withNewTab(src, function(){return;});

nit: I don't think you need the `return;` here (in fact, I think the shortest thing would be `() => {}` if this argument is required).

::: dom/security/test/hsts/head.js:434
(Diff revision 3)
>  async function execute_test(test, mimetype) {
>    var src = build_test_uri(TOP_URI, test_servers[test].host,
>        test, test_settings[which_test].type,
>        test_settings[which_test].timeout);
>  
> -  let tab = openTab(src);
> +  await BrowserTestUtils.withNewTab(src, function(){return;});

Is openTab still used? In any case, seems like this cleanup might be more appropriate to a follow-up bug.

::: security/manager/ssl/nsSiteSecurityService.cpp:1250
(Diff revision 3)
>    if (siteState->mHSTSState != SecurityPropertyUnset) {
>      SSSLOG(("Found HSTS entry for %s", aHost.get()));
>      bool expired = siteState->IsExpired(nsISiteSecurityService::HEADER_HSTS);
>      if (!expired) {
>        SSSLOG(("Entry for %s is not expired", aHost.get()));
>        if (aCached) {

Shouldn't this whole `if (aCached) ...` block go inside the `if (siteState->mHSTSState == SecurityPropertySet) ...` block? (i.e. it should match the code below, on line 1302)

::: security/manager/ssl/nsSiteSecurityService.cpp:1263
(Diff revision 3)
>          return true;
>        } else if (siteState->mHSTSState == SecurityPropertyNegative) {
>          *aResult = false;
> +        if (aCached) {
> +          // if it's negative, it is always cached
> +          *aCached = true;

In a perfect world with infinite developer time, I would ask for tests that cover this case, as well as the dynamic and static preload cases.

::: security/manager/ssl/nsSiteSecurityService.cpp:1337
(Diff revision 3)
>        (preload = GetPreloadListEntry(aHost.get())) != nullptr) {
>      SSSLOG(("%s is a preloaded HSTS host", aHost.get()));
>      *aResult = aRequireIncludeSubdomains ? preload->mIncludeSubdomains
>                                           : true;
>      if (aCached) {
>        *aCached = true;

We need the same logic here, right? (i.e. `aRequreIncludeSubdomains ? preload->mIncludeSubdomains : true`)

Comment 27

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8854113 [details]
Bug 1322044 - Only mark a subdomain cached when includeSubDomains is true

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2501f21f3fea
Only mark a subdomain cached when includeSubDomains is true r=ckerschb,keeler

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2501f21f3fea

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Is this something we need to uplift to Aurora for Fx54 or can it ride the Fx55 train?

Comment 31

[Gerry Chang [:gchang]]

Can we let this ride the train on 55 because 54 is about to be in beta? 54 will be marked as disabled soon.

Comment 32

[Kate McKinley [:kmckinley, :Kate]]

I would rather keep it for 55 since HSTS priming is disabled on beta/release anyway. I ported the patch to 54 and ran a try run yesterday, and there were issues related to the patch that do not exist in 55.

Comment 33

[Gerry Chang [:gchang]]

Per comment# 32, 54 will go to beta soon, mark 54 disabled.