Bug 1039601: Use bounds-checked DERArray instead of plain arrays in pkixocsp.cpp, r=cviecco
authorBrian Smith <brian@briansmith.org>
Wed, 16 Jul 2014 09:54:10 -0700
changeset 14635 e40b8c7a1f7cbbfd863982740e1836ef3f638117
parent 14634 877ad32f2fb0a924d74a6d8aa1ee7bac1951b4f4
child 14636 028a548273ee208df724770232f0e1658052f0f0
push id3202
push userfranziskuskiefer@gmail.com
push dateMon, 01 Oct 2018 08:30:12 +0000
reviewerscviecco
bugs1039601
Bug 1039601: Use bounds-checked DERArray instead of plain arrays in pkixocsp.cpp, r=cviecco
lib/mozpkix/lib/pkixder.h
lib/mozpkix/lib/pkixocsp.cpp
lib/mozpkix/lib/pkixutil.h
lib/mozpkix/test/gtest/pkixder_input_tests.cpp
--- a/lib/mozpkix/lib/pkixder.h
+++ b/lib/mozpkix/lib/pkixder.h
@@ -101,23 +101,16 @@ ExpectTagAndLength(Input& input, uint8_t
 namespace internal {
 
 Result
 ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length);
 
 } // namespace internal
 
 inline Result
-ExpectTagAndSkipLength(Input& input, uint8_t expectedTag)
-{
-  uint16_t ignored;
-  return internal::ExpectTagAndGetLength(input, expectedTag, ignored);
-}
-
-inline Result
 ExpectTagAndSkipValue(Input& input, uint8_t tag)
 {
   uint16_t length;
   Result rv = internal::ExpectTagAndGetLength(input, tag, length);
   if (rv != Success) {
     return rv;
   }
   return input.Skip(length);
--- a/lib/mozpkix/lib/pkixocsp.cpp
+++ b/lib/mozpkix/lib/pkixocsp.cpp
@@ -149,17 +149,17 @@ MOZILLA_PKIX_ENUM_CLASS ResponderIDType 
 
 static inline Result OCSPResponse(Input&, Context&);
 static inline Result ResponseBytes(Input&, Context&);
 static inline Result BasicResponse(Input&, Context&);
 static inline Result ResponseData(
                        Input& tbsResponseData,
                        Context& context,
                        const SignedDataWithSignature& signedResponseData,
-                       /*const*/ SECItem* certs, size_t numCerts);
+                       const DERArray& certs);
 static inline Result SingleResponse(Input& input, Context& context);
 static Result ExtensionNotUnderstood(Input& extnID,
                                      const SECItem& extnValue,
                                      /*out*/ bool& understood);
 static inline Result CertID(Input& input,
                             const Context& context,
                             /*out*/ bool& match);
 static Result MatchKeyHash(TrustDomain& trustDomain,
@@ -223,35 +223,35 @@ VerifyOCSPSignedData(TrustDomain& trustD
 // RFC 6960 section 4.2.2.2: The OCSP responder must either be the issuer of
 // the cert or it must be a delegated OCSP response signing cert directly
 // issued by the issuer. If the OCSP responder is a delegated OCSP response
 // signer, then its certificate is (probably) embedded within the OCSP
 // response and we'll need to verify that it is a valid certificate that chains
 // *directly* to issuerCert.
 static Result
 VerifySignature(Context& context, ResponderIDType responderIDType,
-                const SECItem& responderID, const SECItem* certs,
-                size_t numCerts,
+                const SECItem& responderID, const DERArray& certs,
                 const SignedDataWithSignature& signedResponseData)
 {
   bool match;
   Result rv = MatchResponderID(context.trustDomain, responderIDType,
                                responderID, context.certID.issuer,
                                context.certID.issuerSubjectPublicKeyInfo,
                                match);
   if (rv != Success) {
     return rv;
   }
   if (match) {
     return VerifyOCSPSignedData(context.trustDomain, signedResponseData,
                                 context.certID.issuerSubjectPublicKeyInfo);
   }
 
+  size_t numCerts = certs.GetLength();
   for (size_t i = 0; i < numCerts; ++i) {
-    BackCert cert(certs[i], EndEntityOrCA::MustBeEndEntity, nullptr);
+    BackCert cert(*certs.GetDER(i), EndEntityOrCA::MustBeEndEntity, nullptr);
     rv = cert.Init();
     if (rv != Success) {
       return rv;
     }
     rv = MatchResponderID(context.trustDomain, responderIDType, responderID,
                           cert.GetSubject(), cert.GetSubjectPublicKeyInfo(),
                           match);
     if (rv != Success) {
@@ -408,64 +408,72 @@ BasicResponse(Input& input, Context& con
     if (rv == Result::ERROR_BAD_SIGNATURE) {
       return Result::ERROR_OCSP_BAD_SIGNATURE;
     }
     return rv;
   }
 
   // Parse certificates, if any
 
-  SECItem certs[8];
-  size_t numCerts = 0;
-
+  NonOwningDERArray certs;
   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,
     // and too long and we'll have leftover data that won't parse as a cert.
 
     // [0] wrapper
-    rv = der::ExpectTagAndSkipLength(
-          input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0);
+    Input wrapped;
+    rv = der::ExpectTagAndGetValue(
+          input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0, wrapped);
+    if (rv != Success) {
+      return rv;
+    }
+    rv = der::End(input);
     if (rv != Success) {
       return rv;
     }
 
     // SEQUENCE wrapper
-    rv = der::ExpectTagAndSkipLength(input, der::SEQUENCE);
+    Input certsSequence;
+    rv = der::ExpectTagAndGetValue(wrapped, der::SEQUENCE, certsSequence);
+    if (rv != Success) {
+      return rv;
+    }
+    rv = der::End(wrapped);
     if (rv != Success) {
       return rv;
     }
 
     // sequence of certificates
-    while (!input.AtEnd()) {
-      if (numCerts == PR_ARRAY_SIZE(certs)) {
-        return Result::ERROR_BAD_DER;
-      }
-
-      rv = der::ExpectTagAndGetTLV(input, der::SEQUENCE, certs[numCerts]);
+    while (!certsSequence.AtEnd()) {
+      SECItem cert;
+      rv = der::ExpectTagAndGetTLV(certsSequence, der::SEQUENCE, cert);
       if (rv != Success) {
         return rv;
       }
-      ++numCerts;
+      rv = certs.Append(cert);
+      if (rv != Success) {
+        return rv;
+      }
     }
   }
 
-  return ResponseData(tbsResponseData, context, signedData, certs, numCerts);
+  return ResponseData(tbsResponseData, context, signedData, certs);
 }
 
 // ResponseData ::= SEQUENCE {
 //    version             [0] EXPLICIT Version DEFAULT v1,
 //    responderID             ResponderID,
 //    producedAt              GeneralizedTime,
 //    responses               SEQUENCE OF SingleResponse,
 //    responseExtensions  [1] EXPLICIT Extensions OPTIONAL }
 static inline Result
 ResponseData(Input& input, Context& context,
              const SignedDataWithSignature& signedResponseData,
-             /*const*/ SECItem* certs, size_t numCerts)
+             const DERArray& certs)
 {
   der::Version version;
   Result rv = der::OptionalVersion(input, version);
   if (rv != Success) {
     return rv;
   }
   if (version != der::Version::v1) {
     // TODO: more specific error code for bad version?
@@ -484,17 +492,17 @@ ResponseData(Input& input, Context& cont
                                  responderID);
   if (rv != Success) {
     return rv;
   }
 
   // This is the soonest we can verify the signature. We verify the signature
   // right away to follow the principal of minimizing the processing of data
   // before verifying its signature.
-  rv = VerifySignature(context, responderIDType, responderID, certs, numCerts,
+  rv = VerifySignature(context, responderIDType, responderID, certs,
                        signedResponseData);
   if (rv != Success) {
     return rv;
   }
 
   // TODO: Do we even need to parse this? Should we just skip it?
   PRTime producedAt;
   rv = der::GeneralizedTime(input, producedAt);
--- a/lib/mozpkix/lib/pkixutil.h
+++ b/lib/mozpkix/lib/pkixutil.h
@@ -156,31 +156,34 @@ public:
     // we don't need to initialize the items array because we always check
     // numItems before accessing i.
   }
 
   virtual size_t GetLength() const { return numItems; }
 
   virtual const SECItem* GetDER(size_t i) const
   {
-    return i < numItems ? items[i] : nullptr;
+    return i < numItems ? &items[i] : nullptr;
   }
 
   Result Append(const SECItem& der)
   {
     if (numItems >= MAX_LENGTH) {
       return Result::FATAL_ERROR_INVALID_ARGS;
     }
-    items[numItems] = &der;
+    items[numItems] = der; // structure assignment
     ++numItems;
     return Success;
   }
 
   // Public so we can static_assert on this. Keep in sync with MAX_SUBCA_COUNT.
   static const size_t MAX_LENGTH = 8;
 private:
-  const SECItem* items[MAX_LENGTH]; // avoids any heap allocations
+  SECItem items[MAX_LENGTH]; // avoids any heap allocations
   size_t numItems;
+
+  NonOwningDERArray(const NonOwningDERArray&) /* = delete*/;
+  void operator=(const NonOwningDERArray&) /* = delete*/;
 };
 
 } } // namespace mozilla::pkix
 
 #endif // mozilla_pkix__pkixutil_h
--- a/lib/mozpkix/test/gtest/pkixder_input_tests.cpp
+++ b/lib/mozpkix/test/gtest/pkixder_input_tests.cpp
@@ -722,32 +722,16 @@ TEST_F(pkixder_input_tests, ExpectTagAnd
 {
   Input input;
   ASSERT_EQ(Success,
             input.Init(DER_SEQUENCE_NOT_EMPTY, sizeof DER_SEQUENCE_NOT_EMPTY));
   SECItem tlv;
   ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndGetTLV(input, INTEGER, tlv));
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndSkipLength)
-{
-  Input input;
-  ASSERT_EQ(Success, input.Init(DER_INT16, sizeof DER_INT16));
-  ASSERT_EQ(Success, ExpectTagAndSkipLength(input, INTEGER));
-}
-
-TEST_F(pkixder_input_tests, ExpectTagAndSkipLengthWithWrongTag)
-{
-  Input input;
-  ASSERT_EQ(Success, input.Init(DER_INT16, sizeof DER_INT16));
-
-  ASSERT_EQ(Result::ERROR_BAD_DER,
-            ExpectTagAndSkipLength(input, OCTET_STRING));
-}
-
 TEST_F(pkixder_input_tests, EndAtEnd)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_INT16, sizeof DER_INT16));
   ASSERT_EQ(Success, input.Skip(4));
   ASSERT_EQ(Success, End(input));
 }