bug 977870 - insanity::pkix: consume the rest of input when a CertID doesn't match in an OCSP response r=briansmith
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 17 Mar 2014 14:34:34 -0700
changeset 173973 c2bc849c4169
parent 173972 5854254a309d
child 173974 cfe51017482e
push id26438
push userphilringnalda@gmail.com
push date2014-03-18 05:39 +0000
treeherdermozilla-central@89275f0ae29f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith
bugs977870
milestone31.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 977870 - insanity::pkix: consume the rest of input when a CertID doesn't match in an OCSP response r=briansmith
security/insanity/lib/pkixder.h
security/insanity/lib/pkixocsp.cpp
security/manager/ssl/tests/unit/test_ocsp_stapling.js
--- a/security/insanity/lib/pkixder.h
+++ b/security/insanity/lib/pkixder.h
@@ -158,16 +158,21 @@ public:
     }
     skippedItem.type = siBuffer;
     skippedItem.data = const_cast<uint8_t*>(input);
     skippedItem.len = len;
     input += len;
     return Success;
   }
 
+  void SkipToEnd()
+  {
+    input = end;
+  }
+
   bool AtEnd() const { return input == end; }
 
   class Mark
   {
   private:
     friend class Input;
     explicit Mark(const uint8_t* mark) : mMark(mark) { }
     const uint8_t* const mMark;
--- a/security/insanity/lib/pkixocsp.cpp
+++ b/security/insanity/lib/pkixocsp.cpp
@@ -598,16 +598,21 @@ SingleResponse(der::Input& input, Contex
   bool match = false;
   if (der::Nested(input, der::SEQUENCE,
                   bind(CertID, _1, cref(context), ref(match)))
         != der::Success) {
     return der::Failure;
   }
 
   if (!match) {
+    // This response does not reference the certificate we're interested in.
+    // By consuming the rest of our input and returning successfully, we can
+    // continue processing and examine another response that might have what
+    // we want.
+    input.SkipToEnd();
     return der::Success;
   }
 
   // CertStatus ::= CHOICE {
   //     good        [0]     IMPLICIT NULL,
   //     revoked     [1]     IMPLICIT RevokedInfo,
   //     unknown     [2]     IMPLICIT UnknownInfo }
   //
@@ -740,39 +745,47 @@ CertID(der::Input& input, const Context&
   if (der::CertificateSerialNumber(input, serialNumber) != der::Success) {
     return der::Failure;
   }
 
   const CERTCertificate& cert = context.cert;
   const CERTCertificate& issuerCert = context.issuerCert;
 
   if (!SECITEM_ItemsAreEqual(&serialNumber, &cert.serialNumber)) {
+    // This does not reference the certificate we're interested in.
+    // Consume the rest of the input and return successfully to
+    // potentially continue processing other responses.
+    input.SkipToEnd();
     return der::Success;
   }
 
   // TODO: support SHA-2 hashes.
 
   SECOidTag hashAlg = SECOID_GetAlgorithmTag(&hashAlgorithm);
   if (hashAlg != SEC_OID_SHA1) {
+    // Again, not interested in this response. Consume input, return success.
+    input.SkipToEnd();
     return der::Success;
   }
 
   if (issuerNameHash.len != SHA1_LENGTH) {
     return der::Fail(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
   }
 
   // From http://tools.ietf.org/html/rfc6960#section-4.1.1:
   // "The hash shall be calculated over the DER encoding of the
   // issuer's name field in the certificate being checked."
   uint8_t hashBuf[SHA1_LENGTH];
   if (PK11_HashBuf(SEC_OID_SHA1, hashBuf, cert.derIssuer.data,
                    cert.derIssuer.len) != SECSuccess) {
     return der::Failure;
   }
   if (memcmp(hashBuf, issuerNameHash.data, issuerNameHash.len)) {
+    // Again, not interested in this response. Consume input, return success.
+    input.SkipToEnd();
     return der::Success;
   }
 
   return MatchIssuerKey(issuerKeyHash, issuerCert, match);
 }
 
 // From http://tools.ietf.org/html/rfc6960#section-4.1.1:
 // "The hash shall be calculated over the value (excluding tag and length) of
--- a/security/manager/ssl/tests/unit/test_ocsp_stapling.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_stapling.js
@@ -90,21 +90,18 @@ function add_tests_in_mode(useInsanity, 
                 getXPCOMStatusFromNSS(SEC_ERROR_OCSP_TRY_SERVER_LATER), true);
   add_ocsp_test("ocsp-stapling-needssig.example.com",
                 getXPCOMStatusFromNSS(SEC_ERROR_OCSP_REQUEST_NEEDS_SIG), true);
   add_ocsp_test("ocsp-stapling-unauthorized.example.com",
                 getXPCOMStatusFromNSS(SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST),
                 true);
   add_ocsp_test("ocsp-stapling-unknown.example.com",
                 getXPCOMStatusFromNSS(SEC_ERROR_OCSP_UNKNOWN_CERT), true);
-  // TODO(bug 977870): this should not result in SEC_ERROR_BAD_DER
   add_ocsp_test("ocsp-stapling-good-other.example.com",
-                getXPCOMStatusFromNSS(
-                  useInsanity ? SEC_ERROR_BAD_DER
-                              : SEC_ERROR_OCSP_UNKNOWN_CERT), true);
+                getXPCOMStatusFromNSS(SEC_ERROR_OCSP_UNKNOWN_CERT), true);
   // If the server doesn't staple an OCSP response, we continue as normal
   // (this means that even though stapling is enabled, we expect an OCSP
   // request).
   add_connection_test("ocsp-stapling-none.example.com", Cr.NS_OK,
     function() {
       gExpectOCSPRequest = true;
       clearOCSPCache();
       clearSessionCache();