Bug 1018411: Factor out signed data parsing in mozilla::pkix into a reusable and separately-testable function, r=keeler

--HG--
extra : rebase_source : d65a760f9f8efb656f238794019bd451ca163c0b

security/pkix/lib/pkixder.cpp

@@ -90,4 +90,47 @@ ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length)
 
 } // namespace internal
 
+Result
+SignedData(Input& input, /*out*/ Input& tbs, /*out*/ CERTSignedData& signedData)
+{
+  Input::Mark mark(input.GetMark());
+
+  if (ExpectTagAndGetValue(input, SEQUENCE, tbs) != Success) {
+    return Failure;
+  }
+
+  if (input.GetSECItem(siBuffer, mark, signedData.data) != Success) {
+    return Failure;
+  }
+
+  if (Nested(input, SEQUENCE,
+             bind(AlgorithmIdentifier, _1, ref(signedData.signatureAlgorithm)))
+        != Success) {
+    return Failure;
+  }
+
+  if (ExpectTagAndGetValue(input, BIT_STRING, signedData.signature)
+        != Success) {
+    return Failure;
+  }
+  if (signedData.signature.len == 0) {
+    return Fail(SEC_ERROR_BAD_SIGNATURE);
+  }
+  unsigned int unusedBitsAtEnd = signedData.signature.data[0];
+  // XXX: Really the constraint should be that unusedBitsAtEnd must be less
+  // than 7. But, we suspect there are no real-world OCSP responses or X.509
+  // certificates with non-zero unused bits. It seems like NSS assumes this in
+  // various places, so we enforce it too in order to simplify this code. If we
+  // find compatibility issues, we'll know we're wrong and we'll have to figure
+  // out how to shift the bits around.
+  if (unusedBitsAtEnd != 0) {
+    return Fail(SEC_ERROR_BAD_SIGNATURE);
+  }
+  ++signedData.signature.data;
+  --signedData.signature.len;
+  signedData.signature.len = (signedData.signature.len << 3); // Bytes to bits
+
+  return Success;
+}
+
 } } } // namespace mozilla::pkix::der

security/pkix/lib/pkixder.h

@@ -47,6 +47,8 @@
 #include "secoidt.h"
 #include "stdint.h"
 
+typedef struct CERTSignedDataStr CERTSignedData;
+
 namespace mozilla { namespace pkix { namespace der {
 
 enum Class
@@ -648,6 +650,25 @@ OptionalVersion(Input& input, /*out*/ uint8_t& version)
   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
+// the case of an OCSP response, the caller needs to parse the certs
+// separately.
+//
+// Certificate  ::=  SEQUENCE  {
+//        tbsCertificate       TBSCertificate,
+//        signatureAlgorithm   AlgorithmIdentifier,
+//        signatureValue       BIT STRING  }
+//
+// BasicOCSPResponse       ::= SEQUENCE {
+//    tbsResponseData      ResponseData,
+//    signatureAlgorithm   AlgorithmIdentifier,
+//    signature            BIT STRING,
+//    certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL }
+Result
+SignedData(Input& input, /*out*/ Input& tbs, /*out*/ CERTSignedData& signedData);
+
 } } } // namespace mozilla::pkix::der
 
 #endif // mozilla_pkix__pkixder_h

security/pkix/lib/pkixocsp.cpp

@@ -450,50 +450,15 @@ ResponseBytes(der::Input& input, Context& context)
 der::Result
 BasicResponse(der::Input& input, Context& context)
 {
-  der::Input::Mark mark(input.GetMark());
-
-  // The signature covers the entire DER encoding of tbsResponseData, including
-  // the beginning tag and length. However, when we're parsing tbsResponseData,
-  // we want to strip off the tag and length because we don't need it after
-  // we've confirmed it's there and figured out what length it is.
-
   der::Input tbsResponseData;
-  if (der::ExpectTagAndGetValue(input, der::SEQUENCE, tbsResponseData)
-        != der::Success) {
-    return der::Failure;
-  }
-
   CERTSignedData signedData;
-
-  if (input.GetSECItem(siBuffer, mark, signedData.data) != der::Success) {
+  if (der::SignedData(input, tbsResponseData, signedData) != der::Success) {
+    if (PR_GetError() == SEC_ERROR_BAD_SIGNATURE) {
+      PR_SetError(SEC_ERROR_OCSP_BAD_SIGNATURE, 0);
+    }
     return der::Failure;
   }
 
-  if (der::Nested(input, der::SEQUENCE,
-                  bind(der::AlgorithmIdentifier, _1,
-                       ref(signedData.signatureAlgorithm))) != der::Success) {
-    return der::Failure;
-  }
-
-  if (der::ExpectTagAndGetValue(input, der::BIT_STRING, signedData.signature)
-        != der::Success) {
-    return der::Failure;
-  }
-  if (signedData.signature.len == 0) {
-    return der::Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);
-  }
-  unsigned int unusedBitsAtEnd = signedData.signature.data[0];
-  // XXX: Really the constraint should be that unusedBitsAtEnd must be less
-  // than 7. But, we suspect there are no valid OCSP response signatures with
-  // non-zero unused bits. It seems like NSS assumes this in various places, so
-  // we enforce it. If we find compatibility issues, we'll know we're wrong.
-  if (unusedBitsAtEnd != 0) {
-    return der::Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);
-  }
-  ++signedData.signature.data;
-  --signedData.signature.len;
-  signedData.signature.len = (signedData.signature.len << 3); // Bytes to bits
-
   // Parse certificates, if any
 
   SECItem certs[8];