Closed Bug 1322044 Opened 3 years ago Closed 3 years ago

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

Categories

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

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- disabled
firefox52 --- disabled
firefox-esr52 --- disabled
firefox53 + disabled
firefox54 + disabled
firefox55 + fixed

People

(Reporter: emk, Assigned: kmckinley)

References

()

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(3 files, 2 obsolete files)

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.
Kate: please take a look here.
Assignee: nobody → kmckinley
Flags: needinfo?(kmckinley)
Priority: -- → P2
Whiteboard: [domsecurity-active]
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!
Flags: needinfo?(kmckinley) → needinfo?(VYV03354)
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"
Flags: needinfo?(VYV03354) → needinfo?(kmckinley)
Attached image screenshot
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.
Status: NEW → ASSIGNED
Flags: needinfo?(kmckinley)
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) {
Attachment #8820976 - Flags: review?(ckerschb) → review+
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;
    }
Attachment #8820976 - Flags: review?(dkeeler) → review+
Attachment #8837879 - Attachment is obsolete: true
Attachment #8837879 - Flags: review?(dkeeler)
Attachment #8837879 - Flags: review?(ckerschb)
Tracking... is this still an issue or did the recent aurora uplift fix it in bug 1328460 ?
Flags: needinfo?(kmckinley)
Virtual, what aurora build are you using?
Flags: needinfo?(Virtual)
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.
Flags: needinfo?(kmckinley)
@ Liz Henry (:lizzard) - I'm Nightly user and I can still reproduce part of the issue.
Flags: needinfo?(Virtual)
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.
Attachment #8854113 - Flags: review?(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.
Attachment #8854113 - Flags: review?(ckerschb) → review+
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`)
Attachment #8854113 - Flags: review?(dkeeler) → review+
Pushed by kmckinley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2501f21f3fea
Only mark a subdomain cached when includeSubDomains is true r=ckerschb,keeler
https://hg.mozilla.org/mozilla-central/rev/2501f21f3fea
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Is this something we need to uplift to Aurora for Fx54 or can it ride the Fx55 train?
Flags: needinfo?(kmckinley)
Attachment #8820976 - Attachment is obsolete: true
Flags: needinfo?(kmckinley)
Can we let this ride the train on 55 because 54 is about to be in beta? 54 will be marked as disabled soon.
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.
Per comment# 32, 54 will go to beta soon, mark 54 disabled.
You need to log in before you can comment on or make changes to this bug.