1401796 - HSTS preloading entries get removed on "could not connect to host" errors

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Darkspirit]

same url (same diff), different anchors:
https://hg.mozilla.org/mozilla-central/rev/aa0877abb872#l1.961
> +futos.de: could not connect to host
https://hg.mozilla.org/mozilla-central/rev/aa0877abb872#l2.998
> -futos.de, 1

This isn't intented if you look here:
https://dxr.mozilla.org/mozilla-central/source/security/manager/tools/getHSTSPreloadList.js#254
>      // If we've encountered an error for this entry (other than the site not
>      // sending an HSTS header), be safe and don't remove it from the list
>      // (given that it was already on the list).


I switched some domains from dualstack to IPv6-only on port 443
(and Mozilla can't connect to IPv6 hosts because of bug 1309201):
* futos.de
* gabriele-kluge.de
(all removed from the preloading list in the above diff)

For example, this one was on the same IPv6 address, but also had an A record and was left in the list:
* perfektesgewicht.de

Comment 1

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

https://hg.mozilla.org/mozilla-central/rev/f3339e6a451e#l1.12

--- a/security/manager/tools/getHSTSPreloadList.js
+++ b/security/manager/tools/getHSTSPreloadList.js
@@ -254,43 +254,50 @@ function output(sortedStatuses, currentL
   try {
     var file = FileUtils.getFile("CurWorkD", [OUTPUT]);
     var errorFile = FileUtils.getFile("CurWorkD", [ERROR_OUTPUT]);
     var fos = FileUtils.openSafeFileOutputStream(file);
     var eos = FileUtils.openSafeFileOutputStream(errorFile);
     writeTo(HEADER, fos);
     writeTo(getExpirationTimeString(), fos);
     writeTo(PREFIX, fos);
-    for (var status of sortedStatuses) {
 
+    for (let status in sortedStatuses) {
       // If we've encountered an error for this entry (other than the site not
       // sending an HSTS header), be safe and don't remove it from the list
       // (given that it was already on the list).
       if (status.error != ERROR_NONE &&
           status.error != ERROR_NO_HSTS_HEADER &&
           status.error != ERROR_MAX_AGE_TOO_LOW &&
           status.name in currentList) {
         dump("INFO: error connecting to or processing " + status.name + " - using previous status on list\n");
         writeTo(status.name + ": " + errorToString(status) + "\n", eos);
         status.maxAge = MINIMUM_REQUIRED_MAX_AGE;
         status.includeSubdomains = currentList[status.name];
       }
+    }

sortedStatuses is an array, so `of` is what we want here.

Comment 2

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

Attachment: bug 1401796 - fix HSTS preload script to keep preexisting hosts if there was a connection error DONTBUILD NPOTB

Bug 1255425 changed an 'of' to an 'in', which caused the code that would keep
preexisting entries on the preload list if there was a connection error to loop
over the indices in the array and not the values themselves. Thanks, JavaScript.

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

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8911906 [details]
bug 1401796 - fix HSTS preload script to keep preexisting hosts if there was a connection error  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/183296/#review188450

Comment 4

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

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/218757467ee8
fix HSTS preload script to keep preexisting hosts if there was a connection error r=jcj DONTBUILD NPOTB

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/218757467ee8

Comment 7

[Darkspirit]

Verified fixed. Thank you.
https://hg.mozilla.org/mozilla-central/rev/3b5a9353f77c#l1.74
> +necesitodinero.org: could not connect to host
This domain was switched to IPv6-only after this fix had been pushed. There was a connection error, but it wasn't removed from the list, which is the desired behavior.

Comment 8

[Darkspirit]

Mistake. Sorry.
https://wiki.mozilla.org/SecurityEngineering/HTTP_Strict_Transport_Security_%28HSTS%29_Preload_List
> The versions of the list as it exists in the various channels of Firefox are available here: mozilla-central mozilla-aurora mozilla-beta mozilla-release mozilla-esr45.

https://dxr.mozilla.org/mozilla-beta/source/security/manager/ssl/nsSTSPreloadList.errors
Last Updated: 2017-09-21. futos.de is not preloaded, but we can see a connection error.

https://dxr.mozilla.org/mozilla-beta/source/security/manager/tools/getHSTSPreloadList.js#253
> or (let status in sortedStatuses) {
(wrong)

This looks like a beta uplift might be desirable.

Comment 9

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

The automated updates don't run for beta, so uplifting this wouldn't do anything.