Update getHSTSPreloadList.js to only filter out bulk domains.

VERIFIED FIXED in Firefox 59

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: code, Assigned: jcj)

Tracking

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 verified, firefox60 verified)

Details

Attachments

(4 attachments)

Reporter

Description

2 years ago
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
Component: Untriaged → Security: PSM
Product: Firefox → Core
Assignee

Comment 2

2 years ago
Excellent, thank you Lucas. Taking this.
Assignee: nobody → jjones
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
QA Contact: mwobensmith
Target Milestone: --- → mozilla59
Assignee

Updated

2 years ago
Blocks: 1391475
Comment hidden (mozreview-request)
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.
Attachment #8935177 - Flags: review?(dkeeler) → review+
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.
Attachment #8935177 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8943438 [details]
Bug 1418112 - Cleanup getHSTSPreloadList.js vars to lets  DONTBUILD NPOTB

https://reviewboard.mozilla.org/r/213766/#review219522
Attachment #8943438 - Flags: review?(dkeeler) → review+
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).
Attachment #8935177 - Flags: review?(dkeeler) → review+
Assignee

Comment 12

Last year
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 18

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

The diffs (versus central) look good to me, errors and inc.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

Last year
Pushed by dkeeler@mozilla.com:
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

Last year
mozreview-review
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/77dba1df6e9b
https://hg.mozilla.org/mozilla-central/rev/79a3f75d6a49
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED

Comment 26

Last year
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!
Assignee

Comment 27

Last year
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!
Target Milestone: mozilla59 → mozilla60

Comment 28

Last year
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.
Assignee

Comment 29

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

Comment 30

Last year
That'd be hugely helpful, thanks so much!
Assignee

Updated

Last year
Duplicate of this bug: 1391475

Comment 32

Last year
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.
Assignee

Comment 33

Last year
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
Attachment #8935177 - Flags: approval-mozilla-beta?
Assignee

Comment 34

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

Approval Request Comment

See Comment #33.
Attachment #8943438 - Flags: approval-mozilla-beta?
Assignee

Comment 35

Last year
(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

Last year
Looks like we both interpret "next week" to mean exactly 7 days later, as one should :)
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.
Attachment #8935177 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Attachment #8943438 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Updated

Last year
Blocks: 1435761
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!
Assignee

Comment 40

Last year
The results from Comment #39 are expected. Looks good to me.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.