net: followup to codereview for cl/7096014

net/base/asn1_util.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/asn1_util.h"
 
 namespace net {
 
 namespace asn1 {
 
 bool ParseElement(base::StringPiece* in,
                   unsigned tag_value,
                   base::StringPiece* out,
                   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)
+  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;
+    }
     return false;
+  }
 
   size_t len = 0;
   if ((data[1] & 0x80) == 0) {
     // short form length
     if (out_header_len)
       *out_header_len = 2;
     len = static_cast<size_t>(data[1]) + 2;
   } else {
     // long form length
     const unsigned num_bytes = data[1] & 0x7f;
@@ skipping 32 matching lines @@
                 unsigned tag_value,
                 base::StringPiece* out) {
   unsigned header_len;
   if (!ParseElement(in, tag_value, out, &header_len))
     return false;
   if (out)
     out->remove_prefix(header_len);
   return true;
 }
 
-// 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.
+// 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.
 static bool SeekToSPKI(base::StringPiece* cert) {
   // From RFC 5280, section 4.1
   //    Certificate  ::=  SEQUENCE  {
   //      tbsCertificate       TBSCertificate,
   //      signatureAlgorithm   AlgorithmIdentifier,
   //      signatureValue       BIT STRING  }
 
   // TBSCertificate  ::=  SEQUENCE  {
   //      version         [0]  EXPLICIT Version DEFAULT v1,
   //      serialNumber         CertificateSerialNumber,
   //      signature            AlgorithmIdentifier,
   //      issuer               Name,
   //      validity             Validity,
   //      subject              Name,
   //      subjectPublicKeyInfo SubjectPublicKeyInfo,
 
   base::StringPiece certificate;
   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;
 
-  // The version is optional, so a failure to parse it is fine.
-  GetElement(&tbs_certificate,
-             kCompound | kContextSpecific | 0,
-             NULL);
+  if (!GetElement(&tbs_certificate,
+                  kOptional | kConstructed | kContextSpecific | 0,
+                  NULL)) {
+    return false;
+  }
 
   // serialNumber
   if (!GetElement(&tbs_certificate, kINTEGER, NULL))
     return false;
   // signature
   if (!GetElement(&tbs_certificate, kSEQUENCE, NULL))
     return false;
   // issuer
   if (!GetElement(&tbs_certificate, kSEQUENCE, NULL))
     return false;
@@ skipping 28 matching lines @@
   //      ...
   //      subjectPublicKeyInfo SubjectPublicKeyInfo,
   //      issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL,
   //      subjectUniqueID [2]  IMPLICIT UniqueIdentifier OPTIONAL,
   //      extensions      [3]  EXPLICIT Extensions OPTIONAL
 
   // subjectPublicKeyInfo
   if (!GetElement(&cert, kSEQUENCE, NULL))
     return false;
   // issuerUniqueID
-  GetElement(&cert, kCompound | kContextSpecific | 1, NULL);
+  if (!GetElement(&cert, kOptional | kConstructed | kContextSpecific | 1, NULL))
+    return false;
   // subjectUniqueID
-  GetElement(&cert, kCompound | kContextSpecific | 2, NULL);
+  if (!GetElement(&cert, kOptional | kConstructed | kContextSpecific | 2, NULL))
+    return false;
 
   base::StringPiece extensions_seq;
-  if (!GetElement(&cert, kCompound | kContextSpecific | 3,
+  if (!GetElement(&cert, kOptional | kConstructed | kContextSpecific | 3,
                   &extensions_seq)) {
-    // If there are no extensions, then there are no CRL URLs.
+    return false;
+  }
+
+  if (extensions_seq.empty())
     return true;
-  }
 
   // Extensions  ::=  SEQUENCE SIZE (1..MAX) OF Extension
   // Extension   ::=  SEQUENCE  {
   //      extnID      OBJECT IDENTIFIER,
   //      critical    BOOLEAN DEFAULT FALSE,
   //      extnValue   OCTET STRING
 
   // |extensions_seq| was EXPLICITly tagged, so we still need to remove the
   // ASN.1 SEQUENCE header.
   base::StringPiece extensions;
@@ skipping 38 matching lines @@
     base::StringPiece distribution_points;
     if (!GetElement(&extension_value, kSEQUENCE, &distribution_points))
       return false;
 
     while (distribution_points.size() > 0) {
       base::StringPiece distrib_point;
       if (!GetElement(&distribution_points, kSEQUENCE, &distrib_point))
         return false;
 
       base::StringPiece name;
-      if (!GetElement(&distrib_point, kContextSpecific | kCompound | 0,
+      if (!GetElement(&distrib_point, kContextSpecific | kConstructed | 0,
                       &name)) {
         // If it doesn't contain a name then we skip it.
         continue;
       }
 
       if (GetElement(&distrib_point, kContextSpecific | 1, NULL)) {
         // If it contains a subset of reasons then we skip it. We aren't
         // interested in subsets of CRLs and the RFC states that there MUST be
         // a CRL that covers all reasons.
         continue;
       }
 
-      if (GetElement(&distrib_point, kContextSpecific | kCompound | 2, NULL)) {
+      if (GetElement(&distrib_point,
+                     kContextSpecific | kConstructed | 2, NULL)) {
         // If it contains a alternative issuer, then we skip it.
         continue;
       }
 
       // DistributionPointName ::= CHOICE {
       //   fullName                [0]     GeneralNames,
       //   nameRelativeToCRLIssuer [1]     RelativeDistinguishedName }
       base::StringPiece general_names;
-      if (!GetElement(&name, kContextSpecific | kCompound | 0, &general_names))
+      if (!GetElement(&name,
+                      kContextSpecific | kConstructed | 0, &general_names)) {
         continue;
+      }
 
       // GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName
       // GeneralName ::= CHOICE {
       //   ...
       //   uniformResourceIdentifier [6]  IA5String,
       //   ...
       while (general_names.size() > 0) {
         base::StringPiece url;
         if (GetElement(&general_names, kContextSpecific | 6, &url)) {
           urls_out->push_back(url);
         } else {
-          if (!GetElement(&general_names, kAny, NULL))
+          if (!GetElement(&general_names, kAny, NULL)) {
+            urls_out->clear();
             return false;

[wtc, 2011/06/07 18:17:54]

I think some other "return false" statements also need a
urls_out->clear() call.

Rather than adding a lot of urls_out->clear() calls, perhaps
a more systematic solution would be better.

+          }
         }
       }
     }
   }
 
   return true;
 }
 
 } // namespace asn1
 
 } // namespace net