bug 991898 - mozilla::pkix: temporarily allow empty Extensions in OCSP responses r=briansmith
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 17 Apr 2014 16:01:18 -0700
changeset 179515 f2bd6f0ab7619add682680e9ad1c5e2df7897ded
parent 179514 9dfe9f74ddd25dc4879f1bb9c0fd3d2ef78ed728
child 179516 280c066afbf185fccd74029d177c9824f4e6e381
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewersbriansmith
bugs991898
milestone31.0a1
bug 991898 - mozilla::pkix: temporarily allow empty Extensions in OCSP responses r=briansmith
security/manager/ssl/tests/unit/test_ocsp_stapling.js
security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h
security/pkix/lib/pkixocsp.cpp
security/pkix/test/lib/pkixtestutil.cpp
security/pkix/test/lib/pkixtestutil.h
--- a/security/manager/ssl/tests/unit/test_ocsp_stapling.js
+++ b/security/manager/ssl/tests/unit/test_ocsp_stapling.js
@@ -40,16 +40,17 @@ function add_tests_in_mode(useMozillaPKI
   add_ocsp_test("ocsp-stapling-unknown.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-good-other.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-none.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-expired.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-expired-fresh-ca.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-skip-responseBytes.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-critical-extension.example.com", Cr.NS_OK, false);
   add_ocsp_test("ocsp-stapling-noncritical-extension.example.com", Cr.NS_OK, false);
+  add_ocsp_test("ocsp-stapling-empty-extensions.example.com", Cr.NS_OK, false);
 
   // Now test OCSP stapling
   // The following error codes are defined in security/nss/lib/util/SECerrs.h
 
   add_ocsp_test("ocsp-stapling-good.example.com", Cr.NS_OK, true);
 
   add_ocsp_test("ocsp-stapling-revoked.example.com",
                 getXPCOMStatusFromNSS(SEC_ERROR_REVOKED_CERTIFICATE), true);
@@ -121,16 +122,18 @@ function add_tests_in_mode(useMozillaPKI
   }
 
   add_ocsp_test("ocsp-stapling-critical-extension.example.com",
                 useMozillaPKIX
                   ? getXPCOMStatusFromNSS(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION)
                   : Cr.NS_OK, // TODO(bug 987426): NSS doesn't handle unknown critical extensions
                 true);
   add_ocsp_test("ocsp-stapling-noncritical-extension.example.com", Cr.NS_OK, true);
+  // TODO(bug 997994): Disallow empty Extensions in responses
+  add_ocsp_test("ocsp-stapling-empty-extensions.example.com", Cr.NS_OK, true);
 
   add_ocsp_test("ocsp-stapling-delegated-included.example.com", Cr.NS_OK, true);
   add_ocsp_test("ocsp-stapling-delegated-included-last.example.com", Cr.NS_OK, true);
   add_ocsp_test("ocsp-stapling-delegated-missing.example.com",
                 getXPCOMStatusFromNSS(SEC_ERROR_OCSP_INVALID_SIGNING_CERT), true);
   add_ocsp_test("ocsp-stapling-delegated-missing-multiple.example.com",
                 getXPCOMStatusFromNSS(SEC_ERROR_OCSP_INVALID_SIGNING_CERT), true);
   add_ocsp_test("ocsp-stapling-delegated-no-extKeyUsage.example.com",
@@ -148,18 +151,18 @@ function add_tests_in_mode(useMozillaPKI
 }
 
 function check_ocsp_stapling_telemetry() {
   let histogram = Cc["@mozilla.org/base/telemetry;1"]
                     .getService(Ci.nsITelemetry)
                     .getHistogramById("SSL_OCSP_STAPLING")
                     .snapshot();
   do_check_eq(histogram.counts[0], 2 * 0); // histogram bucket 0 is unused
-  do_check_eq(histogram.counts[1], 4 + 5); // 4 or 5 connections with a good response (bug 987426)
-  do_check_eq(histogram.counts[2], 2 * 17); // 17 connections with no stapled resp.
+  do_check_eq(histogram.counts[1], 5 + 6); // 5 or 6 connections with a good response (bug 987426)
+  do_check_eq(histogram.counts[2], 2 * 18); // 18 connections with no stapled resp.
   do_check_eq(histogram.counts[3], 2 * 0); // 0 connections with an expired response
   do_check_eq(histogram.counts[4], 19 + 17); // 19 or 17 connections with bad responses (bug 979070, bug 987426)
   run_next_test();
 }
 
 function run_test() {
   do_get_profile();
 
--- a/security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp
@@ -35,16 +35,17 @@ const OCSPHost sOCSPHosts[] =
   { "ocsp-stapling-trylater.example.com", ORTTryLater, nullptr },
   { "ocsp-stapling-needssig.example.com", ORTNeedsSig, nullptr },
   { "ocsp-stapling-unauthorized.example.com", ORTUnauthorized, nullptr },
   { "ocsp-stapling-with-intermediate.example.com", ORTGood, "ocspEEWithIntermediate" },
   { "ocsp-stapling-bad-signature.example.com", ORTBadSignature, nullptr },
   { "ocsp-stapling-skip-responseBytes.example.com", ORTSkipResponseBytes, nullptr },
   { "ocsp-stapling-critical-extension.example.com", ORTCriticalExtension, nullptr },
   { "ocsp-stapling-noncritical-extension.example.com", ORTNoncriticalExtension, nullptr },
+  { "ocsp-stapling-empty-extensions.example.com", ORTEmptyExtensions, nullptr },
   { "ocsp-stapling-delegated-included.example.com", ORTDelegatedIncluded, "delegatedSigner" },
   { "ocsp-stapling-delegated-included-last.example.com", ORTDelegatedIncludedLast, "delegatedSigner" },
   { "ocsp-stapling-delegated-missing.example.com", ORTDelegatedMissing, "delegatedSigner" },
   { "ocsp-stapling-delegated-missing-multiple.example.com", ORTDelegatedMissingMultiple, "delegatedSigner" },
   { "ocsp-stapling-delegated-no-extKeyUsage.example.com", ORTDelegatedIncluded, "invalidDelegatedSignerNoExtKeyUsage" },
   { "ocsp-stapling-delegated-from-intermediate.example.com", ORTDelegatedIncluded, "invalidDelegatedSignerFromIntermediate" },
   { "ocsp-stapling-delegated-keyUsage-crlSigning.example.com", ORTDelegatedIncluded, "invalidDelegatedSignerKeyUsageCrlSigning" },
   { "ocsp-stapling-delegated-wrong-extKeyUsage.example.com", ORTDelegatedIncluded, "invalidDelegatedSignerWrongExtKeyUsage" },
--- a/security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
+++ b/security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ -139,16 +139,19 @@ GetOCSPResponseForType(OCSPResponseType 
     }
     extension.critical = (aORT == ORTCriticalExtension);
     static const uint8_t value[2] = { 0x05, 0x00 };
     extension.value.data = const_cast<uint8_t*>(value);
     extension.value.len = PR_ARRAY_SIZE(value);
     extension.next = nullptr;
     context.extensions = &extension;
   }
+  if (aORT == ORTEmptyExtensions) {
+    context.includeEmptyExtensions = true;
+  }
 
   if (!context.signerCert) {
     context.signerCert = CERT_DupCertificate(context.issuerCert.get());
   }
 
   SECItem* response = CreateEncodedOCSPResponse(context);
   if (!response) {
     PrintPRError("CreateEncodedOCSPResponse failed");
--- a/security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h
+++ b/security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h
@@ -27,16 +27,17 @@ enum OCSPResponseType
   ORTSrverr,           // the response indicates there was a server error
   ORTTryLater,         // the responder replied with "try again later"
   ORTNeedsSig,         // the response needs a signature
   ORTUnauthorized,     // the responder is not authorized for this certificate
   ORTBadSignature,     // the response has a signature that does not verify
   ORTSkipResponseBytes, // the response does not include responseBytes
   ORTCriticalExtension, // the response includes a critical extension
   ORTNoncriticalExtension, // the response includes an extension that is not critical
+  ORTEmptyExtensions,  // the response includes a SEQUENCE OF Extension that is empty
   ORTDelegatedIncluded, // the response is signed by an included delegated responder
   ORTDelegatedIncludedLast, // same, but multiple other certificates are included
   ORTDelegatedMissing, // the response is signed by a not included delegated responder
   ORTDelegatedMissingMultiple, // same, but multiple other certificates are included
 };
 
 struct OCSPHost
 {
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -853,21 +853,25 @@ CheckExtensionForCriticality(der::Input&
 
   if (ExpectTagAndGetLength(input, der::OCTET_STRING, toSkip)
         != der::Success) {
     return der::Failure;
   }
   return input.Skip(toSkip);
 }
 
+// Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension
 static der::Result
 CheckExtensionsForCriticality(der::Input& input)
 {
+  // TODO(bug 997994): some responders include an empty SEQUENCE OF
+  // Extension, which is invalid (der::MayBeEmpty should really be
+  // der::MustNotBeEmpty).
   return der::NestedOf(input, der::SEQUENCE, der::SEQUENCE,
-                       der::MustNotBeEmpty, CheckExtensionForCriticality);
+                       der::MayBeEmpty, CheckExtensionForCriticality);
 }
 
 //   1. The certificate identified in a received response corresponds to
 //      the certificate that was identified in the corresponding request;
 //   2. The signature on the response is valid;
 //   3. The identity of the signer matches the intended recipient of the
 //      request;
 //   4. The signer is currently authorized to provide a response for the
--- a/security/pkix/test/lib/pkixtestutil.cpp
+++ b/security/pkix/test/lib/pkixtestutil.cpp
@@ -125,16 +125,17 @@ OCSPResponseContext::OCSPResponseContext
   , nextUpdate(time + 10 * PR_USEC_PER_SEC)
   , includeNextUpdate(true)
   , certIDHashAlg(SEC_OID_SHA1)
   , certStatus(0)
   , revocationTime(0)
   , badSignature(false)
   , responderIDType(ByKeyHash)
   , extensions(nullptr)
+  , includeEmptyExtensions(false)
 {
   for (size_t i = 0; i < MaxIncludedCertificates; i++) {
     includedCertificates[i] = nullptr;
   }
 }
 
 static SECItem* ResponseBytes(OCSPResponseContext& context);
 static SECItem* BasicOCSPResponse(OCSPResponseContext& context);
@@ -508,17 +509,17 @@ ResponseData(OCSPResponseContext& contex
     return nullptr;
   }
   SECItem* responsesNested = EncodeNested(context.arena, der::SEQUENCE,
                                           responses);
   if (!responsesNested) {
     return nullptr;
   }
   SECItem* responseExtensions = nullptr;
-  if (context.extensions) {
+  if (context.extensions || context.includeEmptyExtensions) {
     responseExtensions = Extensions(context);
   }
 
   Output output;
   if (output.Add(responderID) != der::Success) {
     return nullptr;
   }
   if (output.Add(producedAtEncoded) != der::Success) {
--- a/security/pkix/test/lib/pkixtestutil.h
+++ b/security/pkix/test/lib/pkixtestutil.h
@@ -62,16 +62,19 @@ public:
 
   enum ResponderIDType {
     ByName = 1,
     ByKeyHash = 2
   };
   ResponderIDType responderIDType;
 
   OCSPResponseExtension* extensions;
+  bool includeEmptyExtensions; // If true, include the extension wrapper
+                               // regardless of if there are any actual
+                               // extensions.
 };
 
 // The return value, if non-null, is owned by the arena in the context
 // and MUST NOT be freed.
 // This function does its best to respect the NSPR error code convention
 // (that is, if it returns null, calling PR_GetError() will return the
 // error of the failed operation). However, this is not guaranteed.
 SECItem* CreateEncodedOCSPResponse(OCSPResponseContext& context);