Add OCSPVerifyResult for tracking stapled OCSP responses cross-platform.

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 49a170d3cb55b929e0659e6757b6f7089c3086ad..3c3b6065c0f01e398f857e108e4a761eb53e7de9 100644
--- a/net/cert/cert_verify_proc.cc
+++ b/net/cert/cert_verify_proc.cc
@@ -23,7 +23,10 @@
 #include "net/cert/cert_verify_proc_whitelist.h"
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/crl_set.h"
+#include "net/cert/internal/parse_ocsp.h"
+#include "net/cert/ocsp_revocation_status.h"
 #include "net/cert/x509_certificate.h"
+#include "net/der/encode_values.h"
 #include "url/url_canon.h"
 
 #if defined(USE_NSS_CERTS)
@@ -182,6 +185,91 @@ bool IsPastSHA1DeprecationDate(const X509Certificate& cert) {
   return start >= kSHA1DeprecationDate;
 }
 
+bool CheckCertIDMatchesCertificate(const OCSPCertID& cert_id,

[Ryan Sleevi, 2016/07/18 20:08:08]

Document

[dadrian, 2016/07/18 22:23:32]

Done.

+                                   const X509Certificate& certificate) {
+  // TODO(dadrian): Verify name and key hashes. https://crbug.com/620005
+  der::Input serial(&certificate.serial_number());
+  return cert_id.serial_number == serial;
+}
+
+void CheckOCSP(const std::string& raw_response,

[Ryan Sleevi, 2016/07/18 20:08:08]

Document

[dadrian, 2016/07/18 22:23:32]

Done.

+               CertVerifyResult* verify_result) {

[Ryan Sleevi, 2016/07/18 20:08:08]

Is CertVerifyResult really the right dependency? It looks like you're using it
to smuggle two parameters - the output OCSP response, and the cert under
inspection.

I'd suggest making those inputs explicit (that is, rather than taking the
CertVerifyResult directly)

[dadrian, 2016/07/18 22:23:31]

Done.

+  verify_result->ocsp.Reset();

[Ryan Sleevi, 2016/07/18 20:08:08]

Do we need an explicit reset? Can't you just do a by-val assignment? Is this
just cargo-culted from CertVerifyResult?

[dadrian, 2016/07/18 22:23:32]

Done.

+
+  if (raw_response.empty()) {
+    verify_result->ocsp.response_status = OCSPVerifyResult::MISSING;
+    return;
+  }
+  der::Input response_der(&raw_response);

[Ryan Sleevi, 2016/07/18 20:08:08]

move the newline from 204 to 203 :)

[dadrian, 2016/07/18 22:23:31]

Done.

+
+  OCSPResponse response;
+  if (!ParseOCSPResponse(response_der, &response)) {
+    verify_result->ocsp.response_status = OCSPVerifyResult::PARSE_RESPONSE;

[Ryan Sleevi, 2016/07/18 20:08:07]

Is PARSE_RESPONSE a good name for the enum? How does it distinguish from
BAD_RESPONSE?

Should it be PARSE_ERROR?

[dadrian, 2016/07/18 22:23:32]

I changed it to PARSE_RESPONSE_ERROR and PARSE_RESPONSE_DATA_ERROR, to
distinguish the two cases, while still making it clear it's a parsing error.

+    return;
+  }
+
+  // If the OCSP response isn't status SUCCESSFUL, don't parse the rest of the
+  // data.

[Ryan Sleevi, 2016/07/18 20:08:07]

Could you explain the "why" more? Why is this a 'bad' response?

[dadrian, 2016/07/18 22:23:32]

Done.

+  if (response.status != OCSPResponse::ResponseStatus::SUCCESSFUL) {
+    verify_result->ocsp.response_status = OCSPVerifyResult::BAD_RESPONSE;
+    return;
+  }
+
+  OCSPResponseData response_data;
+  if (!ParseOCSPResponseData(response.data, &response_data)) {

[Ryan Sleevi, 2016/07/18 20:08:07]

This could benefit from documentation, because ParseOCSPResponse and
ParseOCSPResponseData aren't immediately clear about the distinction between
these two. For example, citing spec text (as you can see in the other RFC 5280
validation code) is useful to help refresh readers' context about what's
happening.

[dadrian, 2016/07/18 22:23:32]

Done.

+    verify_result->ocsp.response_status = OCSPVerifyResult::PARSE_RESPONSE_DATA;
+    return;
+  }
+
+  // If producedAt is outside of the certificate validity period, reject the
+  // response.
+  der::GeneralizedTime not_before, not_after;
+  if (!der::EncodeTimeAsGeneralizedTime(
+          verify_result->verified_cert->valid_start(), &not_before) ||
+      !der::EncodeTimeAsGeneralizedTime(
+          verify_result->verified_cert->valid_expiry(), &not_after)) {

[Ryan Sleevi, 2016/07/18 20:08:08]

Just to make sure I understand: We parse the certificate validity, and then
re-encode the certificate validity?

[dadrian, 2016/07/18 22:23:32]

Aren't computers great? :) 

The other option would be to implement der::EncodeGeneralizedTimeToTime(), but
based on previous experience, this would literally take months.

+    verify_result->ocsp.response_status = OCSPVerifyResult::BAD_PRODUCED_AT;
+    return;
+  }
+  if (response_data.produced_at < not_before ||
+      response_data.produced_at > not_after) {
+    verify_result->ocsp.response_status = OCSPVerifyResult::BAD_PRODUCED_AT;
+    return;
+  }
+
+  // TODO(svaldez): Unify with GetOCSPCertStatus.

[Ryan Sleevi, 2016/07/18 20:08:08]

Is there a Bug #?

[dadrian, 2016/07/18 22:23:32]

There is now. https://crbug.com/629249

+  base::Time verify_time = base::Time::Now();
+  base::TimeDelta max_age = base::TimeDelta::FromDays(7);

[Ryan Sleevi, 2016/07/18 20:08:08]

Should this be a function-level static constant so it's clearly documented the
magic age?

[dadrian, 2016/07/18 22:23:31]

Sure? Not sure what the rules are here, but this is buried pretty deep. I'll try
to draw it out.

+  verify_result->ocsp.response_status = OCSPVerifyResult::NO_MATCHING_RESPONSE;
+  for (const auto& single_response_der : response_data.responses) {

[Ryan Sleevi, 2016/07/18 20:08:07]

You could do with some high-level documentation about the logic for matching,
and why it's incomplete here.

For example

for (const auto& single_response_der : response_data.responses) {
  // Although in most cases there should only be a single response (for the
certificate requested), it's
  // possible the OCSP responder provided statuses for multiple certificates.
Look through all the
  // responses to see if there's a response that matches the
|verify_result->verified_cert|'s certificate.
  // This matching is ...

}

etc

[dadrian, 2016/07/18 22:23:32]

Done.

+    OCSPSingleResponse single_response;
+    if (!ParseOCSPSingleResponse(single_response_der, &single_response))
+      continue;
+    OCSPCertID cert_id;
+    if (!ParseOCSPCertID(single_response.cert_id_tlv, &cert_id))
+      continue;
+    if (!CheckCertIDMatchesCertificate(cert_id, *verify_result->verified_cert))
+      continue;
+    if (!CheckOCSPDateValid(single_response, verify_time, max_age)) {
+      if (verify_result->ocsp.response_status != OCSPVerifyResult::PROVIDED)

[Ryan Sleevi, 2016/07/18 20:08:07]

This could benefit from documentation

[dadrian, 2016/07/18 22:23:32]

Done.

+        verify_result->ocsp.response_status = OCSPVerifyResult::INVALID_DATE;
+      continue;
+    }
+    verify_result->ocsp.response_status = OCSPVerifyResult::PROVIDED;
+
+    OCSPRevocationStatus current_status =
+        verify_result->ocsp.revocation_status.value_or(
+            OCSPRevocationStatus::GOOD);
+    // In the case that we receive multiple responses, we keep only the

[Ryan Sleevi, 2016/07/18 20:08:08]

Avoid "we" in comments, especially new code.

// In the event multiple responses are received, keep the strictest status
(REVOKED > UNKNOWN > GOOD)

[dadrian, 2016/07/18 22:23:32]

Done.

+    // strictest status (REVOKED > UNKNOWN > GOOD).
+    if (current_status == OCSPRevocationStatus::GOOD ||
+        single_response.cert_status.status == OCSPRevocationStatus::REVOKED) {
+      verify_result->ocsp.revocation_status =
+          single_response.cert_status.status;
+    }
+  }
+}
+
 // 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.
@@ -258,6 +346,8 @@ int CertVerifyProc::Verify(X509Certificate* cert,
                           verify_result->common_name_fallback_used);
   }
 
+  CheckOCSP(ocsp_response, verify_result);
+
   // This check is done after VerifyInternal so that VerifyInternal can fill
   // in the list of public key hashes.
   if (IsPublicKeyBlacklisted(verify_result->public_key_hashes)) {