Backed out changeset a10da316a35f (bug 1029341)

security/pkix/lib/pkixder.h

@@ -655,74 +655,6 @@ OptionalVersion(Input& input, /*out*/ Version& version)
   return Success;
 }
 
-template <typename ExtensionHandler>
-inline Result
-OptionalExtensions(Input& input, uint8_t tag, ExtensionHandler extensionHandler)
-{
-  if (!input.Peek(tag)) {
-    return Success;
-  }
-
-  Input extensions;
-  {
-    Input tagged;
-    if (ExpectTagAndGetValue(input, tag, tagged) != Success) {
-      return Failure;
-    }
-    if (ExpectTagAndGetValue(tagged, SEQUENCE, extensions) != Success) {
-      return Failure;
-    }
-    if (End(tagged) != Success) {
-      return Failure;
-    }
-  }
-
-  // Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension
-  //
-  // TODO(bug 997994): According to the specification, there should never be
-  // an empty sequence of extensions but we've found OCSP responses that have
-  // that (see bug 991898).
-  while (!extensions.AtEnd()) {
-    Input extension;
-    if (ExpectTagAndGetValue(extensions, SEQUENCE, extension)
-          != Success) {
-      return Failure;
-    }
-
-    // Extension  ::=  SEQUENCE  {
-    //      extnID      OBJECT IDENTIFIER,
-    //      critical    BOOLEAN DEFAULT FALSE,
-    //      extnValue   OCTET STRING
-    //      }
-    Input extnID;
-    if (ExpectTagAndGetValue(extension, OIDTag, extnID) != Success) {
-      return Failure;
-    }
-    bool critical;
-    if (OptionalBoolean(extension, false, critical) != Success) {
-      return Failure;
-    }
-    SECItem extnValue;
-    if (ExpectTagAndGetValue(extension, OCTET_STRING, extnValue)
-          != Success) {
-      return Failure;
-    }
-    if (End(extension) != Success) {
-      return Failure;
-    }
-
-    bool understood = false;
-    if (extensionHandler(extnID, extnValue, understood) != Success) {
-      return Failure;
-    }
-    if (critical && !understood) {
-      return Fail(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);
-    }
-  }
-
-  return Success;
-}
-
 // Parses a SEQUENCE into tbs and then parses an AlgorithmIdentifier followed
 // by a BIT STRING into signedData. This handles the commonality between
 // parsing the signed/signature fields of certificates and OCSP responses. In

security/pkix/lib/pkixocsp.cpp

@@ -179,9 +179,7 @@ static inline der::Result ResponseData(
                               /*const*/ SECItem* certs, size_t numCerts);
 static inline der::Result SingleResponse(der::Input& input,
                                           Context& context);
-static der::Result ExtensionNotUnderstood(der::Input& extnID,
-                                          const SECItem& extnValue,
-                                          /*out*/ bool& understood);
+static inline der::Result CheckExtensionsForCriticality(der::Input&);
 static inline der::Result CertID(der::Input& input,
                                   const Context& context,
                                   /*out*/ bool& match);
@@ -533,9 +531,14 @@ ResponseData(der::Input& input, Context& context,
     return der::Failure;
   }
 
-  return der::OptionalExtensions(input,
-                                 der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
-                                 ExtensionNotUnderstood);
+  if (!input.AtEnd()) {
+    if (der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
+                    CheckExtensionsForCriticality) != der::Success) {
+      return der::Failure;
+    }
+  }
+
+  return der::Success;
 }
 
 // SingleResponse ::= SEQUENCE {
@@ -653,11 +656,11 @@ SingleResponse(der::Input& input, Context& context)
     context.expired = true;
   }
 
-  if (der::OptionalExtensions(input,
-                              der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
-                              ExtensionNotUnderstood)
-        != der::Success) {
-    return der::Failure;
+  if (!input.AtEnd()) {
+    if (der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
+                    CheckExtensionsForCriticality) != der::Success) {
+      return der::Failure;
+    }
   }
 
   if (context.thisUpdate) {
@@ -835,14 +838,41 @@ KeyHash(const SECItem& subjectPublicKeyInfo, /*out*/ uint8_t* hashBuf,
   return Success;
 }
 
-der::Result
-ExtensionNotUnderstood(der::Input& /*extnID*/, const SECItem& /*extnValue*/,
-                       /*out*/ bool& understood)
+// Extension  ::=  SEQUENCE  {
+//      extnID      OBJECT IDENTIFIER,
+//      critical    BOOLEAN DEFAULT FALSE,
+//      extnValue   OCTET STRING
+//      }
+static der::Result
+CheckExtensionForCriticality(der::Input& input)
 {
-  understood = false;
+  // TODO: maybe we should check the syntax of the OID value
+  if (ExpectTagAndSkipValue(input, der::OIDTag) != der::Success) {
+    return der::Failure;
+  }
+
+  // The only valid explicit encoding of the value is TRUE, so don't even
+  // bother parsing it, since we're going to fail either way.
+  if (input.Peek(der::BOOLEAN)) {
+    return der::Fail(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);
+  }
+
+  input.SkipToEnd();
+
   return der::Success;
 }
 
+// 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::EmptyAllowed::Yes, 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;