Bug 1083539: Factor out common SEQUENCE unwrapping logic into reusable functions, r=mmc
authorBrian Smith <brian@briansmith.org>
Tue, 14 Oct 2014 02:07:08 -0700
changeset 212045 4c4a06c589989a1c67b0c46df5177cb84c365ad7
parent 212044 b225e2984eeb05fba48874acd22143186e6350ff
child 212046 15f27345d25982d0c5559b53263acdeb36180b8b
push id27697
push usercbook@mozilla.com
push dateFri, 24 Oct 2014 13:48:53 +0000
treeherdermozilla-central@de805196bbc4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmmc
bugs1083539
milestone36.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 1083539: Factor out common SEQUENCE unwrapping logic into reusable functions, r=mmc
security/pkix/lib/pkixcert.cpp
security/pkix/lib/pkixder.h
security/pkix/lib/pkixnames.cpp
security/pkix/lib/pkixocsp.cpp
--- a/security/pkix/lib/pkixcert.cpp
+++ b/security/pkix/lib/pkixcert.cpp
@@ -37,23 +37,18 @@ BackCert::Init()
   //         signatureAlgorithm   AlgorithmIdentifier,
   //         signatureValue       BIT STRING  }
 
   Reader tbsCertificate;
 
   // The scope of |input| and |certificate| are limited to this block so we
   // don't accidentally confuse them for tbsCertificate later.
   {
-    Reader input(der);
     Reader certificate;
-    rv = der::ExpectTagAndGetValue(input, der::SEQUENCE, certificate);
-    if (rv != Success) {
-      return rv;
-    }
-    rv = der::End(input);
+    rv = der::ExpectTagAndGetValueAtEnd(der, der::SEQUENCE, certificate);
     if (rv != Success) {
       return rv;
     }
     rv = der::SignedData(certificate, tbsCertificate, signedData);
     if (rv != Success) {
       return rv;
     }
     rv = der::End(certificate);
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -37,17 +37,17 @@
 // they are able to do so; otherwise they fail with the input mark in an
 // undefined state.
 
 #include "pkix/Input.h"
 #include "pkix/pkixtypes.h"
 
 namespace mozilla { namespace pkix { namespace der {
 
-enum Class
+enum Class : uint8_t
 {
    UNIVERSAL = 0 << 6,
 // APPLICATION = 1 << 6, // unused
    CONTEXT_SPECIFIC = 2 << 6,
 // PRIVATE = 3 << 6 // unused
 };
 
 enum Constructed
@@ -219,16 +219,40 @@ NestedOf(Reader& input, uint8_t outerTag
     if (rv != Success) {
       return rv;
     }
   } while (!inner.AtEnd());
 
   return Success;
 }
 
+// Often, a function will need to decode an Input or Reader that contains
+// DER-encoded data wrapped in a SEQUENCE (or similar) with nothing after it.
+// This function reduces the boilerplate necessary for stripping the outermost
+// SEQUENCE (or similar) and ensuring that nothing follows it.
+inline Result
+ExpectTagAndGetValueAtEnd(Reader& outer, uint8_t expectedTag,
+                          /*out*/ Reader& inner)
+{
+  Result rv = der::ExpectTagAndGetValue(outer, expectedTag, inner);
+  if (rv != Success) {
+    return rv;
+  }
+  return der::End(outer);
+}
+
+// Similar to the above, but takes an Input instead of a Reader&.
+inline Result
+ExpectTagAndGetValueAtEnd(Input outer, uint8_t expectedTag,
+                          /*out*/ Reader& inner)
+{
+  Reader outerReader(outer);
+  return ExpectTagAndGetValueAtEnd(outerReader, expectedTag, inner);
+}
+
 // Universal types
 
 namespace internal {
 
 // This parser will only parse values between 0..127. If this range is
 // increased then callers will need to be changed.
 template <typename T> inline Result
 IntegralValue(Reader& input, uint8_t tag, T& value)
--- a/security/pkix/lib/pkixnames.cpp
+++ b/security/pkix/lib/pkixnames.cpp
@@ -169,27 +169,20 @@ SearchForName(/*optional*/ const Input* 
   // contains a subjectAltName extension."
   //
   // TODO(bug XXXXXXX): Consider dropping support for IP addresses as
   // identifiers completely.
   bool hasAtLeastOneDNSNameOrIPAddressSAN = false;
 
   if (subjectAltName) {
     Reader altNames;
-
-    {
-      Reader input(*subjectAltName);
-      rv = der::ExpectTagAndGetValue(input, der::SEQUENCE, altNames);
-      if (rv != Success) {
-        return rv;
-      }
-      rv = der::End(input);
-      if (rv != Success) {
-        return rv;
-      }
+    rv = der::ExpectTagAndGetValueAtEnd(*subjectAltName, der::SEQUENCE,
+                                        altNames);
+    if (rv != Success) {
+      return rv;
     }
 
     // do { ... } while(...) because subjectAltName isn't allowed to be empty.
     do {
       uint8_t tag;
       Input presentedID;
       rv = der::ReadTagAndGetValue(altNames, tag, presentedID);
       if (rv != Success) {
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -404,33 +404,25 @@ BasicResponse(Reader& input, Context& co
   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
     Reader wrapped;
-    rv = der::ExpectTagAndGetValue(
+    rv = der::ExpectTagAndGetValueAtEnd(
           input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0, wrapped);
     if (rv != Success) {
       return rv;
     }
-    rv = der::End(input);
-    if (rv != Success) {
-      return rv;
-    }
 
     // SEQUENCE wrapper
     Reader certsSequence;
-    rv = der::ExpectTagAndGetValue(wrapped, der::SEQUENCE, certsSequence);
-    if (rv != Success) {
-      return rv;
-    }
-    rv = der::End(wrapped);
+    rv = der::ExpectTagAndGetValueAtEnd(wrapped, der::SEQUENCE, certsSequence);
     if (rv != Success) {
       return rv;
     }
 
     // sequence of certificates
     while (!certsSequence.AtEnd()) {
       Input cert;
       rv = der::ExpectTagAndGetTLV(certsSequence, der::SEQUENCE, cert);
@@ -782,31 +774,18 @@ KeyHash(TrustDomain& trustDomain, const 
 
   // RFC 5280 Section 4.1
   //
   // SubjectPublicKeyInfo  ::=  SEQUENCE  {
   //    algorithm            AlgorithmIdentifier,
   //    subjectPublicKey     BIT STRING  }
 
   Reader spki;
-  Result rv;
-
-  {
-    // The scope of input is limited to reduce the possibility of confusing it
-    // with spki in places we need to be using spki below.
-    Reader input(subjectPublicKeyInfo);
-    rv = der::ExpectTagAndGetValue(input, der::SEQUENCE, spki);
-    if (rv != Success) {
-      return rv;
-    }
-    rv = der::End(input);
-    if (rv != Success) {
-      return rv;
-    }
-  }
+  Result rv = der::ExpectTagAndGetValueAtEnd(subjectPublicKeyInfo,
+                                             der::SEQUENCE, spki);
 
   // Skip AlgorithmIdentifier
   rv = der::ExpectTagAndSkipValue(spki, der::SEQUENCE);
   if (rv != Success) {
     return rv;
   }
 
   Input subjectPublicKey;