1418112 - Update getHSTSPreloadList.js to only filter out bulk domains.

Status: VERIFIED FIXED

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

Description

[Lucas Garron]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3269.3 Safari/537.36




Expected results:

As of yesterday, the Chromium preload list [1] now includes a "policy" field for each entry.

I would like to propose that getHSTSPreloadList.js [2] takes these fields into account by only filtering HSTS entries with a policy that starts with "bulk" (currently: bulk-legacy, bulk-18-weeks, bulk-1-year) and unconditionally taking other HSTS entries.

This will allow Mozilla (and Microsoft) to pick up TLDs as well as eTLD-owner-requested domains (i.e. .gov domains that are preloaded at registration time even if the site does not immediately exist) automatically.

The difference in filtered list size for should be <2% and shrinking, since the non-bulk entries are added manually (per the policies policies at [3]).

[1] https://code.google.com/p/chromium/codesearch#chromium/src/net/http/transport_security_state_static.json
[2] https://dxr.mozilla.org/mozilla-central/source/security/manager/tools/getHSTSPreloadList.js
[3] https://github.com/chromium/hstspreload.org/wiki/Preload-List-Processes#manual-hsts-entries

Comment 1

[Lucas Garron]

Discussion thread: https://groups.google.com/a/chromium.org/forum/#!topic/hsts-discuss/XzMpDMqtkKc

Comment 2

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

Excellent, thank you Lucas. Taking this.

Comment 3

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

Attachment: Bug 1418112 - Force HSTS Preloading for opt-in public suffixes DONTBUILD NPOTB

The Chromium HSTS Preload list now includes a "policy" field that we can use
to filter and force some HSTS entries. This patch unconditionally accepts list
entries with the "google" or "public-suffix-requested" policies, and tests all
others via the existing connect-and-check (with failback tolerance) strategy.

In comment #0 of this bug [2], Lucas recommends we also filter the "all others"
to be the "bulk" entries. This patch does not do that to be conservative and
avoid de-listing sites at this time. We'll probably want a follow-on to
evaluate and potentially do that.

[1] https://github.com/chromium/hstspreload.org/wiki/Preload-List-Processes#manual-hsts-entries
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1418112#c0

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

Comment 4

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/206036/#review212012

Looks good to me.

Comment 5

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

Attachment: This is the diff of the run of the updated tool versus current m-c's nsSTSPreloadList.inc

Comment 6

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

Attachment: And the errors diff versus m-c

Comment 7

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/206036/#review212112

Turns out I was wrong - we need to fix up how we're doing includeSubdomains here.

::: security/manager/tools/getHSTSPreloadList.js:96
(Diff revision 1)
>        if (entry.name) {
>          // We trim the entry name here to avoid malformed URI exceptions when we
>          // later try to connect to the domain.
>          entry.name = entry.name.trim();
>          entry.retries = MAX_RETRIES;
>          entry.originalIncludeSubdomains = entry.include_subdomains;

Now that we're not connecting to hosts that are force-included, we shouldn't need the originalIncludeSubdomains field at all.

::: security/manager/tools/getHSTSPreloadList.js:293
(Diff revision 1)
>      // current list significantly easier when we go to update the list.
>      for (let status of includedStatuses) {
>        let incSubdomainsBool = (status.forceInclude && status.error != ERROR_NONE
>                                 ? status.originalIncludeSubdomains
>                                 : status.includeSubdomains);
>        status.finalIncludeSubdomains = incSubdomainsBool;

Similarly, we don't need the finalIncludeSubdomains either, if I have this right now.

::: security/manager/tools/getHSTSPreloadList.js:441
(Diff revision 1)
> +  // will be included without being checked (forced); the others will be
> +  // checked using active probing.
> +  for (let host of inHosts) {
> +    if (host.policy == "google" || host.policy == "public-suffix-requested") {
> +      host.forceInclude = true;
> +      host.error = ERROR_NONE;

We actually need to set the includeSubdomains property here based on the host entry's include_subdomains value.

Comment 8

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

Attachment: Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets DONTBUILD NPOTB

This is a JS style cleanup; it changes all relevant `var` decls to `let`, and
also moves the `gSSService` up to the top where globals should go.

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

Comment 9

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

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

Comment 10

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

Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/213766/#review219522

Comment 11

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/206036/#review219520

Ok - I agree this looks correct, pending the results of it running.

::: security/manager/tools/getHSTSPreloadList.js
(Diff revisions 1 - 2)
>             maxAge: maxAge.value,
> -           includeSubdomains: includeSubdomains.value,
> +           include_subdomains: include_subdomains.value,
>             error,
>             retries: host.retries - 1,
> -           forceInclude,
> +           forceInclude };
> -           originalIncludeSubdomains: host.originalIncludeSubdomains };

For the naming convention, I'd prefer 'includeSubdomains' over 'include_subdomains' (and we should probably document the difference) (which I think means we have to explicitly set it in getHosts).

Comment 12

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/206036/#review219520

It worked great. Run results (+ diffs) in https://gist.github.com/819021b50b67a574e8587201bdd684e0

> For the naming convention, I'd prefer 'includeSubdomains' over 'include_subdomains' (and we should probably document the difference) (which I think means we have to explicitly set it in getHosts).

OK. I'll fold that into the patch.

Comment 13

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

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

Comment 14

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

Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

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

Comment 15

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

Hmmm - this is at least a logging issue, if not a bug:

4268c4345
< everytrycounts.gov: could not connect to host
---
> everytrycounts.gov: undefined (error ignored - included regardless)

Comment 16

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

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

Comment 17

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

Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

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

Comment 18

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

Run results #2 up at https://gist.github.com/jcjones/7f5a7cbfe3f8d2d595ba3ee2bca9db3b

The diffs (versus central) look good to me, errors and inc.

Comment 19

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

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

Comment 20

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

Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

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

Comment 21

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

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

Comment 22

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

Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77dba1df6e9b
Force HSTS Preloading for opt-in public suffixes r=keeler DONTBUILD NPOTB
https://hg.mozilla.org/integration/autoland/rev/79a3f75d6a49
Cleanup getHSTSPreloadList.js vars to lets r=keeler DONTBUILD NPOTB

Comment 24

[Code Review Bot [:reviewbot]]

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/206036/#review220384


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: security/manager/tools/getHSTSPreloadList.js:278
(Diff revision 5)
>          writeTo(status.name + ": " + errorToString(status) + "\n", eos);
>          return false;
>        }
>  
> -      dump("INFO: " + status.name + " ON the preload list\n");
> +      dump("INFO: " + status.name + " ON the preload list (includeSubdomains: "
> +           + status.includeSubdomains +"\n");

Error: Infix operators must be spaced. [eslint: space-infix-ops]

Comment 25

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/77dba1df6e9b
https://hg.mozilla.org/mozilla-central/rev/79a3f75d6a49

Comment 26

[Ben McIlwain]

I have a question about the timing of this code change as relates to the timing of the release cycle.

I correctly see the new HSTS-preloaded TLDs on the central/nightly branch but I don't see them on the beta branch, even though this issue is tagged for inclusion in Firefox 59.

Compare:
https://hg.mozilla.org/mozilla-central/file/tip/security/manager/ssl/nsSTSPreloadList.inc
https://hg.mozilla.org/releases/mozilla-beta/file/tip/security/manager/ssl/nsSTSPreloadList.inc

(Grep for "insurance," as one example of new HSTS-preloaded TLDs; it's on the former but not the latter.)

My best guess as to this discrepancy is that, since the validation script runs once per day, the change to the script itself was committed in time to make the cut-off for Firefox 59, but the script wasn't run in time for its updated output to make it in as well.  Is that a correct understanding of the situation?  And if so, would it be possible to merge the 7 new TLDs forward to the version of the HSTS preload list targeted for Firefox 59, so that they can land in March as intended?

Or, are changes to the HSTS preload list continually merged in to the branch up until some cut-off before release?  That would seem reasonable to me, as a fresher preload list is better, and does not carry the same risks of introducing bugs as code changes do.  If that's the case, then it seems no further action would be required.

Thanks for your help!

Comment 27

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

This update will only apply to Firefox 60; the code wasn't stable enough fast enough to get it in to 59 at the end of that cycle.

That said, you are correct that the preload list will be updated by this script and that update merged in periodically. However, only the 60 branch and beyond will use the newly-updated script to pull in the preloaded TLDs. (At least that is true unless we choose to uplift this into Beta.)

I'm updating the target field here in the bug to mark it properly for 60 -- the "fixed" flag was already accurate.

Thanks for the question!

Comment 28

[Ben McIlwain]

To the extent that my say matters, can I put in a vote for uplifting at least the inclusion of the new TLDs in the preload list itself to the 59 release?  Some of these TLDs are already launched and some will be launching in the near future, on a timeframe where the difference between Firefox 59 and 60 release dates is significant.  Thus there would be meaningful security improvements delivered to users by getting these additions to the list out sooner.

It's worth noting that .google had already been manually added to the HSTS preload list some time ago, before the script supported it.  So there'd only be one last manual vetting/addition before turning it over to the new script entirely automatically for subsequent releases.  My guess would be that that approach would be lower risk than uplifting the entire change to the script to the new release, but of course that determination is yours to make.

Comment 29

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

We can file for uplift next week after letting the script and its results run in Nightly for a time.

Comment 30

[Ben McIlwain]

That'd be hugely helpful, thanks so much!

Comment 32

[Ben McIlwain]

How's it looking for uplift now?  Near as I can tell everything is working fine.  Recent diffs look good, with entries continuing to be added/removed from the list based on whether the hosts are responding, and the TLDs remain included on the list.

Comment 33

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

Approval Request Comment
[Feature/Bug causing the regression]: n/a

[User impact if declined]: No preloaded HSTS coverage for newly-enrolled eTLDs until Firefox 60

[Is this code covered by automated tests?]: n/a

[Has the fix been verified in Nightly?]: Yes, for the last 7 days

[Needs manual test from QE? If yes, steps to reproduce]: Ensure sites such as http://register.bank/ and  http://register.insurance/ preload. A valid way to confirm is to attempt to load http://insurance/ and watch Firefox fail to load https://insurance/ (as there's no website there.)

[List of other uplifts needed for the feature/fix]: None, this is a script change.

[Is the change risky?]: No

[Why is the change risky/not risky?]: It's adding metadata to the nsSTSPreloadList.inc file to include the new eTLDs rather than changing Gecko. Gecko's parser and HSTS has remained the same between 59 and 60, so essentially we're just taking an update to the preload list like normal.

[String changes made/needed]: None

Comment 34

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

Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

Approval Request Comment

See Comment #33.

Comment 35

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

(In reply to Ben McIlwain from comment #32)
> How's it looking for uplift now?  Near as I can tell everything is working
> fine.  Recent diffs look good, with entries continuing to be added/removed
> from the list based on whether the hosts are responding, and the TLDs remain
> included on the list.

Heh, comments passed mid-flight. This is an irregular thing to uplift to Beta, but we'll see what release management thinks of it.

Comment 36

[Ben McIlwain]

Looks like we both interpret "next week" to mean exactly 7 days later, as one should :)

Comment 37

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

Comment on attachment 8935177 [details]
Bug 1418112 - Force HSTS Preloading for opt-in public suffixes  DONTBUILD NPOTB

OK. I got your email as well. Makes sense, let's uplift this for 59 beta 6.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e4109662e394
https://hg.mozilla.org/releases/mozilla-beta/rev/e3770cfe2187

Comment 39

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Please see below my results while comparing Nightly 59.0a1 2017-11-17 with Firefox 59 beta 7 and latest Nightly under Win 10 64-bit:

Nightly 59.0a1
- http://register.bank/ redirects to https://www.register.bank/
- http://register.insurance/ redirects to https://www.register.insurance
- http://insurance/ redirects to https://www.insurance.com/
- http://bank redirects to http://www.bank.com/

Firefox 59 beta 7 and Nightly 60.0a1 2018-02-04:
- http://register.bank/ redirects to https://www.register.bank/ 
- http://register.insurance/ redirects to https://www.register.insurance/ 
- http://insurance/ redirects to https://insurance/ (site doesn't exist)
- http://bank redirects to https://bank/ (site doesn't exist)

Please confirm whether these are the expected results and if other tests are required. Thank you!

Comment 40

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

The results from Comment #39 are expected. Looks good to me.

Comment 41

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Thanks!

Also verified with Firefox 59 beta 7 and Nightly 60.0a1 2018-02-07 under Mac OS X 10.13 and Ubuntu 16.04 32-bit.