Relanding: Allow the DTLS fingerprint verification to occur after the handshake.

webrtc/base/opensslstreamadapter.cc

Index: webrtc/base/opensslstreamadapter.cc
diff --git a/webrtc/base/opensslstreamadapter.cc b/webrtc/base/opensslstreamadapter.cc
index e04eb04d67c8d05eee38938f406484ccbe5d5247..84de88601db62b41ac21979441d1493f7345a034 100644
--- a/webrtc/base/opensslstreamadapter.cc
+++ b/webrtc/base/opensslstreamadapter.cc
@@ -25,6 +25,7 @@
 #include <memory>
 #include <vector>
 
+#include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/safe_conversions.h"
@@ -293,7 +294,7 @@ OpenSSLStreamAdapter::~OpenSSLStreamAdapter() {
 }
 
 void OpenSSLStreamAdapter::SetIdentity(SSLIdentity* identity) {
-  ASSERT(!identity_);
+  RTC_DCHECK(!identity_);
   identity_.reset(static_cast<OpenSSLIdentity*>(identity));
 }
 
@@ -313,21 +314,39 @@ bool OpenSSLStreamAdapter::SetPeerCertificateDigest(const std::string
                                                     const unsigned char*
                                                     digest_val,
                                                     size_t digest_len) {
-  ASSERT(!peer_certificate_);
-  ASSERT(peer_certificate_digest_algorithm_.size() == 0);
-  ASSERT(ssl_server_name_.empty());
+  RTC_DCHECK(!peer_certificate_verified_);
+  RTC_DCHECK(!have_peer_certificate_digest());
+  RTC_DCHECK(!verify_certificate_using_server_name());
   size_t expected_len;
 
   if (!OpenSSLDigest::GetDigestSize(digest_alg, &expected_len)) {
     LOG(LS_WARNING) << "Unknown digest algorithm: " << digest_alg;
     return false;
   }
-  if (expected_len != digest_len)
+  if (expected_len != digest_len) {
     return false;
+  }
 
   peer_certificate_digest_value_.SetData(digest_val, digest_len);
   peer_certificate_digest_algorithm_ = digest_alg;
 
+  if (!peer_certificate_) {
+    // Normal case, where the digest is set before we obtain the certificate
+    // from the handshake.
+    return true;
+  }
+
+  if (!VerifyPeerCertificate()) {
+    Error("SetPeerCertificateDigest", -1, false);
+    return false;

[pthatcher1, 2016/07/27 19:32:35]

This would cause SetRemoteDescription to fail based upon whether or not the
handshake has completed.  That seems like a problem.  Wouldn't instead we want
SetRemoteDescription to succeed, but have the transport go into a failed state?

[Taylor Brandstetter, 2016/08/13 00:09:53]

I think you meant for this comment to be in DtlsTransportChannel? But yes I
agree that having variable results from SetRemoteDescription depending on timing
is something applications won't expect (and isn't mentioned by JSEP). I'll fix
this and update the test.

+  }
+
+  if (state_ == SSL_CONNECTED) {
+    // Post event to unwind stack. Caller may not be expecting this event
+    // to occur synchronously.
+    PostEvent(SE_OPEN | SE_READ | SE_WRITE, 0);

[pthatcher1, 2016/07/27 19:32:35]

It's not very clear what's going on here.  It appears that ContinueSSL(), which
sets the state_ = SSL_CONNECTED sets the state of the adapter to
open/read/write, but synchronously, but that would be bad for some callers of
SetPeerCertificateDigest, so we post an even that amounts to "call ContinueSSL()
later".   Is that accurate?  If so, can you make that a little more clear?

[Taylor Brandstetter, 2016/08/13 00:09:53]

ContinueSSL should have been posting the event asynchronously as well. It's just
a bug no one noticed (until you did, thanks) and happened to not have any
consequences. Fixed now.

+  }
+
   return true;
 }
 
@@ -450,7 +469,7 @@ bool OpenSSLStreamAdapter::SetDtlsSrtpCryptoSuites(
 
 bool OpenSSLStreamAdapter::GetDtlsSrtpCryptoSuite(int* crypto_suite) {
 #ifdef HAVE_DTLS_SRTP
-  ASSERT(state_ == SSL_CONNECTED);
+  RTC_DCHECK(state_ == SSL_CONNECTED);
   if (state_ != SSL_CONNECTED)
     return false;
 
@@ -461,7 +480,7 @@ bool OpenSSLStreamAdapter::GetDtlsSrtpCryptoSuite(int* crypto_suite) {
     return false;
 
   *crypto_suite = srtp_profile->id;
-  ASSERT(!SrtpCryptoSuiteToName(*crypto_suite).empty());
+  RTC_DCHECK(!SrtpCryptoSuiteToName(*crypto_suite).empty());
   return true;
 #else
   return false;
@@ -469,24 +488,32 @@ bool OpenSSLStreamAdapter::GetDtlsSrtpCryptoSuite(int* crypto_suite) {
 }
 
 int OpenSSLStreamAdapter::StartSSLWithServer(const char* server_name) {
-  ASSERT(server_name != NULL && server_name[0] != '\0');
+  if (state_ != SSL_NONE) {
+    // Don't allow StartSSL to be called twice.
+    return -1;
+  }
+  RTC_DCHECK(server_name != NULL && server_name[0] != '\0');
   ssl_server_name_ = server_name;
   return StartSSL();
 }
 
 int OpenSSLStreamAdapter::StartSSLWithPeer() {
-  ASSERT(ssl_server_name_.empty());
+  if (state_ != SSL_NONE) {
+    // Don't allow StartSSL to be called twice.
+    return -1;
+  }
+  RTC_DCHECK(ssl_server_name_.empty());
   // It is permitted to specify peer_certificate_ only later.
   return StartSSL();
 }
 
 void OpenSSLStreamAdapter::SetMode(SSLMode mode) {
-  ASSERT(state_ == SSL_NONE);
+  RTC_DCHECK(state_ == SSL_NONE);
   ssl_mode_ = mode;
 }
 
 void OpenSSLStreamAdapter::SetMaxProtocolVersion(SSLProtocolVersion version) {
-  ASSERT(ssl_ctx_ == NULL);
+  RTC_DCHECK(ssl_ctx_ == NULL);
   ssl_max_version_ = version;
 }
 
@@ -508,6 +535,9 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data, size_t data_len,
     return SR_BLOCK;
 
   case SSL_CONNECTED:
+    if (waiting_to_verify_client_cert()) {
+      return SR_BLOCK;
+    }
     break;
 
   case SSL_ERROR:
@@ -532,7 +562,7 @@ StreamResult OpenSSLStreamAdapter::Write(const void* data, size_t data_len,
   switch (ssl_error) {
   case SSL_ERROR_NONE:
     LOG(LS_VERBOSE) << " -- success";
-    ASSERT(0 < code && static_cast<unsigned>(code) <= data_len);
+    RTC_DCHECK(0 < code && static_cast<unsigned>(code) <= data_len);
     if (written)
       *written = code;
     return SR_SUCCESS;
@@ -567,6 +597,9 @@ StreamResult OpenSSLStreamAdapter::Read(void* data, size_t data_len,
       return SR_BLOCK;
 
     case SSL_CONNECTED:
+      if (waiting_to_verify_client_cert()) {
+        return SR_BLOCK;
+      }
       break;
 
     case SSL_CLOSED:
@@ -593,7 +626,7 @@ StreamResult OpenSSLStreamAdapter::Read(void* data, size_t data_len,
   switch (ssl_error) {
     case SSL_ERROR_NONE:
       LOG(LS_VERBOSE) << " -- success";
-      ASSERT(0 < code && static_cast<unsigned>(code) <= data_len);
+      RTC_DCHECK(0 < code && static_cast<unsigned>(code) <= data_len);
       if (read)
         *read = code;
 
@@ -644,7 +677,7 @@ void OpenSSLStreamAdapter::FlushInput(unsigned int left) {
     int code = SSL_read(ssl_, buf, toread);
 
     int ssl_error = SSL_get_error(ssl_, code);
-    ASSERT(ssl_error == SSL_ERROR_NONE);
+    RTC_DCHECK(ssl_error == SSL_ERROR_NONE);
 
     if (ssl_error != SSL_ERROR_NONE) {
       LOG(LS_VERBOSE) << " -- error " << code;
@@ -659,7 +692,7 @@ void OpenSSLStreamAdapter::FlushInput(unsigned int left) {
 
 void OpenSSLStreamAdapter::Close() {
   Cleanup();
-  ASSERT(state_ == SSL_CLOSED || state_ == SSL_ERROR);
+  RTC_DCHECK(state_ == SSL_CLOSED || state_ == SSL_ERROR);
   StreamAdapterInterface::Close();
 }
 
@@ -669,6 +702,9 @@ StreamState OpenSSLStreamAdapter::GetState() const {
     case SSL_CONNECTING:
       return SS_OPENING;
     case SSL_CONNECTED:
+      if (waiting_to_verify_client_cert()) {
+        return SS_OPENING;
+      }
       return SS_OPEN;
     default:
       return SS_CLOSED;
@@ -680,11 +716,11 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream, int events,
                                    int err) {
   int events_to_signal = 0;
   int signal_error = 0;
-  ASSERT(stream == this->stream());
+  RTC_DCHECK(stream == this->stream());
   if ((events & SE_OPEN)) {
     LOG(LS_VERBOSE) << "OpenSSLStreamAdapter::OnEvent SE_OPEN";
     if (state_ != SSL_WAIT) {
-      ASSERT(state_ == SSL_NONE);
+      RTC_DCHECK(state_ == SSL_NONE);
       events_to_signal |= SE_OPEN;
     } else {
       state_ = SSL_CONNECTING;
@@ -723,7 +759,7 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream, int events,
     Cleanup();
     events_to_signal |= SE_CLOSE;
     // SE_CLOSE is the only event that uses the final parameter to OnEvent().
-    ASSERT(signal_error == 0);
+    RTC_DCHECK(signal_error == 0);
     signal_error = err;
   }
   if (events_to_signal)
@@ -731,7 +767,7 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream, int events,
 }
 
 int OpenSSLStreamAdapter::StartSSL() {
-  ASSERT(state_ == SSL_NONE);
+  RTC_DCHECK(state_ == SSL_NONE);
 
   if (StreamAdapterInterface::GetState() != SS_OPEN) {
     state_ = SSL_WAIT;
@@ -748,19 +784,19 @@ int OpenSSLStreamAdapter::StartSSL() {
 }
 
 int OpenSSLStreamAdapter::BeginSSL() {
-  ASSERT(state_ == SSL_CONNECTING);
-  // The underlying stream has open. If we are in peer-to-peer mode
-  // then a peer certificate must have been specified by now.
-  ASSERT(!ssl_server_name_.empty() ||
-         !peer_certificate_digest_algorithm_.empty());
+  RTC_DCHECK(state_ == SSL_CONNECTING);
+  // At this point, we should have selected one (and only one) way of
+  // validating the peer's certificate.
+  RTC_DCHECK(verify_certificate_using_server_name() !=
+             verify_certificate_using_peer_digest());

[pthatcher1, 2016/07/27 19:32:35]

Doesn't this equate to the following?

RTC_DCHECK(!ssl_server_name_.empty() != ssl_server_name_.empty());


That doesn't seem like a useful DCHECK.  Or am I missing something?

[Taylor Brandstetter, 2016/08/13 00:09:53]

This checks that one and only one "mode" has been determined. Of course, right
now the DCHECK will always pass. The point of DCHECKS isn't to validate the
external inputs of a class, but to enforce and document implicit assumptions,
both now and into the future. If we later changed the implementation of
"verify_certificate_using_*", and this assumption no longer held, this DCHECK
would catch the problem early.

Of course, this is invalidated now that StartSSLWithServer is gone. But I
thought I'd explain the reasoning.

   LOG(LS_INFO) << "BeginSSL: "
-               << (!ssl_server_name_.empty() ? ssl_server_name_ :
-                                               "with peer");
+               << (verify_certificate_using_server_name() ? ssl_server_name_

[pthatcher1, 2016/07/27 19:32:35]

We already verified the state_, so why not just use "in_peer_mode" here?

[Taylor Brandstetter, 2016/08/13 00:09:53]

I don't know what you mean. I'm using "verify_certificate_using_server_name()"
which is equivalent to "!in_peer_mode()".

+                                                          : "with peer");
 
   BIO* bio = NULL;
 
   // First set up the context
-  ASSERT(ssl_ctx_ == NULL);
+  RTC_DCHECK(ssl_ctx_ == NULL);
   ssl_ctx_ = SetupSSLContext();
   if (!ssl_ctx_)
     return -1;
@@ -814,7 +850,7 @@ int OpenSSLStreamAdapter::BeginSSL() {
 
 int OpenSSLStreamAdapter::ContinueSSL() {
   LOG(LS_VERBOSE) << "ContinueSSL";
-  ASSERT(state_ == SSL_CONNECTING);
+  RTC_DCHECK(state_ == SSL_CONNECTING);
 
   // Clear the DTLS timer
   Thread::Current()->Clear(this, MSG_TIMEOUT);
@@ -825,14 +861,19 @@ int OpenSSLStreamAdapter::ContinueSSL() {
     case SSL_ERROR_NONE:
       LOG(LS_VERBOSE) << " -- success";
 
-      if (!SSLPostConnectionCheck(ssl_, ssl_server_name_.c_str(), NULL,
-                                  peer_certificate_digest_algorithm_)) {
+      if (!SSLPostConnectionCheck()) {
         LOG(LS_ERROR) << "TLS post connection check failed";
         return -1;
       }
 
       state_ = SSL_CONNECTED;
-      StreamAdapterInterface::OnEvent(stream(), SE_OPEN|SE_READ|SE_WRITE, 0);
+      if (!waiting_to_verify_client_cert()) {
+        // We have everything we need to start the connection, so signal
+        // SE_OPEN. If we need a client certificate fingerprint and don't have
+        // it yet, we'll instead signal SE_OPEN in SetPeerCertificateDigest.
+        StreamAdapterInterface::OnEvent(stream(), SE_OPEN | SE_READ | SE_WRITE,
+                                        0);
+      }
       break;
 
     case SSL_ERROR_WANT_READ: {
@@ -1042,43 +1083,23 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() {
   return ctx;
 }
 
-int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
-  // Get our SSL structure from the store
-  SSL* ssl = reinterpret_cast<SSL*>(X509_STORE_CTX_get_ex_data(
-                                        store,
-                                        SSL_get_ex_data_X509_STORE_CTX_idx()));
-  OpenSSLStreamAdapter* stream =
-    reinterpret_cast<OpenSSLStreamAdapter*>(SSL_get_app_data(ssl));
-
-  if (stream->peer_certificate_digest_algorithm_.empty()) {
-    return 0;
-  }
-  X509* cert = X509_STORE_CTX_get_current_cert(store);
-  int depth = X509_STORE_CTX_get_error_depth(store);
-
-  // For now We ignore the parent certificates and verify the leaf against
-  // the digest.
-  //
-  // TODO(jiayl): Verify the chain is a proper chain and report the chain to
-  // |stream->peer_certificate_|.
-  if (depth > 0) {
-    LOG(LS_INFO) << "Ignored chained certificate at depth " << depth;
-    return 1;
+bool OpenSSLStreamAdapter::VerifyPeerCertificate() {
+  if (!have_peer_certificate_digest() || !peer_certificate_) {
+    LOG(LS_WARNING) << "Missing digest or peer certificate.";
+    return false;
   }
 
   unsigned char digest[EVP_MAX_MD_SIZE];
   size_t digest_length;
   if (!OpenSSLCertificate::ComputeDigest(
-           cert,
-           stream->peer_certificate_digest_algorithm_,
-           digest, sizeof(digest),
-           &digest_length)) {
+          peer_certificate_->x509(), peer_certificate_digest_algorithm_, digest,
+          sizeof(digest), &digest_length)) {
     LOG(LS_WARNING) << "Failed to compute peer cert digest.";
-    return 0;
+    return false;
   }
 
   Buffer computed_digest(digest, digest_length);
-  if (computed_digest != stream->peer_certificate_digest_value_) {
+  if (computed_digest != peer_certificate_digest_value_) {
     LOG(LS_WARNING) << "Rejected peer certificate due to mismatched digest.";
     return 0;
   }
@@ -1086,37 +1107,65 @@ int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
   // value in checking the validity of a self-signed cert issued by untrusted
   // sources.
   LOG(LS_INFO) << "Accepted peer certificate.";
+  peer_certificate_verified_ = true;
+  return true;
+}
+
+int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
+  // Get our SSL structure from the store
+  SSL* ssl = reinterpret_cast<SSL*>(
+      X509_STORE_CTX_get_ex_data(store, SSL_get_ex_data_X509_STORE_CTX_idx()));
+  X509* cert = X509_STORE_CTX_get_current_cert(store);
+  int depth = X509_STORE_CTX_get_error_depth(store);
+
+  // For now we ignore the parent certificates and verify the leaf against
+  // the digest.
+  //
+  // TODO(jiayl): Verify the chain is a proper chain and report the chain to
+  // |stream->peer_certificate_|.
+  if (depth > 0) {
+    LOG(LS_INFO) << "Ignored chained certificate at depth " << depth;
+    return 1;
+  }
+
+  OpenSSLStreamAdapter* stream =
+      reinterpret_cast<OpenSSLStreamAdapter*>(SSL_get_app_data(ssl));
 
   // Record the peer's certificate.
   stream->peer_certificate_.reset(new OpenSSLCertificate(cert));
-  return 1;
+
+  // If the peer certificate digest isn't known yet, we'll wait to verify
+  // until it's known, and for now just return a success status.
+  if (stream->peer_certificate_digest_algorithm_.empty()) {
+    LOG(LS_INFO) << "Waiting to verify certificate until digest is known.";
+    return 1;
+  }
+
+  return stream->VerifyPeerCertificate();
 }
 
 // This code is taken from the "Network Security with OpenSSL"
 // sample in chapter 5
-bool OpenSSLStreamAdapter::SSLPostConnectionCheck(SSL* ssl,
-                                                  const char* server_name,
-                                                  const X509* peer_cert,
-                                                  const std::string
-                                                  &peer_digest) {
-  ASSERT(server_name != NULL);
+bool OpenSSLStreamAdapter::SSLPostConnectionCheck() {
   bool ok;
-  if (server_name[0] != '\0') {  // traditional mode
-    ok = OpenSSLAdapter::VerifyServerName(ssl, server_name, ignore_bad_cert());
+  if (verify_certificate_using_server_name()) {
+    RTC_DCHECK(!ssl_server_name_.empty());
+    ok = OpenSSLAdapter::VerifyServerName(ssl_, ssl_server_name_.c_str(),
+                                          ignore_bad_cert());
 
     if (ok) {
-      ok = (SSL_get_verify_result(ssl) == X509_V_OK ||
+      ok = (SSL_get_verify_result(ssl_) == X509_V_OK ||
             custom_verification_succeeded_);
     }
-  } else {  // peer-to-peer mode
-    ASSERT((peer_cert != NULL) || (!peer_digest.empty()));
+  } else {
+    RTC_DCHECK(verify_certificate_using_peer_digest());
     // no server name validation
-    ok = true;
+    ok = peer_certificate_ || !client_auth_enabled();
   }
 
   if (!ok && ignore_bad_cert()) {
-    LOG(LS_ERROR) << "SSL_get_verify_result(ssl) = "
-                  << SSL_get_verify_result(ssl);
+    LOG(LS_ERROR) << "SSL_get_verify_result(ssl_) = "
+                  << SSL_get_verify_result(ssl_);
     LOG(LS_INFO) << "Other TLS post connection checks failed.";
     ok = true;
   }