Bug 1018411: Factor out signed data parsing in mozilla::pkix into a reusable and separately-testable function, r=keeler
authorBrian Smith <brian@briansmith.org>
Sat, 31 May 2014 18:54:34 -0700
changeset 206530 44d8c7a29df144f61b5f7145ec47b07dfe34fed1
parent 206475 078b39cfcf44bb6513d3ffbd2f38ecd00c4bce94
child 206531 7673b3a0749997dddcbe768371fc99a3f174491b
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1018411
milestone32.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 1018411: Factor out signed data parsing in mozilla::pkix into a reusable and separately-testable function, r=keeler
security/pkix/lib/pkixder.cpp
security/pkix/lib/pkixder.h
security/pkix/lib/pkixocsp.cpp
--- a/security/pkix/lib/pkixder.cpp
+++ b/security/pkix/lib/pkixder.cpp
@@ -85,9 +85,52 @@ ExpectTagAndGetLength(Input& input, uint
   }
 
   // Ensure the input is long enough for the length it says it has.
   return input.EnsureLength(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
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -42,16 +42,18 @@
 
 #include "prerror.h"
 #include "prlog.h"
 #include "secder.h"
 #include "secerr.h"
 #include "secoidt.h"
 #include "stdint.h"
 
+typedef struct CERTSignedDataStr CERTSignedData;
+
 namespace mozilla { namespace pkix { namespace der {
 
 enum Class
 {
    UNIVERSAL = 0 << 6,
 // APPLICATION = 1 << 6, // unused
    CONTEXT_SPECIFIC = 2 << 6,
 // PRIVATE = 3 << 6 // unused
@@ -643,11 +645,30 @@ OptionalVersion(Input& input, /*out*/ ui
     return Failure;
   }
   if (version & 0x80) { // negative
     return Fail(SEC_ERROR_BAD_DER);
   }
   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
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -445,60 +445,25 @@ ResponseBytes(der::Input& input, Context
 // BasicOCSPResponse       ::= SEQUENCE {
 //    tbsResponseData      ResponseData,
 //    signatureAlgorithm   AlgorithmIdentifier,
 //    signature            BIT STRING,
 //    certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL }
 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];
   size_t numCerts = 0;
 
   if (!input.AtEnd()) {
     // We ignore the lengths of the wrappers because we'll detect bad lengths
     // during parsing--too short and we'll run out of input for parsing a cert,