847621 - HSTS preload list: don't remove sites that were on the list but now can't be connected to

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

bsmith in bug 836097 comment 7:

> I think this is reasonable. However, I think we should make one
> modification: It is OK to remove entries automatically if we got a valid
> HTTP response from the server that does not include the HSTS header.
> However, if we didn't get a valid HTTP response from the server then we
> shouldn't remove the entry automatically; instead we should leave that
> origin's entry unchanged. This way, transient network failures will not
> result in entries getting dropped from the list.
> 
> In particular, I am worried about the idea that some site will be dropped
> from the preload list due to a networking error and nobody will notice
> before the release to fix it.

Comment 1

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

Attachment: patch

This patch prevents getHSTSPreloadList.js from dropping sites from the list if they were previously on the list but now can't be connected to.

Comment 2

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

Comment on attachment 720889 [details] [diff] [review]
patch

Camilo - Brian asked me to ask you to take a look at this to catch anything obvious I've done wrong before he looks at it. Thanks!

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #2)
> Comment on attachment 720889 [details] [diff] [review]
> patch
> 
> Camilo - Brian asked me to ask you to take a look at this to catch anything
> obvious I've done wrong before he looks at it. Thanks!

Let's reverse that. Camilo can do the real review and I will do the quick scan for any overlooked issues.

Comment 4

[Camilo Viecco (:cviecco)]

Comment on attachment 720889 [details] [diff] [review]
patch

Review of attachment 720889 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/tools/getHSTSPreloadList.js
@@ +196,5 @@
>      writeTo(getExpirationTimeString(), fos);
>      writeTo(PREFIX, fos);
>      for (var status of hstsStatuses) {
> +
> +      if (status.error == "could not connect to host" &&

What should be the behavior if there is an SSL problem? (TCP connect but SSL failure).
The current behavior is to drop if from the list, which I think is not our intent.

I think this calls the meta question:
In what circumstances we would be OK removing an entry from the list?

@@ +200,5 @@
> +      if (status.error == "could not connect to host" &&
> +          status.hostname in currentList) {
> +        dump("INFO: " + status.hostname + " could not be connected to - using previous status on list\n");
> +        writeTo(status.hostname + ": " + status.error + "\n", eos);
> +        status.maxAge = MINIMUM_REQUIRED_MAX_AGE;

also on fail--> should we update the time? or keep the old?

@@ +261,5 @@
> +  }
> +  return currentHosts;
> +}
> +
> +if (arguments.length < 1) {

move this section to after the  //******

Comment 5

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

Thanks for the review. Responses inline.

(In reply to Camilo Viecco (:cviecco) from comment #4)
> What should be the behavior if there is an SSL problem? (TCP connect but SSL
> failure).
> The current behavior is to drop if from the list, which I think is not our
> intent.
> 
> I think this calls the meta question:
> In what circumstances we would be OK removing an entry from the list?

I think it should definitely be okay to remove an entry from the list if we connect to it securely and it no longer sends the header or doesn't meet the max-age requirement.
I was also thinking that if the host is live but we can't securely connect or it returns nonsense, we can remove it from the list. My reasoning here is similar to the first point - the server is up but isn't sending the right information.
Finally, if the server is completely down or there's network troubles and we can't even connect, then we just skip it and keep the old information (so if it was already on the list, then it's still on the list).

> also on fail--> should we update the time? or keep the old?

I'm not sure what you mean here. Are you talking about the expiration time on the overall list? We should probably always update that. If you're talking about my use of the minimum max-age value here, this is sort-of a hack to get the later logic to consider that entry an okay one to use.

> move this section to after the  //******

Can do.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

FWIW, I agree with cviecco that we should only remove items after we've received a successful response (regardless of HTTP response code) that is missing the HSTS header and/or which has a too-short max-age.

If this ends up leaving too many zombies on the list, we can always change things to become more eager to remove things. But, generally, when in doubt, we should always err on the side of leaving a site on the list if it was already on the list, especially if it is still on Google's list.

And, ideally, removal from Google's list wouldn't result in automatic removal from our list.

Comment 7

[Reed Loden [:reed]]

(In reply to Brian Smith (:bsmith) from comment #6)
> And, ideally, removal from Google's list wouldn't result in automatic
> removal from our list.

Some companies/sites have specifically requested to be removed from Google's list (after being added for some period of time), so how would you handle those specific concerns?

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Reed Loden [:reed] from comment #7)
> (In reply to Brian Smith (:bsmith) from comment #6)
> > And, ideally, removal from Google's list wouldn't result in automatic
> > removal from our list.
> 
> Some companies/sites have specifically requested to be removed from Google's
> list (after being added for some period of time), so how would you handle
> those specific concerns?

They won't get added to our list unless they are sending an HSTS header with a long max-age. So, they can avoid being added to our list by using a shorter max-age and/or not using HSTS on their site.

Eventually, this HSTS preload list is going to be based on a much larger source of domains than Google's list. For example, I want us to work with some search engines and domain registrars to basically create a list of every domain on the internet to seed the list with. And, similarly, I want to do reverse-DNS on the entire public IPv4 address space and build the list from that. And similarly, when we find a eTLD+1 from one of these sources, we can ping (at least) common subdomains like www., mail., secure., to automatically add them. So, Google's list will eventually become a basically inconsequential source.

Comment 9

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

Attachment: patch v2

I looked more closely at the patch, and it turns out that as long as the connection does not give a valid http response, it will keep the entry on the list, which is the desired behavior, as I understand it.
I also implemented the 'keep it on the list even if it's no longer on Google's list, as long as it meets the other requirements' functionality.

Comment 10

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

Note to self: do the things in bug 844527 comment 6.

Comment 11

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

Attachment: patch v3

Ok - now the script has a limit on the number of concurrent outstanding requests and it automatically retries failed connections.

Comment 12

[Camilo Viecco (:cviecco)]

Comment on attachment 729674 [details] [diff] [review]
patch v3

Review of attachment 729674 [details] [diff] [review]:
-----------------------------------------------------------------

Also, can you add a comment on how to run this? for future people reference. A la:
// To run: 
// 1. [build firefox]
// 2. cd [source root]/security/manager/boot/src
// 3. LD_LIBRARY_PATH=[path to built libraries] [path to built binaries]/xpcshell ../../tools/getHSTSPreloadList.js < oldlist.inc

I think it is almost there.... (you can keep the recursive version but it is just hard to understand)

::: security/manager/tools/getHSTSPreloadList.js
@@ +236,5 @@
> +  return (response.error != "did not receive HSTS header" &&
> +          response.error != "no error" && response.retries > 0);
> +}
> +
> +function getHSTSStatuses(inHosts, outStatuses) {

This function seems overly complicated. Can this be replaced by(?) or am I missing something obvious:

  var numToDo=inHosts.length;
  var tmpOutput=[];

  for(var i=0;i<numToDo && i<MAX_CONCURRENT_REQUESTS ;i++){
    dump("spinning off '" + inHosts[0].name + "'\n");
    getHSTSStatus(inHosts.shift(), tmpOutput);
  }

  while(outStatuses.length<numToDo){
    waitForAResponse(tmpOutput);
    dump("got response from '" + tmpOutput[0].name + "'\n");
    var response= tmpOutput.shift();
    if (shouldRetry(response)){
       //push to pending queue
       dump("bad response from '" + response.name + ", retrying'\n")
       inHosts.push(response.name);
    }
    else {
      outStatuses.push(response);
    }
    if(inHosts.length>0){
      dump("spinning off '" + inHosts[0].name + "'\n");
      getHSTSStatus(inHosts.shift(), tmpOutput);
    }
  }

@@ +244,5 @@
> +  var tmpOutput = [];
> +  var retries = [];
> +  for (var i = 0; i < MAX_CONCURRENT_REQUESTS && inHosts.length > 0; i++) {
> +    dump("spinning off '" + inHosts[0].name + "'\n");
> +    getHSTSStatus(inHosts.shift(), tmpOutput, MAX_RETRIES);

if not cleaneable, then getHSTSStatus just requires two arguments.

Comment 13

[Reed Loden [:reed]]

(In reply to Camilo Viecco (:cviecco) from comment #12)
> Also, can you add a comment on how to run this? for future people reference.
> A la:
> // To run: 
> // 1. [build firefox]
> // 2. cd [source root]/security/manager/boot/src
> // 3. LD_LIBRARY_PATH=[path to built libraries] [path to built
> binaries]/xpcshell ../../tools/getHSTSPreloadList.js < oldlist.inc

It's preferred to use run-mozilla.sh over LD_LIBRARY_PATH...

./run-mozilla.sh /path/to/xpcshell /path/to/getHSTSPreloadList.js /path/to/oldlist.inc

Comment 14

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

Attachment: patch v4

Thanks for the review, Camilo. And thanks for the note about run-mozilla.sh, Reed - I thought that was just a relic of days long gone.

Comment 15

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

Comment on attachment 730750 [details] [diff] [review]
patch v4

Clearing review for re-doing getHSTSStatuses().

Comment 16

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

Attachment: patch v4.1

Comment 17

[Camilo Viecco (:cviecco)]

Comment on attachment 730969 [details] [diff] [review]
patch v4.1

Review of attachment 730969 [details] [diff] [review]:
-----------------------------------------------------------------

good work.

Comment 18

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

Comment on attachment 730969 [details] [diff] [review]
patch v4.1

Thanks! Off to Brian for final review.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 730969 [details] [diff] [review]
patch v4.1

Review of attachment 730969 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't review the whole the patch line-by-line, but I did spot one small issue.

::: security/manager/tools/getHSTSPreloadList.js
@@ +208,5 @@
>      writeTo(getExpirationTimeString(), fos);
>      writeTo(PREFIX, fos);
>      for (var status of hstsStatuses) {
> +
> +      if (status.error == "could not connect to host" &&

Do not repeat the literal value here. First, it is strange for the program to be using a human-readable error message. const ERROR_CONNECTING_TO_HOST = "could not connect to host" and then using ERROR_CONNECTING_TO_HOST consistently makes it less likely that somebody will break this by changing the formatting and/or text of the error message, thinking it is for humans only.

Comment 20

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

Thanks, Brian.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2a79a01127

Comment 21

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

Attachment: patch as checked in

Here's the patch as checked in. Carrying over r+ and sr+.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed2a79a01127

Comment 23

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

Attachment: follow-up patch

The SOURCE url was wrong in the checked-in patch. Here's a follow-up to fix that. r+ from bsmith via irc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed84d612a69

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ed84d612a69

Comment 25

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

Comment on attachment 736893 [details] [diff] [review]
patch as checked in

[Approval Request Comment]
Bug caused by (feature/regressing bug #): HSTS preload list auto-expiry
User impact if declined: need this patch to be able to run the automated HSTS preload list update script - otherwise, users won't be protected by our HSTS preloading mechanism
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6dfb5e9d5ab