Upstream: Build net_unittests for Android.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index a2e3a1974b31c3d4972db78155ab964e836c90e2..597133b72c4d1c2c71937a3716d14ec273467e0d 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -798,10 +798,24 @@ int SSLClientSocketOpenSSL::SelectNextProtoCallback(unsigned char** out,
     return SSL_TLSEXT_ERR_OK;
   }
 
-  int status = SSL_select_next_proto(
-      out, outlen, in, inlen,
-      reinterpret_cast<const unsigned char*>(ssl_config_.next_protos.data()),
-      ssl_config_.next_protos.size());
+  int status = OPENSSL_NPN_NO_OVERLAP;
+  *out = (unsigned char *) in + 1;

[Ryan Sleevi, 2011/11/06 02:39:48]

BUG? Without checking the implementation of SSL_select_next_proto, what happens
if |in| or |inlen| are small (hostile? incorrect?) values.

Paranoid example, |inlen| == 1, but |*in| == 5. That means line 820 will attempt
to read an invalid range.

Is this input already validated for correctness? Should/can you document this?


style nit: Chromium/Google open-source style uses C++ casts, as convenient as
the C-style cast may be

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

*out = reinterpret_cast<unsigned char*>(in) + i + 1;

This is consistent with say, lines 795 and 820.

[Jing Zhao, 2011/11/07 03:42:49]

Done.

+  *outlen = in[0];
+  for (unsigned int i = 0; i < inlen; i += in[i] + 1) {
+    for (std::vector<std::string>::const_iterator
+         j = ssl_config_.next_protos.begin();
+         j != ssl_config_.next_protos.end(); j++) {

[Ryan Sleevi, 2011/11/06 02:39:48]

nit: ++j

[Jing Zhao, 2011/11/07 03:42:49]

Done.

+      if (in[i] == j->size() &&
+          memcmp(&in[i + 1], j->data(), in[i]) == 0) {

[Ryan Sleevi, 2011/11/06 02:39:48]

I'm a little nervous, quite possibly unreasonably so, when I see a straight up
memcmp in crypto/security code. While my gut says this probably has no security
relevance (and is thus OK), can you double check with the implementation (say,
SSL_select_next_proto) to see if this needs to be using a constant-time
comparison?

[Jing Zhao, 2011/11/07 03:42:49]

Yes, I double checked the implementation from
third_party/openssl/openssl/ssl/ssl_lib.c, Line 2798~2832. I use the exact same
memcmp as it uses.

[joth, 2011/11/08 10:05:23]

FWIW, as we have a more expressive class library than in the guts of openssl, we
could recode this as:

base::StringPiece in_list(in, inlen);
for(i = 0; i < in_list.size(); i += in_list[i] + 1) {
  for(j ... ) {
    base::StringPiece in_proto(in_list.substring(i + 1, in_list[i]));
    if (in_proto == *j) {
      // We found a match...
      *out = in_proto.data();
      *outlen = in_proto.size();
      status = OPENSSL_NPN_NEGOTIATED;
      goto done;
  }
}
done:
...

[Ryan Sleevi, 2011/11/08 15:03:23]

+1 to using StringPiece - it would definitely help here for both safety and
readability. Not critical for landing this, however, but an ideal cleanup
candidate.

+        *out = (unsigned char *) in + i + 1;
+        *outlen = in[i];
+        status = OPENSSL_NPN_NEGOTIATED;
+        break;
+      }
+    }
+    if (status == OPENSSL_NPN_NEGOTIATED)
+      break;
+  }
 
   npn_proto_.assign(reinterpret_cast<const char*>(*out), *outlen);
   switch (status) {