Certificate Transparency: Threading the CT verifier into the SSL client socket.

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 89eab143140f5aadadd7e5cf8678d9692499f8a8..7cbdb0efa100b6c15162ed4b7f91b6236acec297 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -93,6 +93,7 @@
 #include "net/cert/asn1_util.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/cert_verifier.h"
+#include "net/cert/ct_verifier.h"
 #include "net/cert/scoped_nss_types.h"
 #include "net/cert/single_request_cert_verifier.h"
 #include "net/cert/x509_certificate_net_log_param.h"
@@ -2769,6 +2770,7 @@ SSLClientSocketNSS::SSLClientSocketNSS(
       nss_fd_(NULL),
       net_log_(transport_->socket()->NetLog()),
       transport_security_state_(context.transport_security_state),
+      cert_transparency_verifier_(context.cert_transparency_verifier),
       valid_thread_id_(base::kInvalidThreadId) {
   EnterFunction("");
   InitCore();
@@ -3289,6 +3291,12 @@ int SSLClientSocketNSS::DoHandshakeLoop(int last_io_result) {
       case STATE_VERIFY_CERT_COMPLETE:
         rv = DoVerifyCertComplete(rv);
         break;
+      case STATE_VERIFY_CT:
+        rv = DoVerifyCT(rv);
+        break;
+      case STATE_VERIFY_CT_COMPLETE:
+        rv = DoVerifyCTComplete(rv);
+        break;
       case STATE_NONE:
       default:
         rv = ERR_UNEXPECTED;
@@ -3325,7 +3333,6 @@ int SSLClientSocketNSS::DoHandshakeComplete(int result) {
   return result;
 }
 
-
 int SSLClientSocketNSS::DoVerifyCert(int result) {
   DCHECK(!core_->state().server_cert_chain.empty());
   DCHECK(core_->state().server_cert_chain[0]);
@@ -3409,8 +3416,6 @@ int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
   if (result == OK)
     LogConnectionTypeMetrics();
 
-  completed_handshake_ = true;
-
 #if defined(OFFICIAL_BUILD) && !defined(OS_ANDROID) && !defined(OS_IOS)
   // Take care of any mandates for public key pinning.
   //
@@ -3458,11 +3463,76 @@ int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
   }
 #endif
 
+  if (result == OK) {
+    // Only check Certificate Transparency if there were no other errors with
+    // the connection. Note that in this case DoVerifyCTComplete will set
+    // completed_handshake_ to true.
+    GotoState(STATE_VERIFY_CT);

[wtc, 2013/11/26 01:47:23]

DESIGN: A state is necessary only if we need to wait for an async operation to
complete.

Since cert_transparency_verifier_->Verify() works synchronously, we don't need
new states for it. You can just add a VerifyCT() function that calls
cert_transparency_verifier_->Verify() and handles its results, and call
VerifyCT() here.

[Eran M. (Google), 2013/11/26 14:45:53]

Done - note that as the return type of VerifyCT is not used, I've declared it a
void one. Later, when we have higher confidence in SCT validation we could
propagate the error from CTVerifier::Verify.

+    return OK;
+  }
+
+  completed_handshake_ = true;
+
   // Exit DoHandshakeLoop and return the result to the caller to Connect.
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
 }
 
+int SSLClientSocketNSS::DoVerifyCT(int result) {
+  GotoState(STATE_VERIFY_CT_COMPLETE);
+
+  if (!cert_transparency_verifier_)
+    return OK;
+
+  // Note that this is a completely synchronic operation: The CT Log Verifier

[wtc, 2013/11/26 01:47:23]

synchronic => synchronous

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+  // gets all the data it needs for SCT verification and does not do any
+  // external communication

[wtc, 2013/11/26 01:47:23]

Nit: missing a period (.)

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+  return cert_transparency_verifier_->Verify(
+      server_cert_verify_result_.verified_cert,
+      std::string(), // SCT list from OCSP stapling response

[wtc, 2013/11/26 01:47:23]

Nit: remove "stapling"

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+      std::string(),  // SCT list from TLS handshake

[wtc, 2013/11/26 01:47:23]

Nit: TLS handshake => TLS extension

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+      &ct_verify_result_);
+}
+
+int SSLClientSocketNSS::DoVerifyCTComplete(int result) {
+  VLOG(1) << "CT Verification complete: result " << result
+          << " Unverified scts: " << ct_verify_result_.unverified_scts.size()
+          << " Verified scts: " << ct_verify_result_.verified_scts.size()
+          << " scts from unknown logs: "
+          << ct_verify_result_.unknown_logs_scts.size();
+
+  if (!ct_verify_result_.unverified_scts.empty() ||
+      !ct_verify_result_.unknown_logs_scts.empty() ||
+      !ct_verify_result_.verified_scts.empty()) {
+

[wtc, 2013/11/26 01:47:23]

Nit: delete this blank line.

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+    // Saving CT state in cert_status bits, in addition to the SCTs themselves
+    // (which will be threaded into the SSLInfo, as well as into the HTTP
+    // cache).
+    // This persists the CT status and simplifies UI code for figuring out
+    // the right CT info to display.
+    bool has_verified_scts = !ct_verify_result_.verified_scts.empty() &&
+        result == OK;
+    if (has_verified_scts || !ct_verify_result_.unverified_scts.empty()) {
+      // Found SCTs from a known log.
+      server_cert_verify_result_.cert_status =
+        CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;

[wtc, 2013/11/26 01:47:23]

BUG: use |= instead of = here and on line 3526. (Line 3520 is correct.)

This line (a continuation line) should be indented by four spaces.

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+      if (has_verified_scts) {
+        server_cert_verify_result_.cert_status |= CERT_STATUS_HAS_GOOD_SCT;
+      }

[wtc, 2013/11/26 01:47:23]

Omit curly braces when the if statement's body is one line.

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+    } else {
+      DCHECK(!ct_verify_result_.unknown_logs_scts.empty());
+      // When this bit is set but CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG isn't

[wtc, 2013/11/26 01:47:23]

Nit: add a comma (,) at the end of this line.

[Eran M. (Google), 2013/11/26 14:45:53]

Done.

+      // that implies SCTs from unknown logs.
+      server_cert_verify_result_.cert_status = CERT_STATUS_HAS_GOOD_SCT;
+    }
+  }
+
+  completed_handshake_ = true;
+  DCHECK_EQ(STATE_NONE, next_handshake_state_);
+
+  return OK;
+}
+
 void SSLClientSocketNSS::LogConnectionTypeMetrics() const {
   UpdateConnectionTypeHistograms(CONNECTION_SSL);
   int ssl_version = SSLConnectionStatusToVersion(