Record UMA metrics for Must-Staple certificates on private roots

net/cert/cert_verify_proc.cc

Index: net/cert/cert_verify_proc.cc
diff --git a/net/cert/cert_verify_proc.cc b/net/cert/cert_verify_proc.cc
index 8fdd93cbb1bb0c4720273867ee9c9de3fb723ed6..0b6a761d4a58dbcec4eb0ad87fb7f92f3cdf0a74 100644
--- a/net/cert/cert_verify_proc.cc
+++ b/net/cert/cert_verify_proc.cc
@@ -18,6 +18,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/base/url_util.h"
+#include "net/cert/asn1_util.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/cert_verify_proc_whitelist.h"
@@ -309,6 +310,31 @@ void CheckOCSP(const std::string& raw_response,
   }
 }
 
+// Records histograms indicating whether the certificate |cert|, which
+// is assumed to have been validated chaining to a private root,
+// contains the TLS Feature Extension (https://tools.ietf.org/html/rfc7633) and
+// has valid OCSP information stapled.
+void RecordTLSFeatureExtensionWithPrivateRoot(
+    X509Certificate* cert,
+    const OCSPVerifyResult& ocsp_result) {
+  std::string cert_der;
+  if (!X509Certificate::GetDEREncoded(cert->os_cert_handle(), &cert_der))
+    return;
+
+  bool has_tls_feature_extension;
+  if (!asn1::HasTLSFeatureExtension(cert_der, &has_tls_feature_extension))

[eroman, 2016/10/21 01:49:23]

Should we be checking more specifically for the presence of status_request  (and
status_request_2) in the feature list?

The assumption here is that if it has a TLS feature extension, that extension
includes status_request.
Which might be a reasonable assumption to make. Can the feature list also be
empty? Or are there values other than status_request that might be in there?

[estark, 2016/10/21 02:11:29]

My understanding is that the extension is only used for status_request or
status_request2 in practice (and either of those are interesting for my
purposes), and would only appear when a Must-Staple certificate is explicitly
requested (and thus wouldn't be empty). The RFC is pretty OCSP-focused too. So I
think this is a reasonable assumption to make. I'll add a comment explaining it
though.

+    return;
+
+  UMA_HISTOGRAM_BOOLEAN("Net.Certificate.TLSFeatureExtensionWithPrivateRoot",
+                        has_tls_feature_extension);
+  if (!has_tls_feature_extension)
+    return;
+
+  UMA_HISTOGRAM_BOOLEAN(
+      "Net.Certificate.TLSFeatureExtensionWithPrivateRootHasOCSP",
+      (ocsp_result.response_status != OCSPVerifyResult::MISSING));
+}
+
 // Comparison functor used for binary searching whether a given HashValue,
 // which MUST be a SHA-256 hash, is contained with an array of SHA-256
 // hashes.
@@ -472,6 +498,11 @@ int CertVerifyProc::Verify(X509Certificate* cert,
       rv = MapCertStatusToNetError(verify_result->cert_status);
   }
 
+  // Record a histogram for the presence of the TLS feature extension in
+  // a certificate chaining to a private root.
+  if (rv == OK && !verify_result->is_issued_by_known_root)
+    RecordTLSFeatureExtensionWithPrivateRoot(cert, verify_result->ocsp_result);
+
   return rv;
 }