bug 1079436 - fix validThrough as returned by VerifyEncodedOCSPResponse r=briansmith

validThrough should now be the time through which, if passed in as the given
time to validate an OCSP response at, VerifyEncodedOCSPResponse will still
consider it trustworthy. After that time, it will be expired. This makes it
so the OCSP cache compares validity period responses consistently with
mozilla::pkix.

security/certverifier/NSSCertDBTrustDomain.cpp

@@ -532,7 +532,7 @@ NSSCertDBTrustDomain::CheckRevocation(EndEntityOrCA endEntityOrCA,
     Result error = rv;
     if (attemptedRequest) {
       Time timeout(time);
-      if ( timeout.AddSeconds(ServerFailureDelaySeconds) != Success) {
+      if (timeout.AddSeconds(ServerFailureDelaySeconds) != Success) {
         return Result::FATAL_ERROR_LIBRARY_FAILURE; // integer overflow
       }
       rv = mOCSPCache.Put(certID, error, time, timeout);

security/pkix/include/pkix/pkix.h

@@ -113,8 +113,9 @@ Result CreateEncodedOCSPRequest(TrustDomain& trustDomain,
 // good, unknown, or revoked responses that verify correctly are considered
 // trustworthy. If the response is not trustworthy, thisUpdate will be 0.
 // Similarly, the optional parameter validThrough will be the time through
-// which the encoded response is considered trustworthy (that is, if a response had a
-// thisUpdate time of validThrough, it would be considered trustworthy).
+// which the encoded response is considered trustworthy (that is, as long as
+// the given time at which to validate is less than or equal to validThrough,
+// the response will be considered trustworthy).
 Result VerifyEncodedOCSPResponse(TrustDomain& trustDomain,
                                  const CertID& certID, Time time,
                                  uint16_t maxLifetimeInDays,

security/pkix/lib/pkixocsp.cpp

@@ -630,12 +630,15 @@ SingleResponse(Reader& input, Context& context)
     }
   }
 
-  Time timeMinusSlop(context.time);
-  rv = timeMinusSlop.SubtractSeconds(SLOP_SECONDS);
+  // Add some slop to hopefully handle clock-skew.
+  Time notAfterPlusSlop(notAfter);
+  rv = notAfterPlusSlop.AddSeconds(SLOP_SECONDS);
   if (rv != Success) {
-    return rv;
+    // This could only happen if we're dealing with times beyond the year
+    // 10,000AD.
+    return Result::ERROR_OCSP_FUTURE_RESPONSE;
   }
-  if (timeMinusSlop > notAfter) {
+  if (context.time > notAfterPlusSlop) {
     context.expired = true;
   }
 
@@ -650,7 +653,7 @@ SingleResponse(Reader& input, Context& context)
     *context.thisUpdate = thisUpdate;
   }
   if (context.validThrough) {
-    *context.validThrough = notAfter;
+    *context.validThrough = notAfterPlusSlop;
   }
 
   return Success;

security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp

@@ -382,6 +382,65 @@ TEST_F(pkixocsp_VerifyEncodedResponse_successful,
   ASSERT_FALSE(expired);
 }
 
+// Added for bug 1079436. The output variable validThrough represents the
+// latest time for which VerifyEncodedOCSPResponse will succeed, which is
+// different from the nextUpdate time in the OCSP response due to the slop we
+// add for time comparisons to deal with clock skew.
+TEST_F(pkixocsp_VerifyEncodedResponse_successful, check_validThrough)
+{
+  ByteString responseString(
+               CreateEncodedOCSPSuccessfulResponse(
+                         OCSPResponseContext::good, *endEntityCertID, byKey,
+                         *rootKeyPair, oneDayBeforeNow,
+                         oneDayBeforeNow, &oneDayAfterNow,
+                         sha256WithRSAEncryption));
+  Time validThrough(Time::uninitialized);
+  {
+    Input response;
+    ASSERT_EQ(Success,
+              response.Init(responseString.data(), responseString.length()));
+    bool expired;
+    ASSERT_EQ(Success,
+              VerifyEncodedOCSPResponse(trustDomain, *endEntityCertID,
+                                        Now(), END_ENTITY_MAX_LIFETIME_IN_DAYS,
+                                        response, expired, nullptr,
+                                        &validThrough));
+    ASSERT_FALSE(expired);
+    // The response was created to be valid until one day after now, so the
+    // value we got for validThrough should be after that.
+    Time oneDayAfterNowAsPKIXTime(TimeFromEpochInSeconds(oneDayAfterNow));
+    ASSERT_TRUE(validThrough > oneDayAfterNowAsPKIXTime);
+  }
+  {
+    Input response;
+    ASSERT_EQ(Success,
+              response.Init(responseString.data(), responseString.length()));
+    bool expired;
+    // Given validThrough from a previous verification, this response should be
+    // valid through that time.
+    ASSERT_EQ(Success,
+              VerifyEncodedOCSPResponse(trustDomain, *endEntityCertID,
+                                        validThrough, END_ENTITY_MAX_LIFETIME_IN_DAYS,
+                                        response, expired));
+    ASSERT_FALSE(expired);
+  }
+  {
+    Time noLongerValid(validThrough);
+    ASSERT_EQ(Success, noLongerValid.AddSeconds(1));
+    Input response;
+    ASSERT_EQ(Success,
+              response.Init(responseString.data(), responseString.length()));
+    bool expired;
+    // The verification time is now after when the response will be considered
+    // valid.
+    ASSERT_EQ(Result::ERROR_OCSP_OLD_RESPONSE,
+              VerifyEncodedOCSPResponse(trustDomain, *endEntityCertID,
+                                        noLongerValid, END_ENTITY_MAX_LIFETIME_IN_DAYS,
+                                        response, expired));
+    ASSERT_TRUE(expired);
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // indirect responses (signed by a delegated OCSP responder cert)