Last Comment Bug 800444 - disable the HSTS preload list if the list has gone 18 weeks without an update
: disable the HSTS preload list if the list has gone 18 weeks without an update
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: David Keeler [:keeler] (use needinfo?)
:
Mentors:
Depends on: 786417
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 10:50 PDT by David Keeler [:keeler] (use needinfo?)
Modified: 2013-03-01 14:26 PST (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+
fixed
+
fixed


Attachments
patch (10.71 KB, patch)
2012-10-12 16:43 PDT, David Keeler [:keeler] (use needinfo?)
brian: review-
Details | Diff | Review
patch v2 (6.38 KB, patch)
2012-10-18 11:04 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Review
patch v3 (10.79 KB, patch)
2012-10-23 14:41 PDT, David Keeler [:keeler] (use needinfo?)
brian: review+
Details | Diff | Review
patch v4 (11.83 KB, patch)
2012-10-24 16:03 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Review
patch v5 (11.87 KB, patch)
2012-10-24 16:27 PDT, David Keeler [:keeler] (use needinfo?)
honzab.moz: review+
Details | Diff | Review
patch v6 (12.22 KB, patch)
2012-10-26 09:57 PDT, David Keeler [:keeler] (use needinfo?)
dkeeler: review+
Details | Diff | Review
patch for uplift (6.51 KB, patch)
2012-10-29 10:37 PDT, David Keeler [:keeler] (use needinfo?)
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description David Keeler [:keeler] (use needinfo?) 2012-10-11 10:50:01 PDT
The entries on the HSTS preload list have a maxAge of >= 18 weeks. So, if we go 18 weeks without updating, those entries may no longer be valid, and we should stop using them.
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-11 11:03:17 PDT
Tracking 18 and 17 because we're definitely going to ask to uplift this.
Comment 2 Alex Keybl [:akeybl] 2012-10-12 15:39:06 PDT
(In reply to Brian Smith (:bsmith) from comment #1)
> Tracking 18 and 17 because we're definitely going to ask to uplift this.

Requesting uplift in the future != tracking for the release. What's the user impact if you all forget to fix this in time for FF17?
Comment 3 David Keeler [:keeler] (use needinfo?) 2012-10-12 16:43:05 PDT
Created attachment 670984 [details] [diff] [review]
patch

This would be the version to go on trunk, not to uplift. I'll get to that as soon as I can.
Comment 4 David Keeler [:keeler] (use needinfo?) 2012-10-12 16:43:42 PDT
Forgot to mention, this depends on the patch in bug 786417.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-12 19:01:14 PDT
Comment on attachment 670984 [details] [diff] [review]
patch

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +33,5 @@
>  #define STS_UNSET (nsIPermissionManager::UNKNOWN_ACTION)
>  #define STS_KNOCKOUT (nsIPermissionManager::DENY_ACTION)
>  
> +// This is 18 weeks
> +#define MAX_SECONDS_WITHOUT_UPDATE (10886400)

Remove this, and...

@@ +125,5 @@
> +   // Disable the preload list if we haven't updated in 18 weeks.
> +   // If we couldn't get an appInfo, updateTime will be 0 and we will disable
> +   // the preload list, because we have no idea when we were last updated.
> +   // This can be overridden by setting the pref back to true after this
> +   // service has been initialized.

In the Init() of nsStrictTransportSecurityService, you can do something like this:

    if (!mozilla::Preferences::GetTime("test.currentTimeOverride",
                                       &mPreloadCutoffTime)) {
        static const PRTime MAX_SECONDS_WITHOUT_UPDATE 
                           = (18 * 7 * 24 * 60 * 60);
        mPreloadCutOffTime = GRE_BUILDID + MAX_SECONDS_WITHOUT_UPDATE;
    }

Then, right before you are about to search the preload list:

    // Don't search the preload list if too much time has passed since the
    // last update.
    PRTime now = PR_Now();
    if (now > mPreloadCutOffTime) {
        return NS_OK;
    }

We would need to add this to the Makefile.in:

DEFINES += \
    -DGRE_BUILDID=$(GRE_BUILDID) \
    $(NULL)

Then you can avoid the parsing and whatnot above. Then, in your tests, you can just set the test.currentTimeOverride preference to simulate a time in the future.

If you agree this is a better way of doing things, then I will write a patch and you can review it.

@@ +128,5 @@
> +   // This can be overridden by setting the pref back to true after this
> +   // service has been initialized.
> +   PRTime now = PR_Now();
> +   if ((updateTime + (MAX_SECONDS_WITHOUT_UPDATE * PR_USEC_PER_SEC)) < now) {
> +     mozilla::Preferences::SetBool("network.stricttransportsecurity.preloadlist", false);

Wouldn't this permanently disable the preload list?

We should keep the preload list enabled, but we should just ignore the preload list entries when GRE_BUILDID + MAX_SECONDS_WITHOUT_UPDATE < now.
Comment 6 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-12 19:04:16 PDT
(In reply to Brian Smith (:bsmith) from comment #5)
>     if (!mozilla::Preferences::GetTime("test.currentTimeOverride",
>                                        &mPreloadCutoffTime)) {
>         static const PRTime MAX_SECONDS_WITHOUT_UPDATE 
>                            = (18 * 7 * 24 * 60 * 60);
>     }

This is wrong, because test.currentTimeOverride should be used instead of PR_Now() when set. But, I think you know what I mean.

Potentially, we will be able to use this same pref for testing certificate expiration stuff and other things in the future.
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-12 19:06:27 PDT
One more thing: Currently, we require that the website send us a max-age=18+weeks, so that we can run our scanner at any time during the release cycle. But, this 18+weeks is effective as of the release of Firefox, which is different. Seems a little weird, and this discrepancy should be accounted for in some way. Let me know what you think we should do.
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-15 16:21:43 PDT
Still waiting for an answer to Alex's question in comment #2 "What's the user impact if you all forget to fix this in time for FF17?"
Comment 9 David Keeler [:keeler] (use needinfo?) 2012-10-17 14:46:19 PDT
Brian may have a different answer, but my interpretation of the user impact if this doesn't get fixed is if a user gets stuck on firefox 17 and one of the sites on the preload list decides to stop using ssl/tls (maybe the domain gets sold, maybe they decide it's too expensive, etc.), the user will not be able to connect to that site.
Comment 10 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-17 15:08:16 PDT
(In reply to David Keeler from comment #9)
> Brian may have a different answer, but my interpretation of the user impact
> if this doesn't get fixed is if a user gets stuck on firefox 17 and one of
> the sites on the preload list decides to stop using ssl/tls (maybe the
> domain gets sold, maybe they decide it's too expensive, etc.), the user will
> not be able to connect to that site.

Yes, this is right. Note that this is a problem with any HSTS site that sets a long max-age.

Note that this is a safeguard that some important sites have specifically asked us to implement, so I think it is important to have it implemented in the first release for which we enable this feature.
Comment 11 Alex Keybl [:akeybl] 2012-10-17 15:44:04 PDT
(In reply to Brian Smith (:bsmith) from comment #10)
> (In reply to David Keeler from comment #9)
> > Brian may have a different answer, but my interpretation of the user impact
> > if this doesn't get fixed is if a user gets stuck on firefox 17 and one of
> > the sites on the preload list decides to stop using ssl/tls (maybe the
> > domain gets sold, maybe they decide it's too expensive, etc.), the user will
> > not be able to connect to that site.
> 
> Yes, this is right. Note that this is a problem with any HSTS site that sets
> a long max-age.
> 
> Note that this is a safeguard that some important sites have specifically
> asked us to implement, so I think it is important to have it implemented in
> the first release for which we enable this feature.

OK - we'll track for 17/18. What's the risk of regression here? If it's anything but low, what are our options for disabling HSTS support in FF17?
Comment 12 David Keeler [:keeler] (use needinfo?) 2012-10-18 11:04:54 PDT
Created attachment 672860 [details] [diff] [review]
patch v2

I think I've re-worked this according to how you think it should go.
With regard to this 18 weeks not being the same as the minimum 18 weeks to get on the list, I think I understand your point (i.e. we could do the scan early in a release cycle, and by the time we actually release, the list is many weeks stale, so sites on the list are really only guaranteeing something like 12 or 15 weeks of HSTS support). To deal with this, I suppose we could make the longest time without updates be 12 weeks or something.
Comment 13 David Keeler [:keeler] (use needinfo?) 2012-10-18 11:09:49 PDT
(In reply to Alex Keybl [:akeybl] from comment #11)
> OK - we'll track for 17/18. What's the risk of regression here? If it's
> anything but low, what are our options for disabling HSTS support in FF17?

I think the risk is low. We'll land the fix on central, let it bake a bit to be sure, and then uplift. There really aren't any complicated changes being made.

If we really had to, we could easily remove the HSTS preload list from 17 (we wouldn't have to remove HSTS entirely).
Comment 14 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-18 11:54:07 PDT
This, or disabling the HSTS preload feature, is a must-have for B2G, because it is much more likely that B2G will go longer periods without updates.
Comment 15 Andrew Overholt [:overholt] 2012-10-19 10:37:13 PDT
Based on comment #14, marking as a blocker.

What's HSTS?
Comment 16 David Keeler [:keeler] (use needinfo?) 2012-10-19 11:15:25 PDT
HSTS stands for HTTP Strict Transport Security: https://en.wikipedia.org/wiki/HTTP_Strict_Transport_Security
Comment 17 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-23 12:11:07 PDT
Comment on attachment 672860 [details] [diff] [review]
patch v2

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +113,5 @@
> +   rv = GetBuildTime(&buildTime);
> +   // GetBuildTime only fails if parsing the time fails, which can only
> +   // happen if the buildID is not in a format we expect. What should be done?
> +   NS_ENSURE_SUCCESS(rv, rv);
> +   mPreloadListExpirationTime = buildTime + ((18*7*24*60*60) * PR_USEC_PER_SEC);

My understanding is that dkeeler is going to redo the patch to calculate PreloadListExpirationTime in the script and then generate its declaration in the code generator part.

@@ +397,5 @@
>  nsStrictTransportSecurityService::GetPreloadListEntry(const char *aHost)
>  {
> +  PRTime currentTime = PR_Now();
> +  int32_t overrideTime = 0;
> +  nsresult status = mozilla::Preferences::GetInt("test.currentTimeOverride",

It looks like this isn't really an override of the current time, but rather an offset to add to the current time. So, the name should include the word "Offset".
Comment 18 David Keeler [:keeler] (use needinfo?) 2012-10-23 14:41:58 PDT
Created attachment 674390 [details] [diff] [review]
patch v3

This is definitely a better solution.
Also, changed the test pref name to make more sense.
Comment 19 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-23 14:57:04 PDT
Comment on attachment 674390 [details] [diff] [review]
patch v3

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

::: security/manager/boot/src/nsSTSPreloadList.errors
@@ +49,5 @@
>  ottospora.nl: could not connect to host
>  packagist.org: max-age too low: 2592000
>  plus.google.com: did not receive HSTS header
>  profiles.google.com: did not receive HSTS header
> +rhcloud.com: could not connect to host

Does the script try to retry in the case the connection fails? We should be careful about dropping entries because of transient network failures.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +375,5 @@
>  nsStrictTransportSecurityService::GetPreloadListEntry(const char *aHost)
>  {
> +  PRTime currentTime = PR_Now();
> +  int32_t timeOffset = 0;
> +  nsresult status = mozilla::Preferences::GetInt("test.currentTimeOffset",

Name nsresults "rv," or "nrv" when they are used in a function that also deals with PRStatus and/or SECStatus.

@@ +378,5 @@
> +  int32_t timeOffset = 0;
> +  nsresult status = mozilla::Preferences::GetInt("test.currentTimeOffset",
> +                                                 &timeOffset);
> +  if (NS_SUCCEEDED(status)) {
> +    currentTime += (timeOffset * PR_USEC_PER_SEC);

PRTime(timeOffset) + PR_USEC_PER_SEC would be clearer, because then we don't need to worry about the order/precedence of type promotion.

::: security/manager/tools/getHSTSPreloadList.js
@@ +180,5 @@
> +  var nowMillis = now.getTime();
> +  // MINIMUM_REQUIRED_MAX_AGE is in seconds, so convert to milliseconds
> +  var expirationMillis = nowMillis + (MINIMUM_REQUIRED_MAX_AGE * 1000);
> +  var expirationMicros = expirationMillis * 1000;
> +  return "const PRTime gPreloadListExpirationTime = " + expirationMicros + ";\n";

return "const PRTime gPreloadListExpirationTime = PR_INT64(" + expirationMicros + ");\n";

PR_INT64(x) adds the appropriate suffix (e.g. "ll") to the constant.
Comment 20 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-23 15:03:24 PDT
(In reply to Brian Smith (:bsmith) from comment #19)
> PR_INT64(x) adds the appropriate suffix (e.g. "ll") to the constant.

Sorry, I meant that you should use INTN_C( value).
Comment 21 Alex Keybl [:akeybl] 2012-10-24 09:15:11 PDT
(In reply to David Keeler from comment #13)
> (In reply to Alex Keybl [:akeybl] from comment #11)
> > OK - we'll track for 17/18. What's the risk of regression here? If it's
> > anything but low, what are our options for disabling HSTS support in FF17?
> 
> I think the risk is low. We'll land the fix on central, let it bake a bit to
> be sure, and then uplift. There really aren't any complicated changes being
> made.

OK - let's land asap and then make sure to nominate for uplift and land before Tuesday's b4 build.
Comment 22 David Keeler [:keeler] (use needinfo?) 2012-10-24 16:03:05 PDT
Created attachment 674882 [details] [diff] [review]
patch v4

(In reply to Brian Smith (:bsmith) from comment #19)
> Does the script try to retry in the case the connection fails? We should be
> careful about dropping entries because of transient network failures.

The script doesn't really retry. What I did with this change is manually visited the site to see if it sends the header (it doesn't).

(In reply to Brian Smith (:bsmith) from comment #20)
> (In reply to Brian Smith (:bsmith) from comment #19)
> > PR_INT64(x) adds the appropriate suffix (e.g. "ll") to the constant.
> 
> Sorry, I meant that you should use INTN_C( value).

I had to change the Makefile to get -D__STDC_CONSTANT_MACROS in there for this to work - I assume that's okay.
Comment 23 David Keeler [:keeler] (use needinfo?) 2012-10-24 16:05:00 PDT
(In reply to David Keeler from comment #22)
> Created attachment 674882 [details] [diff] [review]
> patch v4
> 
> (In reply to Brian Smith (:bsmith) from comment #19)
> > Does the script try to retry in the case the connection fails? We should be
> > careful about dropping entries because of transient network failures.
> 
> The script doesn't really retry. What I did with this change is manually
> visited the site to see if it sends the header (it doesn't).

Er, I meant in the case that it looks like we're removing something from nsSTSPreloadList.inc that was previously on it. If it's in nsSTSPreloadList.errors and never was on the include list, I think it's okay.
Comment 24 David Keeler [:keeler] (use needinfo?) 2012-10-24 16:27:27 PDT
Created attachment 674891 [details] [diff] [review]
patch v5

Sorry for the bugspam. Brian pointed out I should use mozilla/StandardInteger.h instead of stdint.h.
Comment 25 Honza Bambas (:mayhemer) 2012-10-26 09:30:54 PDT
Comment on attachment 674891 [details] [diff] [review]
patch v5

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

r=honzab

Time logic is correct, if I see correctly.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +375,5 @@
>  nsStrictTransportSecurityService::GetPreloadListEntry(const char *aHost)
>  {
> +  PRTime currentTime = PR_Now();
> +  int32_t timeOffset = 0;
> +  nsresult nrv = mozilla::Preferences::GetInt("test.currentTimeOffset",

Why nrv and not just rv?

Please add to the name of the pref the expected units (seconds).

@@ +389,5 @@
>                                            sizeof(nsSTSPreload),
>                                            STSPreloadCompare);
>    }
>    else {
>      return nullptr;

Could you please also remove the "else" here?  It is not necessary when if-block returns.
Comment 26 David Keeler [:keeler] (use needinfo?) 2012-10-26 09:57:18 PDT
Created attachment 675604 [details] [diff] [review]
patch v6

I had the impression that nrv was used to indicate that it wasn't going to be the return value of the current function, but it doesn't really look like that's the case.

Anyway, I addressed that and the other comments. Thanks for the review.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=340e2b4c48ce
Comment 27 David Keeler [:keeler] (use needinfo?) 2012-10-26 10:02:47 PDT
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/78cf3c6d8fc5
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-10-26 19:11:02 PDT
https://hg.mozilla.org/mozilla-central/rev/78cf3c6d8fc5
Comment 29 Ryan VanderMeulen [:RyanVM] 2012-10-27 18:48:30 PDT
This is marked blocking-basecamp+, but does not apply to Aurora. Please post a branch-specific patch if it's intended to land there.
Comment 30 David Keeler [:keeler] (use needinfo?) 2012-10-29 10:37:36 PDT
Created attachment 676221 [details] [diff] [review]
patch for uplift

[Approval Request Comment]
Bug caused by (feature/regressing bug #): HSTS preload list
User impact if declined: see comments 9 and 10 in this bug
Testing completed (on m-c, etc.): ran through try, has been on m-c for a few days
Risk to taking this patch (and alternatives if risky): low (worst that could happen is the preload list doesn't work, which is exactly the same as the alternative to taking this patch, which is removing the preload list)
String or UUID changes made by this patch: none
Comment 32 Matt Brubeck (:mbrubeck) 2013-03-01 14:26:28 PST
18 weeks is an unfortunate expiration time unless we have much more frequent updates, since it takes up to 18 weeks for changes from mozilla-central to propagate to the release channel (or 19-20 weeks in cases where we extend cycles for holidays and/or throttle auto-updates for various reasons).  So instead of disabling the list for users on old releases, this is currently disabling the list for users on the current release.

See bug 836097 for more discussion.

Note You need to log in before you can comment on or make changes to this bug.