Upstream: Build net_unittests for Android.

net/socket/ssl_client_socket_openssl.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <openssl/ssl.h>
@@ skipping 780 matching lines @@
                                                     const unsigned char* in,
                                                     unsigned int inlen) {
 #if defined(OPENSSL_NPN_NEGOTIATED)
   if (ssl_config_.next_protos.empty()) {
     *out = reinterpret_cast<uint8*>(const_cast<char*>("http/1.1"));
     *outlen = 8;
     npn_status_ = SSLClientSocket::kNextProtoUnsupported;
     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) {
     case OPENSSL_NPN_UNSUPPORTED:
       npn_status_ = SSLClientSocket::kNextProtoUnsupported;
       break;
     case OPENSSL_NPN_NEGOTIATED:
       npn_status_ = SSLClientSocket::kNextProtoNegotiated;
       break;
     case OPENSSL_NPN_NO_OVERLAP:
@@ skipping 411 matching lines @@
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
   int err = SSL_get_error(ssl_, rv);
   return MapOpenSSLError(err, err_tracer);
 }
 
 }  // namespace net