As recommended by the spec: https://tools.ietf.org/html/rfc7469#section-4.1

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

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP Keeler: Does this look like the right approach? Suggestions for how to improve the testing?

https://reviewboard.mozilla.org/r/62748/#review59582 This seems like the right approach. 60 days seems a little short to me, though (perhaps we should add telemetry for how often sites are trying to set longer pins). ::: security/manager/ssl/nsSiteSecurityService.h:148 (Diff revision 1) > uint32_t* aFailureResult); > nsresult SetHPKPState(const char* aHost, SiteHPKPState& entry, uint32_t flags); > > const nsSTSPreload *GetPreloadListEntry(const char *aHost); > > + int64_t mMaxMaxAge; This should probably be unsigned. ::: security/manager/ssl/nsSiteSecurityService.cpp:207 (Diff revision 1) > } > } > > //////////////////////////////////////////////////////////////////////////////// > > +const int64_t kSixtyDaysInSeconds = 60 * 24 * 60 * 60; Same here. ::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js:127 (Diff revision 1) > checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN2 + INCLUDE_SUBDOMAINS); > checkPassSettingPin(VALID_PIN1 + GOOD_MAX_AGE + BACKUP_PIN2 + INCLUDE_SUBDOMAINS); > checkPassSettingPin(VALID_PIN1 + GOOD_MAX_AGE + BACKUP_PIN2 + REPORT_URI + INCLUDE_SUBDOMAINS); > checkPassSettingPin(INCLUDE_SUBDOMAINS + VALID_PIN1 + GOOD_MAX_AGE + BACKUP_PIN2); > checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN1 + UNRECOGNIZED_DIRECTIVE); > + checkPassSettingPin(LONG_MAX_AGE + VALID_PIN1 + BACKUP_PIN1); Since the parsed maxAge is an optional out parameter for processHeader, we could pass the expected clamped maxAge to checkPassSettingPin or something.

> pref("security.cert_pinning.max_max_age_seconds", 60 * 24 * 60 * 60); Using multiplication in a pref isn't going to work. It /looks/ like JavaScript, originally it /was/ parsed and executed as JavaScript (in Netscape days), but no longer: https://dxr.mozilla.org/mozilla-central/source/modules/libpref/prefread.cpp#192 Re: keeler's review comment about 60 days seeming short. Yes, but 1) it's recommended in the spec, and 2) it's what Chrome is doing (see bug 1279662). At this point 60 days is the default expectation and we need to justify any other value.

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62748/diff/1-2/

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP https://reviewboard.mozilla.org/r/62748/#review59764 Cool - r=me. ::: security/manager/ssl/nsSiteSecurityService.cpp:211 (Diff revision 2) > > +const uint64_t kSixtyDaysInSeconds = 60 * 24 * 60 * 60; > + > nsSiteSecurityService::nsSiteSecurityService() > : mUsePreloadList(true) > , mPreloadListTimeOffset(0) I guess we should also initialize mMaxMaxAge here. ::: security/manager/ssl/nsSiteSecurityService.cpp:234 (Diff revision 2) > return NS_ERROR_NOT_SAME_THREAD; > } > > mUsePreloadList = mozilla::Preferences::GetBool( > "network.stricttransportsecurity.preloadlist", true); > + mMaxMaxAge = mozilla::Preferences::GetInt( nit: it looks like the style here is to set the member variable and then add the observer just after (so this bit shouldn't come between mUsePreloadList and its observer, and the observer for this should be just after it)

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62748/diff/2-3/

https://reviewboard.mozilla.org/r/62748/#review59778 Ok - making that type change was less disruptive than I thought. ExpireTimeFromMaxAge needs to be updated as well, though. ::: security/manager/ssl/nsSiteSecurityService.cpp:605 (Diff revisions 2 - 3) > - if (PR_sscanf(directive->mValue.get(), "%lld", &maxAge) != 1) { > + if (PR_sscanf(directive->mValue.get(), "%llu", &maxAge) != 1) { > SSSLOG(("SSS: could not parse delta-seconds")); > return nsISiteSecurityService::ERROR_INVALID_MAX_AGE; > } > > SSSLOG(("SSS: parsed delta-seconds: %lld", maxAge)); Presumably this should be %llu as well. ::: security/manager/ssl/nsSiteSecurityService.cpp:797 (Diff revisions 2 - 3) > *aFailureResult = nsISiteSecurityService::ERROR_NO_BACKUP_PIN; > } > return NS_ERROR_FAILURE; > } > > int64_t expireTime = ExpireTimeFromMaxAge(maxAge); Looks like this needs to be updated as well (relatedly, there's probably an overflow concern in ExpireTimeFromMaxAge...) ::: security/manager/ssl/nsSiteSecurityService.cpp:800 (Diff revisions 2 - 3) > } > > int64_t expireTime = ExpireTimeFromMaxAge(maxAge); > SiteHPKPState dynamicEntry(expireTime, SecurityPropertySet, > foundIncludeSubdomains, sha256keys); > SSSLOG(("SSS: about to set pins for %s, expires=%ld now=%ld maxAge=%ld\n", I guess the maxAge format should be %llu here as well.

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62748/diff/3-4/

Pushed by rlb@ipv.sx: https://hg.mozilla.org/integration/autoland/rev/d75db8a749e7 Enforce a maximum max-age for HPKP r=keeler

Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4a6d64033813 Followup to fix eslint a=me

https://hg.mozilla.org/mozilla-central/rev/d75db8a749e7 https://hg.mozilla.org/mozilla-central/rev/4a6d64033813

I'd like to see this in 49 as well. Can we get a Beta branch patch made and nominated?

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP Approval Request Comment [Feature/regressing bug #]: Greater safety for HPKP [User impact if declined]: Risk that a site can DOS itself with a long-lived bad HPKP header [Describe test coverage new/current, TreeHerder]: Inbound testing [Risks and why]: Minimal risk, just imposing a policy on a fairly niche use case [String/UUID change made/needed]: None

Patch for beta

Comment on attachment 8768590 [details] Bug 1285052 - Enforce a maximum max-age for HPKP This patch fixes a HPKP issue to be more secure. Take it in 49 beta.

And a delayed followup on beta to fix up an eslint failure: https://hg.mozilla.org/releases/mozilla-beta/rev/77414dfaa7d6