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 c2bc849c4169c0d0704fe2756bd37c01d0c66b96
parent 173972 5854254a309dafd0e4fb3fb197d9e8c04072a9f1
child 173974 cfe51017482ed83079c0101b99d10e363bbabc97
push id26438
push userphilringnalda@gmail.com
push dateTue, 18 Mar 2014 05:39:07 +0000
treeherderautoland@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();