Fix PEM parsing on Linux when using X509Certificate::FORMAT_AUTO

When using X509Certificate::FORMAT_AUTO, parsing is attempted optimistically first by seeing if the data decodes as any of the supported binary formats. The NSS routine used to handle PKCS#7 data is actually a generic routine, capable of supporting multiple formats.

When a PEM encoded certificate is decoded using PKCS#7, as is the case with FORMAT_AUTO, NSS will, upon encountering a failure to parse as PKCS#7, attempt to PEM decode the data and parse as either a certificate or a PKCS#7 structure. The problem with NSS's implementation is that it only decodes a single certificate, rather than all of the certificates in the file, preventing a full PEM chain from being read in.

The solution is to no longer optimistically try to decode the data as binary prior to trying to decode as PEM, and instead unconditionally attempt decoding as PEM-wrapped data before falling back to binary. This allows our handling routines to properly parse all of the supported PEM types, before the underlying crypto library is exposed to the data.

BUG=37142
TEST=X509CertificateParseTest.CanParseFormat

Review URL: http://codereview.chromium.org/3148034

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

2 files changed, 62 insertions, 50 deletions

diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 9c69b4a..df62ec8 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -204,8 +204,56 @@ CertificateList X509Certificate::CreateCertificateListFromBytes(
     const char* data, int length, int format) {
   OSCertHandles certificates;
 
+  // Check to see if it is in a PEM-encoded form. This check is performed
+  // first, as both OS X and NSS will both try to convert if they detect
+  // PEM encoding, except they don't do it consistently between the two.
+  base::StringPiece data_string(data, length);
+  std::vector<std::string> pem_headers;
+
+  // To maintain compatibility with NSS/Firefox, CERTIFICATE is a universally
+  // valid PEM block header for any format.
+  pem_headers.push_back(kCertificateHeader);
+  if (format & FORMAT_PKCS7)
+    pem_headers.push_back(kPKCS7Header);
+
+  PEMTokenizer pem_tok(data_string, pem_headers);
+  while (pem_tok.GetNext()) {
+    std::string decoded(pem_tok.data());
+
+    OSCertHandle handle = NULL;
+    if (format & FORMAT_PEM_CERT_SEQUENCE)
+      handle = CreateOSCertHandleFromBytes(decoded.c_str(), decoded.size());
+    if (handle != NULL) {
+      // Parsed a DER encoded certificate. All PEM blocks that follow must
+      // also be DER encoded certificates wrapped inside of PEM blocks.
+      format = FORMAT_PEM_CERT_SEQUENCE;
+      certificates.push_back(handle);
+      continue;
+    }
+
+    // If the first block failed to parse as a DER certificate, and
+    // formats other than PEM are acceptable, check to see if the decoded
+    // data is one of the accepted formats.
+    if (format & ~FORMAT_PEM_CERT_SEQUENCE) {
+      for (size_t i = 0; certificates.empty() &&
+           i < arraysize(kFormatDecodePriority); ++i) {
+        if (format & kFormatDecodePriority[i]) {
+          certificates = CreateOSCertHandlesFromBytes(decoded.c_str(),
+              decoded.size(), kFormatDecodePriority[i]);
+        }
+      }
+    }
+
+    // Stop parsing after the first block for any format but a sequence of
+    // PEM-encoded DER certificates. The case of FORMAT_PEM_CERT_SEQUENCE
+    // is handled above, and continues processing until a certificate fails
+    // to parse.
+    break;
+  }
+
   // Try each of the formats, in order of parse preference, to see if |data|
-  // contains the binary representation of a Format.
+  // contains the binary representation of a Format, if it failed to parse
+  // as a PEM certificate/chain.
   for (size_t i = 0; certificates.empty() &&
        i < arraysize(kFormatDecodePriority); ++i) {
     if (format & kFormatDecodePriority[i])
@@ -213,53 +261,6 @@ CertificateList X509Certificate::CreateCertificateListFromBytes(
                                                   kFormatDecodePriority[i]);
   }
 
-  // No certs were read. Check to see if it is in a PEM-encoded form.
-  if (certificates.empty()) {
-    base::StringPiece data_string(data, length);
-    std::vector<std::string> pem_headers;
-
-    // To maintain compatibility with NSS/Firefox, CERTIFICATE is a universally
-    // valid PEM block header for any format.
-    pem_headers.push_back(kCertificateHeader);
-    if (format & FORMAT_PKCS7)
-      pem_headers.push_back(kPKCS7Header);
-
-    PEMTokenizer pem_tok(data_string, pem_headers);
-    while (pem_tok.GetNext()) {
-      std::string decoded(pem_tok.data());
-
-      OSCertHandle handle = NULL;
-      if (format & FORMAT_PEM_CERT_SEQUENCE)
-        handle = CreateOSCertHandleFromBytes(decoded.c_str(), decoded.size());
-      if (handle != NULL) {
-        // Parsed a DER encoded certificate. All PEM blocks that follow must
-        // also be DER encoded certificates wrapped inside of PEM blocks.
-        format = FORMAT_PEM_CERT_SEQUENCE;
-        certificates.push_back(handle);
-        continue;
-      }
-
-      // If the first block failed to parse as a DER certificate, and
-      // formats other than PEM are acceptable, check to see if the decoded
-      // data is one of the accepted formats.
-      if (format & ~FORMAT_PEM_CERT_SEQUENCE) {
-        for (size_t i = 0; certificates.empty() &&
-             i < arraysize(kFormatDecodePriority); ++i) {
-          if (format & kFormatDecodePriority[i]) {
-            certificates = CreateOSCertHandlesFromBytes(decoded.c_str(),
-                decoded.size(), kFormatDecodePriority[i]);
-          }
-        }
-      }
-
-      // Stop parsing after the first block for any format but a sequence of
-      // PEM-encoded DER certificates. The case of FORMAT_PEM_CERT_SEQUENCE
-      // is handled above, and continues processing until a certificate fails
-      // to parse.
-      break;
-    }
-  }
-
   CertificateList results;
   // No certificates parsed.
   if (certificates.empty())
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index ad0b5c6..e1319d7 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -703,8 +703,19 @@ TEST_P(X509CertificateParseTest, CanParseFormat) {
                   kGoogleParseValidFrom, kGoogleParseValidTo);
 
   size_t i;
-  for (i = 0; i < arraysize(test_data_.chain_fingerprints) &&
-       i < certs.size() && test_data_.chain_fingerprints[i] != NULL; ++i) {
+  for (i = 0; i < arraysize(test_data_.chain_fingerprints); ++i) {
+    if (test_data_.chain_fingerprints[i] == NULL) {
+      // No more test certificates expected - make sure no more were
+      // returned before marking this test a success.
+      EXPECT_EQ(i, certs.size());
+      break;
+    }
+
+    // A cert is expected - make sure that one was parsed.
+    ASSERT_LT(i, certs.size());
+
+    // Compare the parsed certificate with the expected certificate, by
+    // comparing fingerprints.
     const X509Certificate* cert = certs[i];
     const SHA1Fingerprint& actual_fingerprint = cert->fingerprint();
     unsigned char* expected_fingerprint = test_data_.chain_fingerprints[i];