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.

Tracking 18 and 17 because we're definitely going to ask to uplift this.

(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?

This would be the version to go on trunk, not to uplift. I'll get to that as soon as I can.

Forgot to mention, this depends on the patch in bug 786417.

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.

(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.

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.

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?"

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.

(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.

(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?

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.

(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).

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.

Based on comment #14, marking as a blocker. What's HSTS?

HSTS stands for HTTP Strict Transport Security: https://en.wikipedia.org/wiki/HTTP_Strict_Transport_Security

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".

This is definitely a better solution. Also, changed the test pref name to make more sense.

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.

(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).

(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.

(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.

(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.

Sorry for the bugspam. Brian pointed out I should use mozilla/StandardInteger.h instead of stdint.h.

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.

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

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/78cf3c6d8fc5

This is marked blocking-basecamp+, but does not apply to Aurora. Please post a branch-specific patch if it's intended to land there.

[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

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/214dfb353b69 Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/931c7345d861

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.