bug 991898 - mozilla::pkix: temporarily allow empty Extensions in OCSP responses r=briansmith

security/manager/ssl/tests/unit/test_ocsp_stapling.js

@@ -45,6 +45,7 @@ function add_tests_in_mode(useMozillaPKIX, certDB, otherTestCA) {
   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
@@ -126,6 +127,8 @@ function add_tests_in_mode(useMozillaPKIX, certDB, otherTestCA) {
                   : 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);
@@ -153,8 +156,8 @@ function check_ocsp_stapling_telemetry() {
                     .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();

security/manager/ssl/tests/unit/tlsserver/cmd/OCSPStaplingServer.cpp

@@ -40,6 +40,7 @@ const OCSPHost sOCSPHosts[] =
   { "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" },

security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp

@@ -144,6 +144,9 @@ GetOCSPResponseForType(OCSPResponseType aORT, CERTCertificate *aCert,
     extension.next = nullptr;
     context.extensions = &extension;
   }
+  if (aORT == ORTEmptyExtensions) {
+    context.includeEmptyExtensions = true;
+  }
 
   if (!context.signerCert) {
     context.signerCert = CERT_DupCertificate(context.issuerCert.get());

security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h

@@ -32,6 +32,7 @@ enum OCSPResponseType
   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

security/pkix/lib/pkixocsp.cpp

@@ -858,11 +858,15 @@ CheckExtensionForCriticality(der::Input& input)
   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

security/pkix/test/lib/pkixtestutil.cpp

@@ -130,6 +130,7 @@ OCSPResponseContext::OCSPResponseContext(PLArenaPool* arena,
   , badSignature(false)
   , responderIDType(ByKeyHash)
   , extensions(nullptr)
+  , includeEmptyExtensions(false)
 {
   for (size_t i = 0; i < MaxIncludedCertificates; i++) {
     includedCertificates[i] = nullptr;
@@ -513,7 +514,7 @@ ResponseData(OCSPResponseContext& context)
     return nullptr;
   }
   SECItem* responseExtensions = nullptr;
-  if (context.extensions) {
+  if (context.extensions || context.includeEmptyExtensions) {
     responseExtensions = Extensions(context);
   }
 

security/pkix/test/lib/pkixtestutil.h

@@ -67,6 +67,9 @@ public:
   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