Bug 1285052 - Enforce a maximum max-age for HPKP r=keeler draft
authorRichard Barnes <rbarnes@mozilla.com>
Wed, 06 Jul 2016 19:16:29 -0400
changeset 385147 127c9dd479b6a48e72da378a4df357a1bba1e6f3
parent 384700 6de59178cfa0dd306c936cd0e928f9584b47c760
child 524854 716dd4f727b30ccf3f4b7d2defece1c20d9bf30c
push id22430
push userrlb@ipv.sx
push dateThu, 07 Jul 2016 18:05:48 +0000
reviewerskeeler
bugs1285052
milestone50.0a1
Bug 1285052 - Enforce a maximum max-age for HPKP r=keeler MozReview-Commit-ID: 1LD02GkqzTe
netwerk/base/security-prefs.js
security/manager/ssl/nsSiteSecurityService.cpp
security/manager/ssl/nsSiteSecurityService.h
security/manager/ssl/tests/unit/test_pinning_header_parsing.js
--- a/netwerk/base/security-prefs.js
+++ b/netwerk/base/security-prefs.js
@@ -87,8 +87,13 @@ pref("security.pki.netscape_step_up_poli
 
 pref("security.webauth.u2f", false);
 pref("security.webauth.u2f_enable_softtoken", false);
 pref("security.webauth.u2f_enable_usbtoken", false);
 
 pref("security.ssl.errorReporting.enabled", true);
 pref("security.ssl.errorReporting.url", "https://incoming.telemetry.mozilla.org/submit/sslreports/");
 pref("security.ssl.errorReporting.automatic", false);
+
+// Impose a maximum age on HPKP headers, to avoid sites getting permanently
+// blacking themselves out by setting a bad pin.  (60 days by default)
+// https://tools.ietf.org/html/rfc7469#section-4.1
+pref("security.cert_pinning.max_max_age_seconds", 5184000);
--- a/security/manager/ssl/nsSiteSecurityService.cpp
+++ b/security/manager/ssl/nsSiteSecurityService.cpp
@@ -199,18 +199,21 @@ SiteHPKPState::ToString(nsCString& aStri
   aString.Append(',');
   for (unsigned int i = 0; i < mSHA256keys.Length(); i++) {
     aString.Append(mSHA256keys[i]);
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
+const uint64_t kSixtyDaysInSeconds = 60 * 24 * 60 * 60;
+
 nsSiteSecurityService::nsSiteSecurityService()
-  : mUsePreloadList(true)
+  : mMaxMaxAge(kSixtyDaysInSeconds)
+  , mUsePreloadList(true)
   , mPreloadListTimeOffset(0)
 {
 }
 
 nsSiteSecurityService::~nsSiteSecurityService()
 {
 }
 
@@ -222,16 +225,20 @@ nsresult
 nsSiteSecurityService::Init()
 {
   // Don't access Preferences off the main thread.
   if (!NS_IsMainThread()) {
     NS_NOTREACHED("nsSiteSecurityService initialized off main thread");
     return NS_ERROR_NOT_SAME_THREAD;
   }
 
+  mMaxMaxAge = mozilla::Preferences::GetInt(
+    "security.cert_pinning.max_max_age_seconds", kSixtyDaysInSeconds);
+  mozilla::Preferences::AddStrongObserver(this,
+    "security.cert_pinning.max_max_age_seconds");
   mUsePreloadList = mozilla::Preferences::GetBool(
     "network.stricttransportsecurity.preloadlist", true);
   mozilla::Preferences::AddStrongObserver(this,
     "network.stricttransportsecurity.preloadlist");
   mProcessPKPHeadersFromNonBuiltInRoots = mozilla::Preferences::GetBool(
     "security.cert_pinning.process_headers_from_non_builtin_roots", false);
   mozilla::Preferences::AddStrongObserver(this,
     "security.cert_pinning.process_headers_from_non_builtin_roots");
@@ -292,19 +299,19 @@ SetStorageKey(nsAutoCString& storageKey,
     default:
       NS_ASSERTION(false, "SSS:SetStorageKey got invalid type");
   }
 }
 
 // Expire times are in millis.  Since Headers max-age is in seconds, and
 // PR_Now() is in micros, normalize the units at milliseconds.
 static int64_t
-ExpireTimeFromMaxAge(int64_t maxAge)
+ExpireTimeFromMaxAge(uint64_t maxAge)
 {
-  return (PR_Now() / PR_USEC_PER_MSEC) + (maxAge * PR_MSEC_PER_SEC);
+  return (PR_Now() / PR_USEC_PER_MSEC) + ((int64_t)maxAge * PR_MSEC_PER_SEC);
 }
 
 nsresult
 nsSiteSecurityService::SetHSTSState(uint32_t aType,
                                     nsIURI* aSourceURI,
                                     int64_t maxage,
                                     bool includeSubdomains,
                                     uint32_t flags)
@@ -503,17 +510,17 @@ nsSiteSecurityService::ProcessHeaderInte
 }
 
 static uint32_t
 ParseSSSHeaders(uint32_t aType,
                 const char* aHeader,
                 bool& foundIncludeSubdomains,
                 bool& foundMaxAge,
                 bool& foundUnrecognizedDirective,
-                int64_t& maxAge,
+                uint64_t& maxAge,
                 nsTArray<nsCString>& sha256keys)
 {
   // Strict transport security and Public Key Pinning have very similar
   // Header formats.
 
   // "Strict-Transport-Security" ":" OWS
   //      STS-d  *( OWS ";" OWS STS-d  OWS)
   //
@@ -585,22 +592,22 @@ ParseSSSHeaders(uint32_t aType,
       for (size_t i = 0; i < len; i++) {
         char chr = directive->mValue.CharAt(i);
         if (chr < '0' || chr > '9') {
           SSSLOG(("SSS: invalid value for max-age directive"));
           return nsISiteSecurityService::ERROR_INVALID_MAX_AGE;
         }
       }
 
-      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));
+      SSSLOG(("SSS: parsed delta-seconds: %llu", maxAge));
     } else if (directive->mName.Length() == include_subd_var.Length() &&
                directive->mName.EqualsIgnoreCase(include_subd_var.get(),
                                                  include_subd_var.Length())) {
       if (foundIncludeSubdomains) {
         SSSLOG(("SSS: found two includeSubdomains directives"));
         return nsISiteSecurityService::ERROR_MULTIPLE_INCLUDE_SUBDOMAINS;
       }
 
@@ -657,17 +664,17 @@ nsSiteSecurityService::ProcessPKPHeader(
   }
   SSSLOG(("SSS: processing HPKP header '%s'", aHeader));
   NS_ENSURE_ARG(aSSLStatus);
 
   const uint32_t aType = nsISiteSecurityService::HEADER_HPKP;
   bool foundMaxAge = false;
   bool foundIncludeSubdomains = false;
   bool foundUnrecognizedDirective = false;
-  int64_t maxAge = 0;
+  uint64_t maxAge = 0;
   nsTArray<nsCString> sha256keys;
   uint32_t sssrv = ParseSSSHeaders(aType, aHeader, foundIncludeSubdomains,
                                    foundMaxAge, foundUnrecognizedDirective,
                                    maxAge, sha256keys);
   if (sssrv != nsISiteSecurityService::Success) {
     if (aFailureResult) {
       *aFailureResult = sssrv;
     }
@@ -737,16 +744,21 @@ nsSiteSecurityService::ProcessPKPHeader(
     return NS_ERROR_FAILURE;
   }
 
   // if maxAge == 0 we must delete all state, for now no hole-punching
   if (maxAge == 0) {
     return RemoveState(aType, aSourceURI, aFlags);
   }
 
+  // clamp maxAge to the maximum set by pref
+  if (maxAge > mMaxMaxAge) {
+    maxAge = mMaxMaxAge;
+  }
+
   bool chainMatchesPinset;
   rv = PublicKeyPinningService::ChainMatchesPinset(certList, sha256keys,
                                                    chainMatchesPinset);
   if (NS_FAILED(rv)) {
     return rv;
   }
   if (!chainMatchesPinset) {
     // is invalid
@@ -780,30 +792,30 @@ nsSiteSecurityService::ProcessPKPHeader(
       *aFailureResult = nsISiteSecurityService::ERROR_NO_BACKUP_PIN;
     }
     return NS_ERROR_FAILURE;
   }
 
   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",
+  SSSLOG(("SSS: about to set pins for  %s, expires=%ld now=%ld maxAge=%lu\n",
            host.get(), expireTime, PR_Now() / PR_USEC_PER_MSEC, maxAge));
 
   rv = SetHPKPState(host.get(), dynamicEntry, aFlags);
   if (NS_FAILED(rv)) {
     SSSLOG(("SSS: failed to set pins for %s\n", host.get()));
     if (aFailureResult) {
       *aFailureResult = nsISiteSecurityService::ERROR_COULD_NOT_SAVE_STATE;
     }
     return rv;
   }
 
   if (aMaxAge != nullptr) {
-    *aMaxAge = (uint64_t)maxAge;
+    *aMaxAge = maxAge;
   }
 
   if (aIncludeSubdomains != nullptr) {
     *aIncludeSubdomains = foundIncludeSubdomains;
   }
 
   return foundUnrecognizedDirective
            ? NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA
@@ -822,17 +834,17 @@ nsSiteSecurityService::ProcessSTSHeader(
     *aFailureResult = nsISiteSecurityService::ERROR_UNKNOWN;
   }
   SSSLOG(("SSS: processing HSTS header '%s'", aHeader));
 
   const uint32_t aType = nsISiteSecurityService::HEADER_HSTS;
   bool foundMaxAge = false;
   bool foundIncludeSubdomains = false;
   bool foundUnrecognizedDirective = false;
-  int64_t maxAge = 0;
+  uint64_t maxAge = 0;
   nsTArray<nsCString> unusedSHA256keys; // Required for sane internal interface
 
   uint32_t sssrv = ParseSSSHeaders(aType, aHeader, foundIncludeSubdomains,
                                    foundMaxAge, foundUnrecognizedDirective,
                                    maxAge, unusedSHA256keys);
   if (sssrv != nsISiteSecurityService::Success) {
     if (aFailureResult) {
       *aFailureResult = sssrv;
@@ -857,17 +869,17 @@ nsSiteSecurityService::ProcessSTSHeader(
     SSSLOG(("SSS: failed to set STS state"));
     if (aFailureResult) {
       *aFailureResult = nsISiteSecurityService::ERROR_COULD_NOT_SAVE_STATE;
     }
     return rv;
   }
 
   if (aMaxAge != nullptr) {
-    *aMaxAge = (uint64_t)maxAge;
+    *aMaxAge = maxAge;
   }
 
   if (aIncludeSubdomains != nullptr) {
     *aIncludeSubdomains = foundIncludeSubdomains;
   }
 
   return foundUnrecognizedDirective
            ? NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA
@@ -1197,12 +1209,14 @@ nsSiteSecurityService::Observe(nsISuppor
 
   if (strcmp(topic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0) {
     mUsePreloadList = mozilla::Preferences::GetBool(
       "network.stricttransportsecurity.preloadlist", true);
     mPreloadListTimeOffset =
       mozilla::Preferences::GetInt("test.currentTimeOffsetSeconds", 0);
     mProcessPKPHeadersFromNonBuiltInRoots = mozilla::Preferences::GetBool(
       "security.cert_pinning.process_headers_from_non_builtin_roots", false);
+    mMaxMaxAge = mozilla::Preferences::GetInt(
+      "security.cert_pinning.max_max_age_seconds", kSixtyDaysInSeconds);
   }
 
   return NS_OK;
 }
--- a/security/manager/ssl/nsSiteSecurityService.h
+++ b/security/manager/ssl/nsSiteSecurityService.h
@@ -140,15 +140,16 @@ private:
   nsresult ProcessPKPHeader(nsIURI* aSourceURI, const char* aHeader,
                             nsISSLStatus* aSSLStatus, uint32_t flags,
                             uint64_t* aMaxAge, bool* aIncludeSubdomains,
                             uint32_t* aFailureResult);
   nsresult SetHPKPState(const char* aHost, SiteHPKPState& entry, uint32_t flags);
 
   const nsSTSPreload *GetPreloadListEntry(const char *aHost);
 
+  uint64_t mMaxMaxAge;
   bool mUsePreloadList;
   int64_t mPreloadListTimeOffset;
   bool mProcessPKPHeadersFromNonBuiltInRoots;
   RefPtr<mozilla::DataStorage> mSiteStateStorage;
 };
 
 #endif // __nsSiteSecurityService_h__
--- a/security/manager/ssl/tests/unit/test_pinning_header_parsing.js
+++ b/security/manager/ssl/tests/unit/test_pinning_header_parsing.js
@@ -27,72 +27,84 @@ function checkFailParseInvalidPin(pinVal
                         certFromFile('a.pinning2.example.com-pinningroot'));
   let uri = Services.io.newURI("https://a.pinning2.example.com", null, null);
   throws(() => {
     gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri,
                              pinValue, sslStatus, 0);
   }, /NS_ERROR_FAILURE/, `Invalid pin "${pinValue}" should be rejected`);
 }
 
-function checkPassValidPin(pinValue, settingPin) {
+function checkPassValidPin(pinValue, settingPin, expectedMaxAge) {
   let sslStatus = new FakeSSLStatus(
                         certFromFile('a.pinning2.example.com-pinningroot'));
   let uri = Services.io.newURI("https://a.pinning2.example.com", null, null);
+  let maxAge = {};
 
   // setup preconditions for the test, if setting ensure there is no previous
   // state, if removing ensure there is a valid pin in place.
   if (settingPin) {
     gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0);
   } else {
     // add a known valid pin!
     let validPinValue = "max-age=5000;" + VALID_PIN1 + BACKUP_PIN1;
     gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri,
                              validPinValue, sslStatus, 0);
   }
   try {
     gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri,
-                             pinValue, sslStatus, 0);
+                             pinValue, sslStatus, 0, maxAge);
     ok(true, "Valid pin should be accepted");
   } catch (e) {
     ok(false, "Valid pin should have been accepted");
   }
+
+  // check that maxAge was processed correctly
+  if (settingPin && expectedMaxAge) {
+    ok(maxAge.value == expectedMaxAge, `max-age value should be ${expectedMaxAge}`)
+  }
+
   // after processing ensure that the postconditions are true, if setting
   // the host must be pinned, if removing the host must not be pinned
   let hostIsPinned = gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP,
                                              "a.pinning2.example.com", 0);
   if (settingPin) {
     ok(hostIsPinned, "Host should be considered pinned");
   } else {
     ok(!hostIsPinned, "Host should not be considered pinned");
   }
 }
 
-function checkPassSettingPin(pinValue) {
-  return checkPassValidPin(pinValue, true);
+function checkPassSettingPin(pinValue, expectedMaxAge) {
+  return checkPassValidPin(pinValue, true, expectedMaxAge);
 }
 
 function checkPassRemovingPin(pinValue) {
   return checkPassValidPin(pinValue, false);
 }
 
+const MAX_MAX_AGE_SECONDS = 100000;
+const GOOD_MAX_AGE_SECONDS = 69403;
+const LONG_MAX_AGE_SECONDS = 2 * MAX_MAX_AGE_SECONDS;
 const NON_ISSUED_KEY_HASH1 = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
 const NON_ISSUED_KEY_HASH2 = "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ=";
 const PINNING_ROOT_KEY_HASH = "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=";
 const MAX_AGE_ZERO = "max-age=0;";
 const VALID_PIN1 = `pin-sha256="${PINNING_ROOT_KEY_HASH}";`;
 const BACKUP_PIN1 = `pin-sha256="${NON_ISSUED_KEY_HASH1}";`;
 const BACKUP_PIN2 = `pin-sha256="${NON_ISSUED_KEY_HASH2}";`;
 const BROKEN_PIN1 = "pin-sha256=\"jdjsjsjs\";";
-const GOOD_MAX_AGE = "max-age=69403;";
+const GOOD_MAX_AGE = `max-age=${GOOD_MAX_AGE_SECONDS};`;
+const LONG_MAX_AGE = `max-age=${LONG_MAX_AGE_SECONDS};`;
 const INCLUDE_SUBDOMAINS = "includeSubdomains;";
 const REPORT_URI = "report-uri=\"https://www.example.com/report/\";";
 const UNRECOGNIZED_DIRECTIVE = "unreconized-dir=12343;";
 
 function run_test() {
   Services.prefs.setIntPref("security.cert_pinning.enforcement_level", 2);
+  Services.prefs.setIntPref("security.cert_pinning.max_max_age_seconds", MAX_MAX_AGE_SECONDS);
   Services.prefs.setBoolPref("security.cert_pinning.process_headers_from_non_builtin_roots", true);
 
   loadCert("pinningroot", "CTu,CTu,CTu");
   loadCert("badca", "CTu,CTu,CTu");
 
   checkFailParseInvalidPin("max-age=INVALID");
   // check that incomplete headers are failure
   checkFailParseInvalidPin(GOOD_MAX_AGE);
@@ -110,16 +122,19 @@ function run_test() {
   checkFailParseInvalidPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN1 + REPORT_URI + REPORT_URI);
   checkFailParseInvalidPin("thisisinvalidtest");
   checkFailParseInvalidPin("invalid" + GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN1);
 
   checkPassRemovingPin("max-age=0"); //test removal without terminating ';'
   checkPassRemovingPin(MAX_AGE_ZERO);
   checkPassRemovingPin(MAX_AGE_ZERO + VALID_PIN1);
 
+  checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN1, GOOD_MAX_AGE_SECONDS);
+  checkPassSettingPin(LONG_MAX_AGE + VALID_PIN1 + BACKUP_PIN1, MAX_MAX_AGE_SECONDS);
+
   checkPassRemovingPin(VALID_PIN1 + MAX_AGE_ZERO + VALID_PIN1);
   checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN1);
   checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN2);
   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);