Properly implement various HSTS states.

Previously, HSTS preload list values could be overridden temporarily due
to counter-intuitive behavior of the API's removeState function.
This adds an explicit flag to the API for writing knockout values to
the Site Security Service, with the default resetting to whatever the
preload list state is.

2 files changed, 27 insertions, 16 deletions

diff --git a/security/manager/ssl/nsISiteSecurityService.idl b/security/manager/ssl/nsISiteSecurityService.idl
index 753f32b57..b61577152 100644
--- a/security/manager/ssl/nsISiteSecurityService.idl
+++ b/security/manager/ssl/nsISiteSecurityService.idl
@@ -23,7 +23,7 @@ namespace mozilla
 [ref] native nsCStringTArrayRef(nsTArray<nsCString>);
 [ref] native mozillaPkixTime(mozilla::pkix::Time);
 
-[scriptable, uuid(275127f8-dbd7-4681-afbf-6df0c6587a01)]
+[scriptable, uuid(233908bd-6741-4474-a6e1-f298c6ce9eaf)]
 interface nsISiteSecurityService : nsISupports
 {
     const uint32_t HEADER_HSTS = 0;
@@ -98,15 +98,21 @@ interface nsISiteSecurityService : nsISupports
      * Given a header type, removes state relating to that header of a host,
      * including the includeSubdomains state that would affect subdomains.
      * This essentially removes the state for the domain tree rooted at this
-     * host.
+     * host. If any preloaded information is present for that host, that
+     * information will then be used instead of any other previously existing
+     * state, unless the force parameter is set.
+     *
      * @param aType   the type of security state in question
      * @param aURI    the URI of the target host
      * @param aFlags  options for this request as defined in nsISocketProvider:
      *                  NO_PERMANENT_STORAGE
+     * @param force   if set, forces no-HSTS state by writing a knockout value,
+     *                overriding any preload list state
      */
     void removeState(in uint32_t aType,
                      in nsIURI aURI,
-                     in uint32_t aFlags);
+                     in uint32_t aFlags,
+                     [optional] in boolean force);
 
     /**
      * See isSecureURI
diff --git a/security/manager/ssl/nsSiteSecurityService.cpp b/security/manager/ssl/nsSiteSecurityService.cpp
index cfee79d8d..44ee7dcc0 100644
--- a/security/manager/ssl/nsSiteSecurityService.cpp
+++ b/security/manager/ssl/nsSiteSecurityService.cpp
@@ -330,21 +330,22 @@ nsSiteSecurityService::SetHSTSState(uint32_t aType,
                                     uint32_t flags,
                                     SecurityPropertyState aHSTSState)
 {
-  // If max-age is zero, that's an indication to immediately remove the
-  // security state, so here's a shortcut.
-  if (!maxage) {
-    return RemoveState(aType, aSourceURI, flags);
+  // Exit early if STS not enabled
+  if (!mUseStsService) {
+    return NS_OK;
+  }
+
+  // If max-age is zero, the host is no longer considered HSTS. If the host was
+  // preloaded, we store an entry indicating that this host is not HSTS, causing
+  // the preloaded information to be ignored.
+  if (maxage == 0) {
+    return RemoveState(aType, aSourceURI, flags, true);
   }
 
   MOZ_ASSERT((aHSTSState == SecurityPropertySet ||
               aHSTSState == SecurityPropertyNegative),
       "HSTS State must be SecurityPropertySet or SecurityPropertyNegative");
 
-  // Exit early if STS not enabled
-  if (!mUseStsService) {
-    return NS_OK;
-  }
-
   int64_t expiretime = ExpireTimeFromMaxAge(maxage);
   SiteHSTSState siteState(expiretime, aHSTSState, includeSubdomains);
   nsAutoCString stateString;
@@ -367,7 +368,7 @@ nsSiteSecurityService::SetHSTSState(uint32_t aType,
 
 NS_IMETHODIMP
 nsSiteSecurityService::RemoveState(uint32_t aType, nsIURI* aURI,
-                                   uint32_t aFlags)
+                                   uint32_t aFlags, bool force = false)
 {
    // Child processes are not allowed direct access to this.
    if (!XRE_IsParentProcess()) {
@@ -387,8 +388,9 @@ nsSiteSecurityService::RemoveState(uint32_t aType, nsIURI* aURI,
   mozilla::DataStorageType storageType = isPrivate
                                          ? mozilla::DataStorage_Private
                                          : mozilla::DataStorage_Persistent;
-  // If this host is in the preload list, we have to store a knockout entry.
-  if (GetPreloadListEntry(hostname.get())) {
+  // If this host is in the preload list, we have to store a knockout entry
+  // if it's explicitly forced to not be HSTS anymore
+  if (force && GetPreloadListEntry(hostname.get())) {
     SSSLOG(("SSS: storing knockout entry for %s", hostname.get()));
     SiteHSTSState siteState(0, SecurityPropertyKnockout, false);
     nsAutoCString stateString;
@@ -769,7 +771,10 @@ nsSiteSecurityService::ProcessPKPHeader(nsIURI* aSourceURI,
     return NS_ERROR_FAILURE;
   }
 
-  // if maxAge == 0 we must delete all state, for now no hole-punching
+  // If maxAge == 0, we remove dynamic HPKP state for this host. Due to
+  // architectural constraints, if this host was preloaded, any future lookups
+  // will use the preloaded state (i.e. we can't store a "this host is not HPKP"
+  // entry like we can for HSTS).
   if (maxAge == 0) {
     return RemoveState(aType, aSourceURI, aFlags);
   }