net: rework the NPN patch.

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 79fd69a407d23a159ae017ac16a58b69b89afaba..b3d3c08a619facc44efa9d7f4a479feebdd95a02 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -465,6 +465,7 @@ SSLClientSocketNSS::SSLClientSocketNSS(ClientSocketHandle* transport_socket,
       net_log_(transport_socket->socket()->NetLog()),
       ssl_host_info_(ssl_host_info),
       dns_cert_checker_(context.dns_cert_checker),
+      next_proto_status_(kNextProtoUnsupported),
       valid_thread_id_(base::kInvalidThreadId) {
   EnterFunction("");
 }
@@ -552,38 +553,8 @@ int SSLClientSocketNSS::ExportKeyingMaterial(const base::StringPiece& label,
 
 SSLClientSocket::NextProtoStatus
 SSLClientSocketNSS::GetNextProto(std::string* proto) {
-#if defined(SSL_NEXT_PROTO_NEGOTIATED)
-  unsigned char buf[255];
-  int state;
-  unsigned len;
-  SECStatus rv = SSL_GetNextProto(nss_fd_, &state, buf, &len, sizeof(buf));
-  if (rv != SECSuccess) {
-    NOTREACHED() << "Error return from SSL_GetNextProto: " << rv;
-    proto->clear();
-    return kNextProtoUnsupported;
-  }
-  // We don't check for truncation because sizeof(buf) is large enough to hold
-  // the maximum protocol size.
-  switch (state) {
-    case SSL_NEXT_PROTO_NO_SUPPORT:
-      proto->clear();
-      return kNextProtoUnsupported;
-    case SSL_NEXT_PROTO_NEGOTIATED:
-      *proto = std::string(reinterpret_cast<char*>(buf), len);
-      return kNextProtoNegotiated;
-    case SSL_NEXT_PROTO_NO_OVERLAP:
-      *proto = std::string(reinterpret_cast<char*>(buf), len);
-      return kNextProtoNoOverlap;
-    default:
-      NOTREACHED() << "Unknown status from SSL_GetNextProto: " << state;
-      proto->clear();
-      return kNextProtoUnsupported;
-  }
-#else
-  // No NPN support in the libssl that we are building with.
-  proto->clear();
-  return kNextProtoUnsupported;
-#endif
+  *proto = next_proto_;
+  return next_proto_status_;
 }
 
 int SSLClientSocketNSS::Connect(OldCompletionCallback* callback) {
@@ -963,14 +934,12 @@ int SSLClientSocketNSS::InitializeSSLOptions() {
   }
 #endif  // SSL_ENABLE_RENEGOTIATION
 
-#ifdef SSL_NEXT_PROTO_NEGOTIATED
+#ifdef SSL_NEXT_PROTO_NEGOTIATION_SUPPORTED
   if (!ssl_config_.next_protos.empty()) {
-    rv = SSL_SetNextProtoNego(
-       nss_fd_,
-       reinterpret_cast<const unsigned char *>(ssl_config_.next_protos.data()),
-       ssl_config_.next_protos.size());
+    rv = SSL_SetNextProtoCallback(
+        nss_fd_, SSLClientSocketNSS::NextProtoCallback, this);
     if (rv != SECSuccess)
-      LogFailedNSSFunction(net_log_, "SSL_SetNextProtoNego", "");
+      LogFailedNSSFunction(net_log_, "SSL_SetNextProtoCallback", "");
   }
 #endif
 
@@ -2517,6 +2486,58 @@ void SSLClientSocketNSS::HandshakeCallback(PRFileDesc* socket,
   that->UpdateConnectionStatus();
 }
 
+// NextProtoCallback is called by NSS during the handshake, if the server
+// supports NPN, to select a protocol from the list that the server offered.
+// See the comment in net/third_party/nss/ssl/ssl.h for the meanings of the
+// arguments.
+// static
+SECStatus
+SSLClientSocketNSS::NextProtoCallback(void* arg,
+                                      PRFileDesc* nss_fd,
+                                      const unsigned char* protos,
+                                      unsigned short protos_len,
+                                      unsigned char* proto_out,
+                                      unsigned char* proto_out_len) {
+  SSLClientSocketNSS* that = reinterpret_cast<SSLClientSocketNSS*>(arg);
+
+  // For each protocol in server preference, see if we support it.
+  for (unsigned int i = 0; i < protos_len; ) {
+    const size_t len = protos[i];
+    for (std::vector<std::string>::const_iterator
+         j = that->ssl_config_.next_protos.begin();
+         j != that->ssl_config_.next_protos.end(); j++) {
+      // Having very long elements in the |next_protos| vector isn't a disaster
+      // because they'll never be selected, but it does indicate an error
+      // somewhere.
+      DCHECK_LT(j->size(), 256u);
+
+      if (j->size() == len &&
+          memcmp(&protos[i+1], j->data(), len) == 0) {
+        that->next_proto_status_ = kNextProtoNegotiated;
+        that->next_proto_ = *j;
+        break;
+      }
+    }
+
+    if (that->next_proto_status_ == kNextProtoNegotiated)
+      break;
+
+    // NSS checks that the data in |protos| is well formed, so we know that
+    // this doesn't cause us to jump off the end of the buffer.
+    i += len + 1;
+  }
+
+  // If we didn't find a protocol, we select the first one from our list.
+  if (that->next_proto_status_ != kNextProtoNegotiated) {
+    that->next_proto_status_ = kNextProtoNoOverlap;
+    that->next_proto_ = that->ssl_config_.next_protos[0];
+  }
+
+  memcpy(proto_out, that->next_proto_.data(), that->next_proto_.size());
+  *proto_out_len = that->next_proto_.size();
+  return SECSuccess;
+}

[wtc, 2011/10/05 23:34:25]

Rather than having every application implement the callback,
can NSS provide a default NextProtoCallback appropriate for
most applications?  Then only the apps that custom protocol
selection logic need to install their own NextProtoCallbacks.

[agl, 2011/10/07 19:19:17]

I tried writing this function, but it quickly becomes messy.

Writing a function which matches the callback interface isn't nice because it
needs to return several other bits of information to the application: the status
and the matched protocol.

the |arg| value could be casted to a structure like:

struct SSLNextProtoCallbackState {
  const unsigned char* client_protos;
  unsigned client_protos_len;
  
  unsigned out_proto_len;
  unsigned char out_proto[256];
  SSLNextProtoState state;
};

But then using this is almost as hard as writing the callback oneself, since the
application is probably written in C++ these days and will have to marshal the
client_protos in any case.

[wtc, 2011/10/11 22:27:57]

Thanks a lot for looking into this suggestion.  I came to
the same conclusion that it doesn't make sense for NSS to
provide a default NextProtoCallback.  Instead, NSS should
continue to provide the SSL_SetNextProtoNego and
SSL_GetNextProto functions, which an application should
use if it does not give NSS a NextProtoCallback.

What do you think?  I think it is useful for NSS to
help most applications use NPN easily.

+
 void SSLClientSocketNSS::EnsureThreadIdAssigned() const {
   base::AutoLock auto_lock(lock_);
   if (valid_thread_id_ != base::kInvalidThreadId)