Add CertErrors* parameter to the main Certificate parsing functions.

components/cast_certificate/cast_crl.cc

 // Copyright 2016 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 "components/cast_certificate/cast_crl.h"
 
 #include <unordered_map>
 #include <unordered_set>
 
 #include "base/base64.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
 #include "components/cast_certificate/proto/revocation.pb.h"
 #include "crypto/sha2.h"
+#include "net/cert/internal/cert_errors.h"
 #include "net/cert/internal/parse_certificate.h"
 #include "net/cert/internal/parsed_certificate.h"
 #include "net/cert/internal/path_builder.h"
 #include "net/cert/internal/signature_algorithm.h"
 #include "net/cert/internal/signature_policy.h"
 #include "net/cert/internal/trust_store_in_memory.h"
 #include "net/cert/internal/verify_certificate_chain.h"
 #include "net/cert/internal/verify_signed_data.h"
 #include "net/cert/x509_certificate.h"
 #include "net/der/encode_values.h"
 #include "net/der/input.h"
+#include "net/der/parse_values.h"
 #include "net/der/parser.h"
-#include "net/der/parse_values.h"
 
 namespace cast_certificate {
 namespace {
 
 enum CrlVersion {
   // version 0: Spki Hash Algorithm = SHA-256
   //            Signature Algorithm = RSA-PKCS1 V1.5 with SHA-256
   CRL_VERSION_0 = 0,
 };
 
@@ skipping 17 matching lines @@
                                                   CastCRLTrustStore>>::get();
   }
 
   static net::TrustStore& Get() { return GetInstance()->store_; }
 
  private:
   friend struct base::DefaultSingletonTraits<CastCRLTrustStore>;
 
   CastCRLTrustStore() {
     // Initialize the trust store with the root certificate.
+    net::CertErrors errors;
     scoped_refptr<net::ParsedCertificate> cert =
-        net::ParsedCertificate::CreateFromCertificateData(
-            kCastCRLRootCaDer, sizeof(kCastCRLRootCaDer),
-            net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE, {});
-    CHECK(cert);
+        net::ParsedCertificate::CreateWithoutCopyingUnsafe(
+            kCastCRLRootCaDer, sizeof(kCastCRLRootCaDer), {}, &errors);
+    CHECK(cert) << errors.ToDebugString();
     // Enforce pathlen constraints and policies defined on the root certificate.
     scoped_refptr<net::TrustAnchor> anchor =
         net::TrustAnchor::CreateFromCertificateWithConstraints(std::move(cert));
     CHECK(anchor);
     store_.AddTrustAnchor(std::move(anchor));
   }
 
   net::TrustStoreInMemory store_;
   DISALLOW_COPY_AND_ASSIGN(CastCRLTrustStore);
 };
@@ skipping 20 matching lines @@
 // certificate and signature in |crl|. The validity of |tbs_crl| is verified
 // at |time|. The validity period of the CRL is adjusted to be the earliest
 // of the issuer certificate chain's expiration and the CRL's expiration and
 // the result is stored in |overall_not_after|.
 bool VerifyCRL(const Crl& crl,
                const TbsCrl& tbs_crl,
                const base::Time& time,
                net::TrustStore* trust_store,
                net::der::GeneralizedTime* overall_not_after) {
   // Verify the trust of the CRL authority.
+  net::CertErrors errors;
   scoped_refptr<net::ParsedCertificate> parsed_cert =
-      net::ParsedCertificate::CreateFromCertificateData(
-          reinterpret_cast<const uint8_t*>(crl.signer_cert().data()),
-          crl.signer_cert().size(),
-          net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE, {});
+      net::ParsedCertificate::Create(crl.signer_cert().data(), {}, &errors);
   if (parsed_cert == nullptr) {
-    VLOG(2) << "CRL - Issuer certificate parsing failed.";
+    VLOG(2) << "CRL - Issuer certificate parsing failed:\n"
+            << errors.ToDebugString();
     return false;
   }
 
   // Wrap the signature in a BitString.
   net::der::BitString signature_value_bit_string = net::der::BitString(
       net::der::Input(base::StringPiece(crl.signature())), 0);
 
   // Verify the signature.
   auto signature_policy = CreateCastSignaturePolicy();
   std::unique_ptr<net::SignatureAlgorithm> signature_algorithm_type =
       net::SignatureAlgorithm::CreateRsaPkcs1(net::DigestAlgorithm::Sha256);
-  net::CertErrors errors;
   if (!VerifySignedData(*signature_algorithm_type,
                         net::der::Input(&crl.tbs_crl()),
                         signature_value_bit_string, parsed_cert->tbs().spki_tlv,
                         signature_policy.get(), &errors)) {

[mattm, 2016/09/12 23:29:27]

Should it use a separate errors object just in case there is anything left over
from the ParsedCertificate call?

[eroman, 2016/09/12 23:42:59]

Good question.

I debated this a bit when writing, and my thinking was:
 (a) If there was an error, the first call would return
 (b) If there was a warning from the parsing, and then VerifySignedData failed,
that warning would leak through here.

In case (b) it isn't necessarily harmful to show that warning here, maybe it is
even relevant. (Logging the warnings each time might be overkill though)

WDYT if I add a ".Clear()" method on CertErrors?
Having two different named "errors" seems a bit weird, so I could either use
.Clear() or use a short lived instance for each call.

[mattm, 2016/09/13 22:27:38]

I worry a bit about a Clear() method since it seems it could get weird if you
used it in the presence of scopers. I guess you could add rules about when it's
allowed to be called, but seems easier just to not have it.

I suppose there is little chance that parsing and verifying would use the same
error ids, so it wouldn't be overly confusing if it did happen. So I guess just
leaving it could be okay too.

[eroman, 2016/09/13 22:47:30]

I went ahead and created 2 different error objects: |parse_errors| and
|verify_errors| to remove the ambiguity.

(I decided against enclosing them in smaller scopes since I found it hurt
readability.)

I'll see what the Cast reviewer thinks.

     // TODO(634443): Dump the error information.
-    VLOG(2) << "CRL - Signature verification failed.";
+    VLOG(2) << "CRL - Signature verification failed:\n"
+            << errors.ToDebugString();
     return false;
   }
 
   // Verify the issuer certificate.
   net::der::GeneralizedTime verification_time;
   if (!net::der::EncodeTimeAsGeneralizedTime(time, &verification_time)) {
     VLOG(2) << "CRL - Unable to parse verification time.";
     return false;
   }
   net::CertPathBuilder::Result result;
@@ skipping 209 matching lines @@
 }
 
 std::unique_ptr<CastCRL> ParseAndVerifyCRLForTest(
     const std::string& crl_proto,
     const base::Time& time,
     net::TrustStore* trust_store) {
   return ParseAndVerifyCRL(crl_proto, time, trust_store);
 }
 
 }  // namespace cast_certificate