bug 977865 - mozilla::pkix: add backoff for ocsp fetching when a responder fails r=cviecco
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 28 Apr 2014 16:38:15 -0700
changeset 181112 6d813156e49133e6416cfd16873feec564936eb2
parent 181111 2b041298914b297598fc5a421ddba8d1ceac933b
child 181113 ec6b9792a042f4124f4dd63d1c8f6ad6a32d1be1
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewerscviecco
bugs977865
milestone32.0a1
bug 977865 - mozilla::pkix: add backoff for ocsp fetching when a responder fails r=cviecco
security/certverifier/NSSCertDBTrustDomain.cpp
security/certverifier/NSSCertDBTrustDomain.h
security/certverifier/OCSPCache.cpp
security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
security/manager/ssl/tests/unit/test_ocsp_caching.js
security/manager/ssl/tests/unit/test_ocsp_required.js
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -161,17 +161,18 @@ NSSCertDBTrustDomain::CheckRevocation(
   // If we have a stapled OCSP response then the verification of that response
   // determines the result unless the OCSP response is expired. We make an
   // exception for expired responses because some servers, nginx in particular,
   // are known to serve expired responses due to bugs.
   if (stapledOCSPResponse) {
     PR_ASSERT(endEntityOrCA == MustBeEndEntity);
     SECStatus rv = VerifyAndMaybeCacheEncodedOCSPResponse(cert, issuerCert,
                                                           time,
-                                                          stapledOCSPResponse);
+                                                          stapledOCSPResponse,
+                                                          ResponseWasStapled);
     if (rv == SECSuccess) {
       // stapled OCSP response present and good
       Telemetry::Accumulate(Telemetry::SSL_OCSP_STAPLING, 1);
       PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
              ("NSSCertDBTrustDomain: stapled OCSP response: good"));
       return rv;
     }
     if (PR_GetError() != SEC_ERROR_OCSP_OLD_RESPONSE) {
@@ -218,16 +219,25 @@ NSSCertDBTrustDomain::CheckRevocation(
            ("NSSCertDBTrustDomain: cached OCSP response: error %ld valid "
            "until %lld", cachedResponseErrorCode, cachedResponseValidThrough));
     // When a good cached response has expired, it is more convenient
     // to convert that to an error code and just deal with
     // cachedResponseErrorCode from here on out.
     if (cachedResponseErrorCode == 0 && cachedResponseValidThrough < time) {
       cachedResponseErrorCode = SEC_ERROR_OCSP_OLD_RESPONSE;
     }
+    // We may have a cached indication of server failure. Ignore it if
+    // it has expired.
+    if (cachedResponseErrorCode != 0 &&
+        cachedResponseErrorCode != SEC_ERROR_OCSP_UNKNOWN_CERT &&
+        cachedResponseErrorCode != SEC_ERROR_OCSP_OLD_RESPONSE &&
+        cachedResponseValidThrough < time) {
+      cachedResponseErrorCode = 0;
+      cachedResponsePresent = false;
+    }
   } else {
     PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
            ("NSSCertDBTrustDomain: no cached OCSP response"));
   }
   // At this point, if and only if cachedErrorResponseCode is 0, there was no
   // cached response.
   PR_ASSERT((!cachedResponsePresent && cachedResponseErrorCode == 0) ||
             (cachedResponsePresent && cachedResponseErrorCode != 0));
@@ -285,25 +295,41 @@ NSSCertDBTrustDomain::CheckRevocation(
     return SECSuccess;
   }
 
   ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
   if (!arena) {
     return SECFailure;
   }
 
-  const SECItem* request(CreateEncodedOCSPRequest(arena.get(), cert,
-                                                  issuerCert));
-  if (!request) {
-    return SECFailure;
+  // Only request a response if we didn't have a cached indication of failure
+  // (don't keep requesting responses from a failing server).
+  const SECItem* response = nullptr;
+  if (cachedResponseErrorCode == 0 ||
+      cachedResponseErrorCode == SEC_ERROR_OCSP_UNKNOWN_CERT ||
+      cachedResponseErrorCode == SEC_ERROR_OCSP_OLD_RESPONSE) {
+    const SECItem* request(CreateEncodedOCSPRequest(arena.get(), cert,
+                                                    issuerCert));
+    if (!request) {
+      return SECFailure;
+    }
+
+    response = CERT_PostOCSPRequest(arena.get(), url.get(), request);
   }
 
-  const SECItem* response(CERT_PostOCSPRequest(arena.get(), url.get(),
-                                               request));
   if (!response) {
+    PRErrorCode error = PR_GetError();
+    if (error == 0) {
+      error = cachedResponseErrorCode;
+    }
+    PRTime timeout = time + ServerFailureDelay;
+    if (mOCSPCache.Put(cert, issuerCert, error, time, timeout) != SECSuccess) {
+      return SECFailure;
+    }
+    PR_SetError(error, 0);
     if (mOCSPFetching != FetchOCSPForDVSoftFail) {
       PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
              ("NSSCertDBTrustDomain: returning SECFailure after "
               "CERT_PostOCSPRequest failure"));
       return SECFailure;
     }
     if (cachedResponseErrorCode == SEC_ERROR_OCSP_UNKNOWN_CERT) {
       PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
@@ -315,17 +341,18 @@ NSSCertDBTrustDomain::CheckRevocation(
 
     PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
            ("NSSCertDBTrustDomain: returning SECSuccess after "
             "CERT_PostOCSPRequest failure"));
     return SECSuccess; // Soft fail -> success :(
   }
 
   SECStatus rv = VerifyAndMaybeCacheEncodedOCSPResponse(cert, issuerCert, time,
-                                                        response);
+                                                        response,
+                                                        ResponseIsFromNetwork);
   if (rv == SECSuccess || mOCSPFetching != FetchOCSPForDVSoftFail) {
     PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
       ("NSSCertDBTrustDomain: returning after VerifyEncodedOCSPResponse"));
     return rv;
   }
 
   PRErrorCode error = PR_GetError();
   if (error == SEC_ERROR_OCSP_UNKNOWN_CERT ||
@@ -337,33 +364,47 @@ NSSCertDBTrustDomain::CheckRevocation(
          ("NSSCertDBTrustDomain: end of CheckRevocation"));
 
   return SECSuccess;
 }
 
 SECStatus
 NSSCertDBTrustDomain::VerifyAndMaybeCacheEncodedOCSPResponse(
   const CERTCertificate* cert, CERTCertificate* issuerCert, PRTime time,
-  const SECItem* encodedResponse)
+  const SECItem* encodedResponse, EncodedResponseSource responseSource)
 {
   PRTime thisUpdate = 0;
   PRTime validThrough = 0;
   SECStatus rv = VerifyEncodedOCSPResponse(*this, cert, issuerCert, time,
                                            encodedResponse, &thisUpdate,
                                            &validThrough);
   PRErrorCode error = (rv == SECSuccess ? 0 : PR_GetError());
-  if (rv == SECSuccess || error == SEC_ERROR_REVOKED_CERTIFICATE ||
+  // validThrough is only trustworthy if the response successfully verifies
+  // or it indicates a revoked or unknown certificate.
+  // If this isn't the case, store an indication of failure (to prevent
+  // repeatedly requesting a response from a failing server).
+  if (rv != SECSuccess && error != SEC_ERROR_REVOKED_CERTIFICATE &&
+      error != SEC_ERROR_OCSP_UNKNOWN_CERT) {
+    validThrough = time + ServerFailureDelay;
+  }
+  if (responseSource == ResponseIsFromNetwork ||
+      rv == SECSuccess ||
+      error == SEC_ERROR_REVOKED_CERTIFICATE ||
       error == SEC_ERROR_OCSP_UNKNOWN_CERT) {
     PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
            ("NSSCertDBTrustDomain: caching OCSP response"));
     if (mOCSPCache.Put(cert, issuerCert, error, thisUpdate, validThrough)
           != SECSuccess) {
       return SECFailure;
     }
-    // The call to Put may have un-set the error. Re-set it.
+  }
+
+  // If the verification failed, re-set to that original error
+  // (the call to Put may have un-set it).
+  if (rv != SECSuccess) {
     PR_SetError(error, 0);
   }
   return rv;
 }
 
 namespace {
 
 static char*
--- a/security/certverifier/NSSCertDBTrustDomain.h
+++ b/security/certverifier/NSSCertDBTrustDomain.h
@@ -74,19 +74,24 @@ public:
 
   virtual SECStatus CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
                                     const CERTCertificate* cert,
                           /*const*/ CERTCertificate* issuerCert,
                                     PRTime time,
                        /*optional*/ const SECItem* stapledOCSPResponse);
 
 private:
+  enum EncodedResponseSource {
+    ResponseIsFromNetwork = 1,
+    ResponseWasStapled = 2
+  };
+  static const PRTime ServerFailureDelay = 5 * 60 * PR_USEC_PER_SEC;
   SECStatus VerifyAndMaybeCacheEncodedOCSPResponse(
     const CERTCertificate* cert, CERTCertificate* issuerCert, PRTime time,
-    const SECItem* encodedResponse);
+    const SECItem* encodedResponse, EncodedResponseSource responseSource);
 
   const SECTrustType mCertDBTrustType;
   const OCSPFetching mOCSPFetching;
   OCSPCache& mOCSPCache; // non-owning!
   void* mPinArg; // non-owning!
 };
 
 } } // namespace mozilla::psm
--- a/security/certverifier/OCSPCache.cpp
+++ b/security/certverifier/OCSPCache.cpp
@@ -209,16 +209,26 @@ OCSPCache::Put(const CERTCertificate* aC
     if (mEntries[index]->mThisUpdate > aThisUpdate &&
         aErrorCode != SEC_ERROR_REVOKED_CERTIFICATE) {
       LogWithCerts("OCSPCache::Put(%s, %s) already in cache with more recent "
                    "validity - not replacing", aCert, aIssuerCert);
       MakeMostRecentlyUsed(index, lock);
       return SECSuccess;
     }
 
+    // Only known good responses or responses indicating an unknown
+    // or revoked certificate should replace previously known responses.
+    if (aErrorCode != 0 && aErrorCode != SEC_ERROR_OCSP_UNKNOWN_CERT &&
+        aErrorCode != SEC_ERROR_REVOKED_CERTIFICATE) {
+      LogWithCerts("OCSPCache::Put(%s, %s) already in cache - not replacing "
+                   "with less important status", aCert, aIssuerCert);
+      MakeMostRecentlyUsed(index, lock);
+      return SECSuccess;
+    }
+
     LogWithCerts("OCSPCache::Put(%s, %s) already in cache - replacing",
                  aCert, aIssuerCert);
     mEntries[index]->mErrorCode = aErrorCode;
     mEntries[index]->mThisUpdate = aThisUpdate;
     mEntries[index]->mValidThrough = aValidThrough;
     MakeMostRecentlyUsed(index, lock);
     return SECSuccess;
   }
--- a/security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
+++ b/security/manager/ssl/tests/gtest/OCSPCacheTest.cpp
@@ -2,16 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "CertVerifier.h"
 #include "OCSPCache.h"
 #include "nss.h"
+#include "prerr.h"
 #include "prprf.h"
 #include "secerr.h"
 
 #include "gtest/gtest.h"
 
 const int MaxCacheEntries = 1024;
 
 class OCSPCacheTest : public ::testing::Test
@@ -243,8 +244,40 @@ TEST_F(OCSPCacheTest, Times)
   PRTime timeOut;
   ASSERT_TRUE(cache.Get(&subject, &issuer, errorOut, timeOut));
   // Here we see the more recent time.
   ASSERT_TRUE(errorOut == 0 && timeOut == 200);
 
   // SEC_ERROR_REVOKED_CERTIFICATE overrides everything
   PutAndGet(cache, &subject, &issuer, SEC_ERROR_REVOKED_CERTIFICATE, 50);
 }
+
+TEST_F(OCSPCacheTest, NetworkFailure)
+{
+  SCOPED_TRACE("");
+  CERTCertificate subject;
+  MakeFakeCert(&subject, "CN=subject1", "CN=issuer1", "001", "key001");
+  CERTCertificate issuer;
+  MakeFakeCert(&issuer, "CN=issuer1", "CN=issuer1", "000", "key000");
+  PutAndGet(cache, &subject, &issuer, PR_CONNECT_REFUSED_ERROR, 100);
+  PutAndGet(cache, &subject, &issuer, 0, 200);
+  // This should not override the already present entry.
+  SECStatus rv = cache.Put(&subject, &issuer, PR_CONNECT_REFUSED_ERROR, 300, 350);
+  ASSERT_TRUE(rv == SECSuccess);
+  PRErrorCode errorOut;
+  PRTime timeOut;
+  ASSERT_TRUE(cache.Get(&subject, &issuer, errorOut, timeOut));
+  ASSERT_TRUE(errorOut == 0 && timeOut == 200);
+
+  PutAndGet(cache, &subject, &issuer, SEC_ERROR_OCSP_UNKNOWN_CERT, 400);
+  // This should not override the already present entry.
+  rv = cache.Put(&subject, &issuer, PR_CONNECT_REFUSED_ERROR, 500, 550);
+  ASSERT_TRUE(rv == SECSuccess);
+  ASSERT_TRUE(cache.Get(&subject, &issuer, errorOut, timeOut));
+  ASSERT_TRUE(errorOut == SEC_ERROR_OCSP_UNKNOWN_CERT && timeOut == 400);
+
+  PutAndGet(cache, &subject, &issuer, SEC_ERROR_REVOKED_CERTIFICATE, 600);
+  // This should not override the already present entry.
+  rv = cache.Put(&subject, &issuer, PR_CONNECT_REFUSED_ERROR, 700, 750);
+  ASSERT_TRUE(rv == SECSuccess);
+  ASSERT_TRUE(cache.Get(&subject, &issuer, errorOut, timeOut));
+  ASSERT_TRUE(errorOut == SEC_ERROR_REVOKED_CERTIFICATE && timeOut == 600);
+}
--- a/security/manager/ssl/tests/unit/test_ocsp_caching.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ -102,23 +102,20 @@ function add_tests_in_mode(useMozillaPKI
   add_test(function() { clearOCSPCache(); gFetchCount = 0; run_next_test(); });
 
   // A failure to retrieve an OCSP response will result in an error entry being
   // added to the cache.
   add_connection_test("ocsp-stapling-none.example.com", Cr.NS_OK,
                       clearSessionCache);
   add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
 
-  // TODO(bug 977865): implement this for mozilla::pkix
-  if (!useMozillaPKIX) {
-    // The error entry will prevent a fetch from happening for a while.
-    add_connection_test("ocsp-stapling-none.example.com", Cr.NS_OK,
-                        clearSessionCache);
-    add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
-  }
+  // The error entry will prevent a fetch from happening for a while.
+  add_connection_test("ocsp-stapling-none.example.com", Cr.NS_OK,
+                      clearSessionCache);
+  add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
 
   // The error entry must not prevent a stapled OCSP response from being
   // honored.
   add_connection_test("ocsp-stapling-revoked.example.com",
                       getXPCOMStatusFromNSS(SEC_ERROR_REVOKED_CERTIFICATE),
                       clearSessionCache);
   add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
 
--- a/security/manager/ssl/tests/unit/test_ocsp_required.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_required.js
@@ -48,15 +48,13 @@ function add_tests_in_mode(useMozillaPKI
     run_next_test();
   });
 
   add_connection_test("ocsp-stapling-none.example.com",
                       getXPCOMStatusFromNSS(SEC_ERROR_OCSP_BAD_SIGNATURE));
   add_connection_test("ocsp-stapling-none.example.com",
                       getXPCOMStatusFromNSS(SEC_ERROR_OCSP_BAD_SIGNATURE));
   add_test(function () {
-    // TODO(bug 977865): mozilla::pkix keeps requesting responses from
-    // failing responders
-    do_check_eq(gOCSPRequestCount, useMozillaPKIX ? 2 : 1);
+    do_check_eq(gOCSPRequestCount, 1);
     gOCSPRequestCount = 0;
     run_next_test();
   });
 }