attestation: Fix policy observer expiry check.

chrome/browser/chromeos/attestation/attestation_policy_observer.cc

Index: chrome/browser/chromeos/attestation/attestation_policy_observer.cc
diff --git a/chrome/browser/chromeos/attestation/attestation_policy_observer.cc b/chrome/browser/chromeos/attestation/attestation_policy_observer.cc
index 371c0be964b756751f10fc51e160c6125097f3bd..804edaebe768c6dd32153b185162c3d3999ab012 100644
--- a/chrome/browser/chromeos/attestation/attestation_policy_observer.cc
+++ b/chrome/browser/chromeos/attestation/attestation_policy_observer.cc
@@ -23,6 +23,7 @@
 #include "components/policy/core/common/cloud/cloud_policy_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
+#include "net/cert/pem_tokenizer.h"
 #include "net/cert/x509_certificate.h"
 
 namespace {
@@ -211,12 +212,24 @@ void AttestationPolicyObserver::GetExistingCertificate() {
 
 void AttestationPolicyObserver::CheckCertificateExpiry(
     const std::string& certificate) {
-  scoped_refptr<net::X509Certificate> x509(
-      net::X509Certificate::CreateFromBytes(certificate.data(),
-                                            certificate.length()));
-  if (!x509.get() || x509->valid_expiry().is_null()) {
-    LOG(WARNING) << "Failed to parse certificate, cannot check expiry.";
-  } else {
+  int num_certificates = 0;
+  net::PEMTokenizer pem_tokenizer(certificate, {"CERTIFICATE"});
+  while (pem_tokenizer.GetNext()) {
+    ++num_certificates;
+    scoped_refptr<net::X509Certificate> x509 =
+        net::X509Certificate::CreateFromBytes(pem_tokenizer.data().data(),
+                                              pem_tokenizer.data().length());
+    if (!x509.get() || x509->valid_expiry().is_null()) {
+      // This logic intentionally fails open. In theory this should not happen
+      // but in practice parsing X.509 can be brittle and there are a lot of
+      // factors including which underlying module is parsing the certificate,
+      // whether that module performs more checks than just ASN.1/DER format,
+      // and the server module that generated the certificate(s). Renewal is
+      // expensive so we only renew certificates with good evidence that they
+      // have expired or will soon expire; if we don't know, we don't renew.
+      LOG(WARNING) << "Failed to parse certificate, cannot check expiry.";
+      continue;
+    }
     const base::TimeDelta threshold =
         base::TimeDelta::FromDays(kExpiryThresholdInDays);
     if ((base::Time::Now() + threshold) > x509->valid_expiry()) {
@@ -225,7 +238,9 @@ void AttestationPolicyObserver::CheckCertificateExpiry(
       return;
     }
   }
-
+  if (num_certificates == 0) {
+    LOG(WARNING) << "Failed to parse certificate chain, cannot check expiry.";
+  }
   // Get the payload and check if the certificate has already been uploaded.
   GetKeyPayload(base::Bind(&AttestationPolicyObserver::CheckIfUploaded,
                            weak_factory_.GetWeakPtr(),