OpenSSL/NSS implementation of ProofVerfifier.

net/quic/quic_crypto_client_stream.cc

Index: net/quic/quic_crypto_client_stream.cc
diff --git a/net/quic/quic_crypto_client_stream.cc b/net/quic/quic_crypto_client_stream.cc
index 674a441e7971a0b62e9c6ee49486139a489cfcbf..870664e669418559c67e31c6d7c967c02e434597 100644
--- a/net/quic/quic_crypto_client_stream.cc
+++ b/net/quic/quic_crypto_client_stream.cc
@@ -4,6 +4,8 @@
 
 #include "net/quic/quic_crypto_client_stream.h"
 
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
 #include "net/quic/crypto/crypto_protocol.h"
 #include "net/quic/crypto/crypto_utils.h"
 #include "net/quic/crypto/null_encrypter.h"
@@ -18,6 +20,7 @@ QuicCryptoClientStream::QuicCryptoClientStream(
     QuicSession* session,
     QuicCryptoClientConfig* crypto_config)
     : QuicCryptoStream(session),
+      weak_factory_(this),
       next_state_(STATE_IDLE),
       num_client_hellos_(0),
       crypto_config_(crypto_config),
@@ -29,12 +32,12 @@ QuicCryptoClientStream::~QuicCryptoClientStream() {
 
 void QuicCryptoClientStream::OnHandshakeMessage(
     const CryptoHandshakeMessage& message) {
-  DoHandshakeLoop(&message);
+  DoHandshakeLoop(&message, OK);
 }
 
 bool QuicCryptoClientStream::CryptoConnect() {
   next_state_ = STATE_SEND_CHLO;
-  DoHandshakeLoop(NULL);
+  DoHandshakeLoop(NULL, OK);
   return true;
 }
 
@@ -50,7 +53,8 @@ int QuicCryptoClientStream::num_sent_client_hellos() const {
 static const int kMaxClientHellos = 3;
 
 void QuicCryptoClientStream::DoHandshakeLoop(
-    const CryptoHandshakeMessage* in) {
+    const CryptoHandshakeMessage* in,
+    int result) {
   CryptoHandshakeMessage out;
   QuicErrorCode error;
   string error_details;
@@ -139,28 +143,34 @@ void QuicCryptoClientStream::DoHandshakeLoop(
           return;
         }
         if (!cached->proof_valid()) {
-          const ProofVerifier* verifier = crypto_config_->proof_verifier();
-          if (!verifier) {
-            // If no verifier is set then we don't check the certificates.
-            cached->SetProofValid();
-          } else if (!cached->signature().empty()) {
-            // TODO(rtenneti): In Chromium, we will need to make VerifyProof()
-            // asynchronous.
-            if (!verifier->VerifyProof(server_hostname_,
-                                       cached->server_config(),
-                                       cached->certs(),
-                                       cached->signature(),
-                                       &error_details)) {
-              CloseConnectionWithDetails(QUIC_PROOF_INVALID,
-                                         "Proof invalid: " + error_details);
+          ProofVerifier* verifier = crypto_config_->proof_verifier();
+          if (verifier && !cached->signature().empty()) {
+            result = verifier->VerifyProof(
+                server_hostname_,
+                cached->server_config(),
+                cached->certs(),
+                cached->signature(),
+                base::Bind(&QuicCryptoClientStream::VerifyProofCompleted,

[agl2, 2013/06/28 20:30:09]

likewise, base::Bind can't be used in this code either, sadly.

[ramant (doing other things), 2013/06/29 04:08:44]

Will use internal API call here.

+                           weak_factory_.GetWeakPtr()));
+            if (result == ERR_IO_PENDING) {
+              next_state_ = STATE_PROOF_VERIFY;
+              DVLOG(1) << "Doing VerifyProof";
               return;
             }
-            cached->SetProofValid();
           }
         }
+        next_state_ = STATE_PROOF_VERIFICATION_COMPLETED;
+        break;
+      case STATE_PROOF_VERIFICATION_COMPLETED:
+        if (result != OK) {
+          error_details = crypto_config_->proof_verifier()->error_details();
+          CloseConnectionWithDetails(QUIC_PROOF_INVALID,
+                                     "Proof invalid: " + error_details);
+          return;
+        }
+        cached->SetProofValid();

[agl2, 2013/06/28 20:30:09]

This is insecure:

In the time that a proof was being verified, another connection to the same
origin may have started and receive different certificates. Thus the proof that
this line is marking as valid may not be the proof that was passed to
VerifyProof.

verifier needs to implement a cache (I think there already is one under the
covers so that should be a no-op). I think there needs to be a
STATE_PROOF_VERIFY containing just the VerifyProof call. If VerifyProof needs to
block then the handshake state machine should return to STATE_PROOF_VERIFY and
call VerifyProof again. If the certs have changed, then it'll verify the new
ones. If they are the same then it'll hit the cache and return immediately.

[ramant (doing other things), 2013/06/29 04:08:44]

Done.

         // Send the subsequent client hello in plaintext.
-        session()->connection()->SetDefaultEncryptionLevel(
-            ENCRYPTION_NONE);
+        session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);
         next_state_ = STATE_SEND_CHLO;
         break;
       case STATE_RECV_SHLO: {
@@ -228,8 +238,17 @@ void QuicCryptoClientStream::DoHandshakeLoop(
         // This means that the peer sent us a message that we weren't expecting.
         CloseConnection(QUIC_INVALID_CRYPTO_MESSAGE_TYPE);
         return;
+      case STATE_PROOF_VERIFY:
+        NOTREACHED() << "STATE_PROOF_VERIFY shouldn't be reached";
+        return;
     }
   }
 }
 
+void QuicCryptoClientStream::VerifyProofCompleted(int result) {
+  DVLOG(1) << "VerifyProof completed: " << result;
+  next_state_ = STATE_PROOF_VERIFICATION_COMPLETED;
+  DoHandshakeLoop(NULL, result);
+}
+
 }  // namespace net