Revert "net: followup to codereview for cl/7096014"

This reverts commit r88331.

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@88333 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 22 insertions, 66 deletions

diff --git a/net/base/asn1_util.cc b/net/base/asn1_util.cc
index a1e8637..ee25d41 100644
--- a/net/base/asn1_util.cc
+++ b/net/base/asn1_util.cc
@@ -14,32 +14,11 @@ bool ParseElement(base::StringPiece* in,
                   unsigned *out_header_len) {
   const uint8* data = reinterpret_cast<const uint8*>(in->data());
 
-  // We don't support kAny and kOptional at the same time.
-  if ((tag_value & kAny) && (tag_value & kOptional))
-    return false;
-
-  if (in->empty() && (tag_value & kOptional)) {
-    if (out_header_len)
-      *out_header_len = 0;
-    if (out)
-      *out = base::StringPiece();
-    return true;
-  }
-
   if (in->size() < 2)
     return false;
 
-  if (tag_value != kAny &&
-      static_cast<unsigned char>(data[0]) != (tag_value & 0xff)) {
-    if (tag_value & kOptional) {
-      if (out_header_len)
-        *out_header_len = 0;
-      if (out)
-        *out = base::StringPiece();
-      return true;
-    }
+  if (tag_value != kAny && static_cast<unsigned char>(data[0]) != tag_value)
     return false;
-  }
 
   size_t len = 0;
   if ((data[1] & 0x80) == 0) {
@@ -92,9 +71,9 @@ bool GetElement(base::StringPiece* in,
   return true;
 }
 
-// SeekToSPKI changes |cert| so that it points to a suffix of the
-// TBSCertificate where the suffix begins at the start of the ASN.1
-// SubjectPublicKeyInfo value.
+// SeekToSPKI changes |cert| so that it points to a suffix of the original
+// value where the suffix begins at the start of the ASN.1 SubjectPublicKeyInfo
+// value.
 static bool SeekToSPKI(base::StringPiece* cert) {
   // From RFC 5280, section 4.1
   //    Certificate  ::=  SEQUENCE  {
@@ -115,19 +94,14 @@ static bool SeekToSPKI(base::StringPiece* cert) {
   if (!GetElement(cert, kSEQUENCE, &certificate))
     return false;
 
-  // We don't allow junk after the certificate.
-  if (!cert->empty())
-    return false;
-
   base::StringPiece tbs_certificate;
   if (!GetElement(&certificate, kSEQUENCE, &tbs_certificate))
     return false;
 
-  if (!GetElement(&tbs_certificate,
-                  kOptional | kConstructed | kContextSpecific | 0,
-                  NULL)) {
-    return false;
-  }
+  // The version is optional, so a failure to parse it is fine.
+  GetElement(&tbs_certificate,
+             kCompound | kContextSpecific | 0,
+             NULL);
 
   // serialNumber
   if (!GetElement(&tbs_certificate, kINTEGER, NULL))
@@ -160,7 +134,6 @@ bool ExtractSPKIFromDERCert(base::StringPiece cert,
 bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
                                std::vector<base::StringPiece>* urls_out) {
   urls_out->clear();
-  std::vector<base::StringPiece> tmp_urls_out;
 
   if (!SeekToSPKI(&cert))
     return false;
@@ -177,20 +150,16 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
   if (!GetElement(&cert, kSEQUENCE, NULL))
     return false;
   // issuerUniqueID
-  if (!GetElement(&cert, kOptional | kConstructed | kContextSpecific | 1, NULL))
-    return false;
+  GetElement(&cert, kCompound | kContextSpecific | 1, NULL);
   // subjectUniqueID
-  if (!GetElement(&cert, kOptional | kConstructed | kContextSpecific | 2, NULL))
-    return false;
+  GetElement(&cert, kCompound | kContextSpecific | 2, NULL);
 
   base::StringPiece extensions_seq;
-  if (!GetElement(&cert, kOptional | kConstructed | kContextSpecific | 3,
+  if (!GetElement(&cert, kCompound | kContextSpecific | 3,
                   &extensions_seq)) {
-    return false;
-  }
-
-  if (extensions_seq.empty())
+    // If there are no extensions, then there are no CRL URLs.
     return true;
+  }
 
   // Extensions  ::=  SEQUENCE SIZE (1..MAX) OF Extension
   // Extension   ::=  SEQUENCE  {
@@ -249,7 +218,7 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
         return false;
 
       base::StringPiece name;
-      if (!GetElement(&distrib_point, kContextSpecific | kConstructed | 0,
+      if (!GetElement(&distrib_point, kContextSpecific | kCompound | 0,
                       &name)) {
         // If it doesn't contain a name then we skip it.
         continue;
@@ -262,8 +231,7 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
         continue;
       }
 
-      if (GetElement(&distrib_point,
-                     kContextSpecific | kConstructed | 2, NULL)) {
+      if (GetElement(&distrib_point, kContextSpecific | kCompound | 2, NULL)) {
         // If it contains a alternative issuer, then we skip it.
         continue;
       }
@@ -272,10 +240,8 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
       //   fullName                [0]     GeneralNames,
       //   nameRelativeToCRLIssuer [1]     RelativeDistinguishedName }
       base::StringPiece general_names;
-      if (!GetElement(&name,
-                      kContextSpecific | kConstructed | 0, &general_names)) {
+      if (!GetElement(&name, kContextSpecific | kCompound | 0, &general_names))
         continue;
-      }
 
       // GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName
       // GeneralName ::= CHOICE {
@@ -285,7 +251,7 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
       while (general_names.size() > 0) {
         base::StringPiece url;
         if (GetElement(&general_names, kContextSpecific | 6, &url)) {
-          tmp_urls_out.push_back(url);
+          urls_out->push_back(url);
         } else {
           if (!GetElement(&general_names, kAny, NULL))
             return false;
@@ -294,7 +260,6 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
     }
   }
 
-  urls_out->swap(tmp_urls_out);
   return true;
 }
 
diff --git a/net/base/asn1_util.h b/net/base/asn1_util.h
index dc9982b..ea6277a 100644
--- a/net/base/asn1_util.h
+++ b/net/base/asn1_util.h
@@ -24,12 +24,10 @@ static const unsigned kSEQUENCE = 0x30;
 
 // These are flags that can be ORed with the above tag numbers.
 static const unsigned kContextSpecific = 0x80;
-static const unsigned kConstructed = 0x20;
+static const unsigned kCompound = 0x20;
 
 // kAny matches any tag value;
 static const unsigned kAny = 0x10000;
-// kOptional denotes an optional element.
-static const unsigned kOptional = 0x20000;
 
 // ParseElement parses a DER encoded ASN1 element from |in|, requiring that
 // it have the given |tag_value|. It returns true on success. The following
@@ -40,9 +38,6 @@ static const unsigned kOptional = 0x20000;
 //   |in| is advanced over the element
 //   |out| contains the element, including the tag and length bytes.
 //   |out_header_len| contains the length of the tag and length bytes in |out|.
-//
-// If |tag_value & kOptional| is true then *out_header_len can be zero after a
-// true return value if the element was not found.
 bool ParseElement(base::StringPiece* in,
                   unsigned tag_value,
                   base::StringPiece* out,
@@ -50,13 +45,11 @@ bool ParseElement(base::StringPiece* in,
 
 // GetElement performs the same actions as ParseElement, except that the header
 // bytes are not included in the output.
-//
-// If |tag_value & kOptional| is true then this function cannot distinguish
-// between a missing optional element and an empty one.
 bool GetElement(base::StringPiece* in,
                 unsigned tag_value,
                 base::StringPiece* out);
 
+
 // ExtractSPKIFromDERCert parses the DER encoded certificate in |cert| and
 // extracts the bytes of the SubjectPublicKeyInfo. On successful return,
 // |spki_out| is set to contain the SPKI, pointing into |cert|.
@@ -70,8 +63,6 @@ NET_TEST bool ExtractSPKIFromDERCert(base::StringPiece cert,
 // CRLs that only cover a subset of the reasons are omitted as the spec
 // requires that at least one CRL be included that covers all reasons.
 //
-// CRLs that use an alternative issuer are also omitted.
-//
 // The nested set of GeneralNames is flattened into a single list because
 // having several CRLs with one location is equivalent to having one CRL with
 // several locations as far as a CRL filter is concerned.
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 702db57..cb2e9f0 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -588,10 +588,10 @@ TEST(X509CertificateTest, ExtractCRLURLsFromDERCert) {
   std::vector<base::StringPiece> crl_urls;
   EXPECT_TRUE(asn1::ExtractCRLURLsFromDERCert(derBytes, &crl_urls));
 
-  EXPECT_EQ(1u, crl_urls.size());
+  EXPECT_EQ(crl_urls.size(), 1u);
   if (crl_urls.size() > 0) {
-    EXPECT_EQ("http://SVRSecure-G3-crl.verisign.com/SVRSecureG3.crl",
-              crl_urls[0].as_string());
+    EXPECT_EQ(crl_urls[0].as_string(),
+              "http://SVRSecure-G3-crl.verisign.com/SVRSecureG3.crl");
   }
 }